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

Support for constructor-related functionality in Soroban SDK. #1327

Merged
merged 3 commits into from
Sep 16, 2024

Conversation

dmkozh
Copy link
Contributor

@dmkozh dmkozh commented Sep 6, 2024

What

The main non-test changes are updates for the auth-related data structures and a wrapper for the new deployer function (create_contract_with_constructor).

The remaining changes concern the test infrastructure support, specifically contract creation utilities that support passing constructor arguments.

Why

Supporting constructors introduced in protocol 22.

Known limitations

N/A

@dmkozh dmkozh force-pushed the ctor_bump branch 2 times, most recently from a7ac4f4 to 5c43fca Compare September 6, 2024 21:07
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Couple small asks otherwise lgtm.

soroban-sdk-macros/src/derive_fn.rs Outdated Show resolved Hide resolved
soroban-sdk/src/deploy.rs Outdated Show resolved Hide resolved
@dmkozh dmkozh force-pushed the ctor_bump branch 2 times, most recently from 5cdb384 to 6385e06 Compare September 10, 2024 23:13
@dmkozh dmkozh marked this pull request as ready for review September 10, 2024 23:33
leighmcculloch added a commit that referenced this pull request Sep 16, 2024
This fix is extracted from #1327 by @dmkozh.

Co-authored-by: Dmytro Kozhevin <dmytro@stellar.org>
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

Nice catching the testutils bug. I'd like to merge the testutils fix separately in a v21 release, and I've opened a PR to do that:

Other than that, I left a couple minor comments inline, and it LGTM.

Cargo.toml Show resolved Hide resolved
soroban-sdk-macros/src/derive_fn.rs Outdated Show resolved Hide resolved
soroban-sdk-macros/src/derive_fn.rs Outdated Show resolved Hide resolved
soroban-sdk/src/deploy.rs Outdated Show resolved Hide resolved
soroban-sdk/src/env.rs Outdated Show resolved Hide resolved
soroban-sdk/src/env.rs Outdated Show resolved Hide resolved
github-merge-queue bot pushed a commit that referenced this pull request Sep 16, 2024
### What

Panic if the number of args being passed to a natively registered
contract don't match the number of args the contract function expects.

### Why

This bug exists only in the testutils path of invoking a natively
registered contract and does not impact WASM registered contracts, or
the env as deployed.

If a native contract is invoked with too few arguments, a panic occurs
with an index out of bounds errors that is not intuitive to debug. The
panic displayed to the developer should be as intuitive as possible.
- Before: `index out of bounds: the len is 1 but the index is 1`
- After: `invalid number of input arguments: 2 expected, got 1`

If a native contract is invoked with too many arguments, no panic
occurs, the additional arguments are simply ignored. The behaviour of
invoking natively registered contracts in testutils should be as
consistent as possible to invoking contracts on a deployed network. In
both of these cases the invoke fails.
- Before: no panic
- After: `invalid number of input arguments: 2 expected, got 3`

Thanks to @dmkozh who fixed this bug and proposed the better error
experience in #1327 which is destined for the next major release of the
sdk v22, and this pull request extracts the fix in isolation so we can
release a bug fix release of the sdk v21.

---------

Co-authored-by: Dmytro Kozhevin <dmytro@stellar.org>
The main non-test changes are updates for the auth-related data structures and a wrapper for the new deployer function (create_contract_with_constructor).

The remaining changes concern the test infrastructure support, specifically contract creation utilities that support passing constructor arguments.
soroban-sdk/src/deploy.rs Outdated Show resolved Hide resolved
@dmkozh dmkozh added this pull request to the merge queue Sep 16, 2024
Merged via the queue into stellar:main with commit 6ede50a Sep 16, 2024
16 checks passed
@dmkozh dmkozh deleted the ctor_bump branch September 16, 2024 22:40
leighmcculloch added a commit that referenced this pull request Sep 17, 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.

2 participants