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

[CAP-46-02] Updated contract lifecycle CAP. #1315

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

dmkozh
Copy link
Contributor

@dmkozh dmkozh commented Nov 2, 2022

Note: This includes the configuration update commit that belongs to #1291, so the diff from CAP-46-09 commit should be ignored while reviewing this.

The main updates:

  • Change InvokeHostFunctionOp to use XDR for contract creation args, as it's getting quite complex and doesn't ever need to be called from the contracts.
  • Decouple WASM sources from the contract instances in order to deduplicate contracts that share their code.
  • Change the contract-from-contract creation to use the code reference instead of the actual code
  • Removed some outdated parts that don't allow well with the current implementation/vision

Some debatable decisions:

  • This CAP still allows fusing WASM installation with contract instantiation. This could in theory be replaced with 2 invoke host fn operations (install, then create), though building these operations could be a bit tricky as the user would need to compute the code hash themselves.
  • Contract initializers are not a part of the CAP. If we allow multiple host fn invocations, initializer call can be added to the list of operations (although the user would need to specify the contract ID, which could be a bit tricky). Unlike in the installation case, initialization is user-defined, so defining the initializer spec at the protocol level could be too rigid, especially considering auth.
  • The CAP doesn't allow a contract source to refer another contract. On one hand, it would be easy to implement. On another hand, this doesn't seem too useful without the ability to modify the contract source references.
  • I've removed the ability to deploy contracts from contracts with ID derived from ed25519 key for now. This is not because I necessarily think we should never have those, but because I'd like to make sure it would align with the auth patterns in case if we change them.

core/cap-0046.md Outdated
Comment on lines 560 to 577
+ union switch (ContractIDType type)
+ {
+ case CONTRACT_ID_FROM_PUBLIC_KEY:
+ struct
+ {
+ union switch (ContractIDPublicKeyType type)
+ {
+ case CONTRACT_ID_PUBLIC_KEY_SOURCE_ACCOUNT:
+ void;
+ case CONTRACT_ID_PUBLIC_KEY_ED25519:
+ struct
+ {
+ uint256 key;
+ Signature signature;
+ } ed25519KeyWithSignature;
+ } keySource;
+ uint256 salt;
+ } publicKey;
Copy link
Member

Choose a reason for hiding this comment

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

Fyi I have this lurking memory that some of the SDKs don't do well with many nested layers of anonymous types, and we might need to make each layer in this XDR type a named type. We can see how it plays out first, but definitely remember experiencing some pain with structures defined anonymously with more than 2 levels deep in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, reduced the nesting. I still left a couple anonymous structs, but as they're no longer nested this shouldn't hopefully be a problem.

The main updates:

- Change `InvokeHostFunctionOp` to use XDR for contract creation args, as it's getting quite complex and doesn't ever need to be called from the contracts.
- Decouple WASM sources from the contract instances in order to deduplicate contracts that share their code.
- Change the contract-from-contract creation to use the code reference instead of the actual code
- Removed some outdated parts that don't allow well with the current implementation/vision
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