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

add a from method to ContractClient to instantiate from a contractId #960

Merged
merged 20 commits into from
May 14, 2024

Conversation

BlaineHeffron
Copy link
Contributor

Adds a from method which uses the contractId to create an instance of the ContractClient by retrieving the wasm from the blockchain and extracting its ContractSpec. I also add a fromWasm method which bypasses the wasm retrieval portion if you already have the wasm buffer.

@BlaineHeffron BlaineHeffron marked this pull request as draft May 8, 2024 19:27
src/contract_client/client.ts Outdated Show resolved Hide resolved
src/contract_client/client.ts Outdated Show resolved Hide resolved
src/contract_client/client.ts Outdated Show resolved Hide resolved
src/contract_client/utils.ts Outdated Show resolved Hide resolved
src/contract_client/utils.ts Outdated Show resolved Hide resolved
src/contract_client/utils.ts Outdated Show resolved Hide resolved
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.

👏

test/e2e/initialize.sh Show resolved Hide resolved
src/soroban/server.ts Outdated Show resolved Hide resolved
test/e2e/src/util.js Outdated Show resolved Hide resolved
test/e2e/src/util.js Outdated Show resolved Hide resolved
test/e2e/src/util.js Outdated Show resolved Hide resolved
@chadoh
Copy link
Contributor

chadoh commented May 9, 2024

Still marked as draft. Is that intended, @BlaineHeffron? Seems like it's about ready to go!

@BlaineHeffron
Copy link
Contributor Author

BlaineHeffron commented May 9, 2024

Still marked as draft. Is that intended, @BlaineHeffron? Seems like it's about ready to go!

See my comment here - #960 (comment)

To summarize, I think it makes sense since we are giving a way to create it from a wasm we should also give a way to create it from a wasmHash. This would also make sense in the context of our e2e tests.

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.

The RPC work looks good to me, I'll wait for approval from @chadoh / @willemneal on the actual client-side work as I see there's open discussion there before merging!

@BlaineHeffron BlaineHeffron marked this pull request as ready for review May 9, 2024 16:47
@BlaineHeffron
Copy link
Contributor Author

Should be all ready to go now.

src/soroban/server.ts Outdated Show resolved Hide resolved
@BlaineHeffron
Copy link
Contributor Author

Latest commit fixes all of Chad's comments.

@chadoh
Copy link
Contributor

chadoh commented May 9, 2024

Tests are failing

@BlaineHeffron
Copy link
Contributor Author

Tests are failing

Looks like that weird spawnSync issue we were seeing before. I added a fallback in case it doesnt work the first time.

Copy link
Contributor

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

Thanks for all the work here! Looks great

Copy link
Contributor

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

I think my last approval was for a single commit?

@chadoh
Copy link
Contributor

chadoh commented May 10, 2024

ok, I guess I just don't have the correct permissions to get this thing marked as mergeable

@BlaineHeffron BlaineHeffron requested a review from Shaptic May 10, 2024 18:11
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.

another pass 👀

also be sure to run yarn fmt

src/contract_client/client.ts Outdated Show resolved Hide resolved
src/contract_client/utils.ts Outdated Show resolved Hide resolved
src/contract_client/utils.ts Show resolved Hide resolved
src/contract_client/utils.ts Outdated Show resolved Hide resolved
src/contract_client/utils.ts Outdated Show resolved Hide resolved
src/soroban/server.ts Outdated Show resolved Hide resolved
src/soroban/server.ts Show resolved Hide resolved
src/soroban/server.ts Outdated Show resolved Hide resolved
test/e2e/initialize.sh Show resolved Hide resolved
@BlaineHeffron
Copy link
Contributor Author

I addressed comments from @Shaptic although this now is dependent on stellar/js-stellar-base#744 being merged first. The latest changes remove the external js-xdr dependency and uses the ceareal namespace from stellar-base for XdrReader.

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.

❤️

Copy link

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

Package New capabilities Transitives Size Publisher
npm/@stellar/[email protected] None +3 711 kB

🚮 Removed packages: npm/@stellar/[email protected]

View full report↗︎

@Shaptic Shaptic mentioned this pull request May 14, 2024
4 tasks
@Shaptic Shaptic merged commit b5729cc into stellar:master May 14, 2024
10 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.

4 participants