Implement CAP-58 (constructor support) #1447
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dmkozh
added
the
skip-env-interface-version-bump
|
If a pre-v22 contract gets its wasm updated in v22+ protocol with |
Nevermind, I see this mentioned in the CAP, |
dmkozh
force-pushed
the
ctor
branch
2 times, most recently
from
2b4a5b0 to
25551c6
Compare
The implementation itself is fairly simple (thanks to not needing to support v21 semantics) and follows the CAP. It only has a few caveats: - Pre-v22 protocol Wasms are treated as if they don't have a constructor (they shouldn't have it anyway, but just in case they do we ignore it in order to not break the contract semantics). Thus we don't need to even try calling constructor for them - We try to call constructor for all v22+ contracts, so we charge for VM instantiation, which increases the creation cost to some degree. I'm not sure if we can discount calling non-existent functions; if we find that necessary, we can still improve this before the protocol release. Bulk of this change is tests, and testing is really not trivial as we want to cover a set product of several different options: - p21 vs p22 contracts - v1 vs v2 host function - Constructor return values and errors - constructor arguments present/not present/'default' constructor - auth for constructor itself and inside the constructor - deployer support - invoker auth support - custom account support To make things worse, a contract may only have a single constructor, so this needed a lot of new test Wasms. I'm sure I haven't covered every possible combination, but at least tried to have some basic coverage for most of the cases and most obvious combinations.
The diffs are quite significant, but generally seem to fall into the following categories: - Changed auth data structure hashes (as we use the new XDR structure for the host fn) - Slightly increased instructions and memory for the CreateContract host fn and other XDRs that include it (+24 bytes) - New calls to the 'default' constructor for applicable contracts (typically generated Wasms that target the current protocol)
sisuresh
reviewed
Aug 22, 2024
After thinking for a bit, it doesn't seem like this could cause any security or logic issues, so we can improve backwards compatibility without much added maintenance cost.
sisuresh
reviewed
Aug 26, 2024
sisuresh
approved these changes
Aug 26, 2024
sisuresh
added a commit
to sisuresh/rs-soroban-env
that referenced
this pull request
Aug 30, 2024
This reverts commit 5ed156b.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
The implementation itself is fairly simple (thanks to not needing to support v21 semantics) and follows the CAP-58. It only has a few caveats:
Bulk of this change is tests, and testing is really not trivial as we want to cover a set product of several different options:
To make things worse, a contract may only have a single constructor, so this needed a lot of new test Wasms.
Why
See CAP-58 motivation.
Known limitations
I'm sure I haven't covered every possible combination of parameters in tests, but at least tried to have some basic coverage for most of the cases and most obvious combinations.