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

feat:ContractSpec class to parse ScVals #115

Merged
merged 16 commits into from
Aug 17, 2023
Merged

Conversation

willemneal
Copy link
Member

@willemneal willemneal commented Jul 27, 2023

This adds a new ContractSpec class which is comprised of the spec entries of a contract. This allows the internal user defined types to be accessible making the parsing to and from XDR more accurate. This should be moved to the js-stellar-base, but for now it is best served here for the type support.

This should replace the current parsing logic in the TS bindings currently living in the soroban-tools repo.

Also discussion here: https://gist.github.com/tyvdh/9391482f7490b085b283ea96d2f50b11

Copy link
Contributor

@paulbellamy paulbellamy left a comment

Choose a reason for hiding this comment

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

A couple small suggestions, but looks awesome.

test/unit/strval_test.js Outdated Show resolved Hide resolved
src/strval.ts Outdated Show resolved Hide resolved
@willemneal willemneal force-pushed the feat/strval.ts branch 3 times, most recently from 5a5adc1 to 5fee3cf Compare August 7, 2023 20:11
src/contract_spec.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/contract_spec.ts Outdated Show resolved Hide resolved
src/contract_spec.ts Outdated Show resolved Hide resolved
This adds a new ContractSpec class which is comprised of the spec entries of a contract. This allows the internal user defined types to be accessible making the parsing to and from XDR more accurate. This should be moved to the js-stellar-base, but for now it is best served here for the type support.
@socket-security
Copy link

socket-security bot commented Aug 11, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@types/mocha 10.0.1 None +0 96.3 kB types
stellar-base 10.0.0-soroban.7 None +4 8.49 MB stellar-npm-ci

@willemneal willemneal marked this pull request as ready for review August 15, 2023 17:43
@willemneal willemneal changed the title feat: add strval and ContractSpec class to parse ScVals feat:ContractSpec class to parse ScVals Aug 15, 2023
src/contract_spec.ts Outdated Show resolved Hide resolved
src/contract_spec.ts Outdated Show resolved Hide resolved
src/contract_spec.ts Outdated Show resolved Hide resolved
src/contract_spec.ts Outdated Show resolved Hide resolved
@willemneal
Copy link
Member Author

@Shaptic @paulbellamy I removed having the optional typeDef and it simplified everything. I was also able to switch all string comparisons to value comparisons which should improve performance (though I'm sure that's not the bottleneck currently for devs).

@willemneal
Copy link
Member Author

@paulbellamy I went with still having the base64 as it was easiest to copy over from the soroban-cli tests we added for the bindgen. For now I can regenerate the json file manually, but I'll add a script to do it soon.

test/unit/contract_spec.ts Outdated Show resolved Hide resolved
test/unit/contract_spec.ts Outdated Show resolved Hide resolved
test/unit/contract_spec.ts Show resolved Hide resolved
test/unit/contract_spec.ts Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@willemneal
Copy link
Member Author

@Shaptic Updated after your comments. Definitely could test more errors, but I think the tests are good enough for now

Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

Really really awesome work! Excited to get this across the finish line.

Last question: this uses futurenet-compatible XDR, right? I think yes based on the package.json but I want to confirm.

@willemneal
Copy link
Member Author

Yep! That is why I ported the CLI's tests! But using this to do the parsing of the generated bindings I was able to get a roundtrip with futurenet (we don't have the tests in CI in that repo yet because we don't want to depend on CI).

@Shaptic
Copy link
Contributor

Shaptic commented Aug 17, 2023

Love it! So are we ready to merge, then? It'd be awesome if you could add a brief changelog entry that matches the format of the others, then I can hit the button :shipit:

@Shaptic Shaptic merged commit 2bd3b09 into stellar:main Aug 17, 2023
4 checks passed
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.

3 participants