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 multi-config support #1503

Merged
merged 46 commits into from
Mar 21, 2024
Merged

Add multi-config support #1503

merged 46 commits into from
Mar 21, 2024

Conversation

smiasojed
Copy link
Collaborator

@smiasojed smiasojed commented Feb 15, 2024

Summary

Related #1168

  • y/n | Does it introduce breaking changes?
  • y/n | Is it dependent on the specific version of ink or pallet-contracts?
    Add multi chain support

Description

Add multi-chain config support. This solution includes an option --config for commands that depend on chain configuration. Polkadot serves as the default config, and if not provided, the default configuration is used.

For now, the chain configuration is selected using the --config option, but in the future, it may be discovered automatically.

Current limitations:
hash: [u8;32] - hardcoded in metadata crate
AccountId32 - transcoder trait implemented only for this type (crate transcoder)
Balance: From<u128>/Into<u128>

Checklist before requesting a review

  • [y] My code follows the style guidelines of this project
  • I have added an entry to CHANGELOG.md
  • [y] I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • [y] Any dependent changes have been merged and published in downstream modules

@smiasojed smiasojed marked this pull request as ready for review February 23, 2024 09:36
Copy link
Collaborator

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

Looking good, the main thing would be to attempt to reduce the duplication of the verbose trait bounds on all the run fns

crates/cargo-contract/src/cmd/call.rs Outdated Show resolved Hide resolved
crates/cargo-contract/src/main.rs Outdated Show resolved Hide resolved
/// A runtime configuration for the Polkadot based chain.
/// This thing is not meant to be instantiated; it is just a collection of types.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum Polkadot {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
pub enum Polkadot {}
pub enum Substrate {}

I'm leaning towards renaming it to Substrate, as that's more correct and includes also solochains like Aleph Zero.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In subxt there is another config called Substrate, which matches the default kitchen sink node extrinsic params. So in subxt Polkadot -> relay chain and node-template config (used by most parachains and substrate-contracts-node), and Substrate -> substrate kitchen sink node with different extrinsic params.

We can give these different names possibly here though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added Substrate config

@cmichi
Copy link
Collaborator

cmichi commented Feb 29, 2024

I'm getting an unexpected error when trying to instantiate flipper on substrate-contracts-node with Ecdsachain ‒ so a mismatching config:

$ cargo contract instantiate --suri //Alice --args true -x -y --verbose --config Ecdsachain
 Dry-running new (skip with --skip-dry-run)
                Result ModuleError: Contracts::StorageDepositNotEnoughFunds: ["Origin doesn't have enough balance to pay the required storage deposits."]
          Gas Consumed Weight { ref_time: 0, proof_size: 0 }
          Gas Required Weight { ref_time: 0, proof_size: 0 }
 Storage Total Deposit StorageDeposit::Charge(0)
ERROR: Pre-submission dry-run failed. Use --skip-dry-run to skip this step.

The origin has enough balance. Do you have an idea why this misleading error is output?

Ideally the @SkymanOne implementation of #1377 would catch this, but I guess it doesn't check the Signer yet. @SkymanOne Can you check?

@smiasojed
Copy link
Collaborator Author

smiasojed commented Feb 29, 2024

@cmichi I think that If you are using an ECDSA signer to generate keys and sign transactions, but the chain expects SR25519 keys, then the keys will differ. As a result, the addresses derived from these keys will also differ.
The @SkymanOne check will not catch it, because he is not checking node configs like used Signatures

@SkymanOne
Copy link
Contributor

Yep, Signature is not the type that is persisted in the Environment trait of pallet-contracts. There is still planned work to configure hash function and persist it in the Environment use-ink/ink#1820
we can do something similar for the Signature I guess.

@smiasojed
Copy link
Collaborator Author

smiasojed commented Mar 1, 2024

The subxt team is going to check if they can validate subxt::Config to see if it matches with node, before issuing request to it. I would suggest to wait for it before we decide to extend the pallet_contract API
paritytech/subxt#1461

@smiasojed smiasojed merged commit 04e42b9 into master Mar 21, 2024
11 checks passed
@smiasojed smiasojed deleted the sm/multichain branch March 21, 2024 14:16
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