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 initial version of pallet_revive #5293

Merged
merged 21 commits into from
Aug 23, 2024
Merged

Add initial version of pallet_revive #5293

merged 21 commits into from
Aug 23, 2024

Conversation

athei
Copy link
Member

@athei athei commented Aug 8, 2024

This is a heavily modified and stripped down version of pallet_contracts. We decided to fork instead of extend the old pallet. Reasons for that are:

  • There is no benefit of supporting both on the same pallet as the intended payload for the new pallet (recompiled YUL) will be using a different ABI.
  • It is much easier since it allows us to remove all the code that was necessary to support Wasm and focus fully on running cross compiled YUL contracts.

The code is reviewable but can't be merged because it depends on an unreleased version of PolkaVM via git.

Current state

All tests are passing and the code is not quick and dirty but written to last. The work is not finished, though. It is included in the kitchensink-runtime and a node can be built. However, we merge early in order to be able to start testing other components as early as possible.

Outstanding changes are tracked here and will be merged separately: #5308

Syscall Interface

The syscall interface is best explored by generating the docs of this crate and looking at the SyscallDoc trait. Arguments are passed in registers a0-a5 in the order they are listed. If there are more than 6 arguments (call, instantiate) a pointer to a packed struct of the arguments is expected as the only argument. I plan to create variants of those syscalls with less arguments specifically for YUL.

Functions are just referenced by their name as ASCII within the PolkaVM container. Rather than by a syscall number as it was the case in the last implementation.

Changes vs. pallet_contracts

The changes are too numerous to list them all here. This is an incomplete list:

  • Use PolkaVM instead of wasmi to execute contracts
  • Made Runtime generic over a new Memory trait as we can't map memory directly on PolkaVM anymore
  • No static verification on code upload. Everything is a determinstic runtime failure
  • Removed all migrations and reset the pallet version
  • Removed the nonce storage item and instead use the deployers account nonce to generate a unique trie
    • We now bump the deployers account nonce on contract instantiation to they are bumped even within a batch transaction
    • Removed the instantiation nonce host function: We should add a new instantiate variant as a replacement for thos
  • ContractInfoOf of uses the indentity hasher now
  • Remove the determinism feature: User of that feature should switch to soft floats
  • The unstable attribute has been replaced by a api_version attribute to declare at which version an API became available
    • leaving out that attribute makes the API effectively unstable
    • a new api_version field on the CodeInfo makes sure that old contracts can't access new APIs (necessary due to lack of static verification.
  • Added a behaviour_version field to CodeInfo that can used if we need to introduce breaking changes and keep the old behaviour for existing contracts
  • Unified storage vs. transient and fixed vs. variable sized keys all into one set of multiplexing host functions
  • Removed all contract observeable limits from the Config trait and instead hardcode them
  • Removed the Schedule
  • Removed all deprecated host functions
  • Simplify chain extension as preperation for making it a pre-compile

@athei athei requested a review from a team as a code owner August 8, 2024 17:24
@athei athei changed the title Add initial version of pallet_contracts Add initial version of pallet_revive Aug 8, 2024
@athei athei added the R0-silent Changes should not be mentioned in any release notes label Aug 8, 2024
@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/contracts-on-assethub-roadmap/9513/1

@deuszx
Copy link

deuszx commented Aug 8, 2024

Will YUL contracts be able to call (Wasm) ink! ones (and vice versa)?

@athei
Copy link
Member Author

athei commented Aug 9, 2024

Will YUL contracts be able to call (Wasm) ink! ones (and vice versa)?

Building cross calling capabilities between a pallet_contracts and pallet_revive which exist on the same chain is not too hard. However, it will not be super useful since ink! uses a different ABI than what a YUL contract is using. With ABI I mean how contract calls encode arguments and return values. So it would need changes to existing ink! contracts to be aware of the different ABI.

But if you are making changes you can deploy the modified ink! contract to pallet_revive directly. In my opinion we should change ink! to also use Ethereum ABI. This means that not every type can be passed between contracts but this is fine. Better than to fragment the ecosystem.

The best way forward is to say: Contracts on pallet_revive use Eth ABI. Of course we can't enforce that. But everything that wants to talk to Solidity (YUL cross compiled) contracts need to do this.

So I expect the next major ink! version to:

  1. Support RISC-V/pallet_revive
  2. Change it's ABI to Eth ABI

This will make sure that we can all play together nicely. Of course ink! could keep its native ABI as an option for ink! to ink! calls. I am unsure if it is worth the effort though.

@peterwht
Copy link

peterwht commented Aug 9, 2024

In the forum post, you mention that pallet-revive will use AccountId20. Will the pallet continue to support AccountId32 as well for ink! contracts? Or, will ink! contracts need to update their environment to use AccountId20 when deploying to pallet-revive

@athei
Copy link
Member Author

athei commented Aug 9, 2024

The latter. For simplicity reasons we will fix all types to be whatever EVM expects. There will be no environment configuration anymore and contracts will just work on any chain without knowing which types the runtime uses. Conversion from and to the native type will be done by the runtime.

The most important change is that addresses are AccountId20 as you pointed out. But other things like block number will also be fixed to u256 instead of being configurable. ink! can (and should probably) still use smaller integers internally. It is just the syscall interface where we require wider integers. ink! will just sign extend for arguments and truncate + bound for return values.

As for the address type: ink! should just use AccoundId20 and thats it. It is an easy change since it is configurable anyways.

Those changes are not implemented, yet. The current code still uses native types.

@athei
Copy link
Member Author

athei commented Aug 9, 2024

bot fmt

@command-bot
Copy link

command-bot bot commented Aug 9, 2024

@athei https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6957347 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 28-b14f3ff2-1940-49ba-8c9d-d59f0d7c7c46 to cancel this command or bot cancel to cancel all commands in this pull request.

Copy link

github-actions bot commented Aug 9, 2024

We are migrating the command bot to be a GitHub Action

Please, see the documentation on how to use it

@command-bot
Copy link

command-bot bot commented Aug 9, 2024

@athei Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6957347 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6957347/artifacts/download.

@peterwht
Copy link

peterwht commented Aug 9, 2024

As for the address type: ink! should just use AccoundId20 and thats it. It is an easy change since it is configurable anyways.

Thanks for the answer.

So, is signing via Metamask / EVM compatible wallets necessary to interact with an ink! contract deployed on revive? Or will it map from a Polkadot-wallet to an AccountId20. I.e., a Polkadot-wallet internally controls an EVM-address?

@athei
Copy link
Member Author

athei commented Aug 9, 2024

We will have mappings. It is also possible to call a contract with an AccountId32 just by using a normal extrinsic instead of using the RPC proxy. However, Dapps be written against something like ether.js will only work with an Eth wallet like Metamask for now. That said, existing ink! dapps written against polkadot.js will continue to be viable due to the mapping described above.

The main problem is to how use AccountId32 with an app written against ether.js. It is possible but it will need some wallet support.

@peterwht
Copy link

@athei just to clarify because you say these types will be "fixed". Will the account id type and the block number be configurable in the runtime? So, if a different parachain wanted to have pallet-revive with AccountId32 and a "regular" BlockNumber, could they easily configure it?

@athei
Copy link
Member Author

athei commented Aug 21, 2024

@athei just to clarify because you say these types will be "fixed". Will the account id type and the block number be configurable in the runtime? So, if a different parachain wanted to have pallet-revive with AccountId32 and a "regular" BlockNumber, could they easily configure it?

No it will not be configurable. The reason is that it makes it much easier for the contract to be portable between chains. From the point of the contract it is always the widest type (u256) at the syscall interface. The runtime then does the conversion and errors out in case of an overflow.

And I don't think any chain wants to configure the account id differently as this would make running Solidity contracts impossible. In my opinion the way forward is to have Solidity and ink! contracts be fully interoperable. Please keep in mind that existing chains that use AccountId32 will work with this new pallet.

@athei athei requested review from a team as code owners August 21, 2024 09:50
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 1/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7089891

@athei athei added T2-pallets This PR/Issue is related to a particular pallet. and removed R0-silent Changes should not be mentioned in any release notes labels Aug 21, 2024
@athei
Copy link
Member Author

athei commented Aug 21, 2024

bot fmt

@command-bot
Copy link

command-bot bot commented Aug 21, 2024

@athei https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7091630 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 4-ac41a69c-72d2-40be-a646-046a09a87a53 to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Aug 21, 2024

@athei Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7091630 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7091630/artifacts/download.

@athei athei requested a review from xermicus August 21, 2024 15:59
@athei athei enabled auto-merge August 22, 2024 06:35
@athei athei added this pull request to the merge queue Aug 23, 2024
Merged via the queue into master with commit 559fa1d Aug 23, 2024
190 of 191 checks passed
@athei athei deleted the at/pallet_revive branch August 23, 2024 11:07
ordian added a commit that referenced this pull request Aug 27, 2024
* master: (36 commits)
  Bump the ci_dependencies group across 1 directory with 2 updates (#5401)
  Remove deprecated calls in cumulus-parachain-system (#5439)
  Make the PR template a default for new PRs (#5462)
  Only log the propagating transactions when they are not empty (#5424)
  [CI] Fix SemVer check base commit (#5361)
  Sync status refactoring (#5450)
  Add build options to the srtool build step (#4956)
  `MaybeConsideration` extension trait for `Consideration` (#5384)
  Skip slot before creating inherent data providers during major sync (#5344)
  Add symlinks for code of conduct and contribution guidelines (#5447)
  pallet-collator-selection: correctly register weight in `new_session` (#5430)
  Derive `Clone` on `EncodableOpaqueLeaf` (#5442)
  Moving `Find FAIL-CI` check to GHA (#5377)
  Remove panic, as proof is invalid. (#5427)
  Reactive syncing metrics (#5410)
  [bridges] Prune messages from confirmation tx body, not from the on_idle (#5006)
  Change the chain to Rococo in the parachain template Zombienet config (#5279)
  Improve the appearance of crates on `crates.io` (#5243)
  Add initial version of `pallet_revive` (#5293)
  Update OpenZeppelin template documentation (#5398)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

10 participants