Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ITF serialization of Values #36

Merged
merged 7 commits into from
May 7, 2024
Merged

ITF serialization of Values #36

merged 7 commits into from
May 7, 2024

Conversation

Kukovec
Copy link
Collaborator

@Kukovec Kukovec commented Apr 22, 2024

closes #35

Introduces the following:

  • the State type
  • A rudimentary type system, notationally based on that of Apalache, with type literals from Soroban (u32, symb, etc.)
  • An ITF representation (of a single state) as a length-1 trace
  • modifies validity checks on collections, to require homogeneous contents, unless explicitly allowed otherwise.

Copy link
Collaborator

@thpani thpani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A first set of comments.

The ITF-serialization of state looks good.

I've not had time to look at all the code for supporting nonhomogenous collections.

solarkraft/test/io/itf.test.ts Outdated Show resolved Hide resolved
solarkraft/test/io/itf.test.ts Outdated Show resolved Hide resolved
solarkraft/test/io/itf.test.ts Outdated Show resolved Hide resolved
solarkraft/test/state/value.test.ts Outdated Show resolved Hide resolved
Comment on lines 41 to 43
case 'arr':
// array of 0/1 s, must be wrapped
return v.val.map(numWrap)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaict, for a byte[] = 101 this would give

[{
  "#bigint": "1"
}, {
  "#bigint": "0"
}, {
  "#bigint": "1"
}]

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I did it for consistency, but for Bytes specifically, we could deviate from ITF and just use the JS numbers, since it's only 0s and 1s, b/c I do agree that it's a bit clunky.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's an array of zeroes and ones, why would not we just wrap it as a string, e.g., by introducing a new kinds of a field { "#bytes": "101" }, or by just using an array of numbers, as you are suggesting. Actually, ITF does not prohibit JS numbers from being used for the small ranges.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we had a discussion about standardizing this in Apalache where every value was #bigint. But yeah, we don't have to 100% copy ITF here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we encode a bytes[] as bits in the first place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because I'm illiterate apparently

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this question resolved? I am not sure I understand the last comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I fixed it. bytes = number now, and the validations check for 2^8

solarkraft/src/io/itf.ts Outdated Show resolved Hide resolved
solarkraft/src/io/itf.ts Outdated Show resolved Hide resolved
solarkraft/src/state/value.ts Outdated Show resolved Hide resolved
solarkraft/src/state/value.ts Show resolved Hide resolved
Comment on lines 45 to 46
case 'arr':
return 'Bytes'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is 'arr' different from the other basic types?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a comment about this inline, but I wanted to convey that Bytes, while it is a collection, is basically atomic as a type, since it's always an array of exactly 0s or 1s. I felt like using the word array/arr might give the impression that this is an array in the traditional sense, i.e. a fixed-sized ordered collection of arbitrary values, which is very much not supported (but there is Vec).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a comment on that. I think it would improve our understanding of the issue if you added a link to the original description of this datatypes in Stellar or Soroban SDK?

@Kukovec Kukovec requested a review from thpani April 23, 2024 11:50
Copy link
Contributor

@konnov konnov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments

Comment on lines 41 to 43
case 'arr':
// array of 0/1 s, must be wrapped
return v.val.map(numWrap)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's an array of zeroes and ones, why would not we just wrap it as a string, e.g., by introducing a new kinds of a field { "#bytes": "101" }, or by just using an array of numbers, as you are suggesting. Actually, ITF does not prohibit JS numbers from being used for the small ranges.

timeStyle: 'long',
})

// const desc = `Created by Solarkraft on ${formatter.format(new Date().getTime())}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// const desc = `Created by Solarkraft on ${formatter.format(new Date().getTime())}`

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, actually, considering the other response I made, it should be this line that stays uncommented and the line below removed. Either way, do you have a preference for one of the two approaches to dates?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idk. Perhaps, using the UTC date would be more consistent. I don't have any strong preference.

}

// Returns the full type annotation of the provided (possibly complex) Value `v`.
// It uses Apalache-style type notation, with Soroban type primitives.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is Apalache-style type notation? What we see below looks like Soroban to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, our "maps" are A -> B

@thpani
Copy link
Collaborator

thpani commented Apr 23, 2024

@Kukovec your last commit deleted ~2kLOC from the npm lockfile.
I doubt that's intended?

@Kukovec
Copy link
Collaborator Author

Kukovec commented Apr 23, 2024

@Kukovec your last commit deleted ~2kLOC from the npm lockfile. I doubt that's intended?

I have not touched it beyond what npm run test, npm run lint and git commit do, I have no why it's like that.

@konnov
Copy link
Contributor

konnov commented Apr 23, 2024

@Kukovec your last commit deleted ~2kLOC from the npm lockfile. I doubt that's intended?

I have not touched it beyond what npm run test, npm run lint and git commit do, I have no why it's like that.

Oh, that's indeed strange. It looks like all of yargs and eslint is about to be removed from package-lock.json. There are plenty of packages I have no idea about though.

@Kukovec
Copy link
Collaborator Author

Kukovec commented Apr 23, 2024

I just took the package-lock that was on remote main, still no idea what caused the deletion.

@Kukovec Kukovec requested a review from konnov May 6, 2024 14:23
Copy link
Contributor

@konnov konnov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left a few comments on the unresolved issues. I think we should resolve as much as we can asap and merge this PR.

Comment on lines 56 to 58
// As a type, `Bytes` is atomic, even though it is an array. We avoid `arr`, as it could imply
// a generic array type, which does not exist.
return 'Bytes'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand what it means for a type to be atomic. Atomicity means that a value is updated in a single machine instruction, or it is updated in a way such that another thread/process cannot interfere with an update. Do you mean that a value of type Bytes is opaque in the sense that it cannot be decomposed into smaller pieces? I am still not sure about the second sentence. What is a generic array type that does not exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Atomic as in the smallest type-term (or in this context, not a generic collection). E.g. Array[T] would be a generic array, which Bytes is not. There is Vec, which is a dynamic, non-fixed sized collection, but there are no arrays in the Rust sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll rewrite this a bit.

Comment on lines 45 to 46
case 'arr':
return 'Bytes'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a comment on that. I think it would improve our understanding of the issue if you added a link to the original description of this datatypes in Stellar or Soroban SDK?

timeStyle: 'long',
})

// const desc = `Created by Solarkraft on ${formatter.format(new Date().getTime())}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idk. Perhaps, using the UTC date would be more consistent. I don't have any strong preference.

@Kukovec Kukovec merged commit 1406d59 into main May 7, 2024
2 checks passed
@Kukovec Kukovec deleted the jk/itf branch May 7, 2024 16:59
@konnov konnov added this to the M1: Transaction extractor milestone May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write ITF serialization
3 participants