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 external account provider #1329

Conversation

dmitrylavrenov
Copy link
Contributor

@dmitrylavrenov dmitrylavrenov commented Mar 5, 2024

The goal of this PR is to expose account related logic that pallet_evm required to control accounts existence in the network and their transactions uniqueness. By default, the pallet operates native system accounts records that frame_system provides.

The interface allow any custom account provider logic to be used instead of just using frame_system account provider. The accounts records should store nonce value for each account at least.

Related to #1331

cc @MOZGIII

@dmitrylavrenov dmitrylavrenov marked this pull request as ready for review March 5, 2024 16:47
@MOZGIII
Copy link
Contributor

MOZGIII commented Mar 15, 2024

Some context: this implementation has been used by the Humanode network for a while now, and we got tired of maintaining these patches in our own fork. It is our hope to merge this in the upstream such that:

  1. Our contribution is shared with the broader community, we believe this would be useful as an option to apply the EVM integration.
  2. We can avoid doing double work of applying upgrades and changes that frontier applies.
  3. We could get feedback from the upstream on our implementation of the approach we chose.

@boundless-forest
Copy link
Collaborator

Some context: this implementation has been used by the Humanode network for a while now, and we got tired of maintaining these patches in our own fork

Totally understand. I think it would be possible to accept the changes that abstract the operations related to the account into a pallet config(trait), but it makes no sense to include the pallet-evm-system implementation in your another pr in the frontier repo, as it will increase the maintenance burden, most of the downstream frontier users don't have that need. Exposing the trait and keeping the implementation on your side is a preferable way in my opinion. what do you think?

@MOZGIII
Copy link
Contributor

MOZGIII commented Mar 15, 2024

This would work for us. The reason why we are adding pallet-evm-system is that it is a very basic implementation of the independent accounts system that is required for this trait to make sense. We are worried that it will be difficult to maintain this trait without a buddy crate that would use it, at least with the mock tests.

So, the main rationale for merging both is to actually ensure the code doesn't break.

There are a lot of other things we'd suggest upstreaming, like a custom balances implementation that can work with the pallet-evm-system setting - that one, in our opinion, is something that we'd probably be able to leave out in our fork still.

@dmitrylavrenov
Copy link
Contributor Author

dmitrylavrenov commented Aug 20, 2024

@boundless-forest @crystalin @koushiro any updates ?

Will be ready to rebase after getting your feedback as I would like to merge it.

@boundless-forest
Copy link
Collaborator

Will be ready to rebase after getting your feedback as I would like to merge it.

I reviewed your solution regarding the pallet-evm-system and pallet-evm-balances. In my understanding, the chain has two account systems (currently, there is only one substrate account system in essence). The questions are as follows:

  1. How can tokens be exchanged between these two account systems? For example, if I have an AccountId32 A in the native account and another H160 account in the EvmSystem or EvmBalance, I assume there would be an additional bridge to facilitate transfers or withdrawals between these two accounts, correct? If so, I see little difference compared to the current mapping method. The bridge mentioned above is equivalent to mapping and then transferring.
  2. For the existing running chains with AccountId32, the H160 account information is stored in the native modules, which may require migration to your designed system and balance pallets.
  3. This solution will disrupt the dispatch precompile, which is used in several projects I know of. Since there is no mapping from H160 to AccountId32, it requires a valid origin for the substrate call underneath.

As for the chains with H160 account systems as the default, since they do not require account mapping, your solution does not change the way they operate.

The Frontier repo lacks reviewers and maintainers. I'm also quite frustrated about this. We need more people involved in the issue and PR discussions.

@MOZGIII
Copy link
Contributor

MOZGIII commented Aug 21, 2024

  1. How can tokens be exchanged between these two account systems? For example, if I have an AccountId32 A in the native account and another H160 account in the EvmSystem or EvmBalance, I assume there would be an additional bridge to facilitate transfers or withdrawals between these two accounts, correct? If so, I see little difference compared to the current mapping method. The bridge mentioned above is equivalent to mapping and then transferring.

There is a huge difference in that with separate account systems there is no trying to retrofit AccountId20 into AccountId32, and also no possibility of the confusing interactions between other pallets and EVM - EVM accounts are completely isolated in their own system, with only explicitly implemented pathways to interact with the native system.

  1. For the existing running chains with AccountId32, the H160 account information is stored in the native modules, which may require migration to your designed system and balance pallets.

This is not intended to be deployed to either preexisting chains - neither for the ones using the mixed accounts in the system table with the EVM address mappings, not for the ones using the EVM-native AccountId20 as AccountId.

This is for the new chains, and for Humanode which is running this setup in mainnet for a couple years now.

  1. This solution will disrupt the dispatch precompile, which is used in several projects I know of. Since there is no mapping from H160 to AccountId32, it requires a valid origin for the substrate call underneath.

This solution does not replace the account mappings though - they can still be used; we wanted it explicitly because we dislike the mappings system and the implications of mixing those with respect to the RPCs and our (Huamnode) chain's sybil-attack mechanisms.

As for the chains with H160 account systems as the default, since they do not require account mapping, your solution does not change the way they operate.

Yep. For the mixed accounts too - this adds an entirely new way to set things up.


You can inspect how this works in practice at Humanode chain:

Upstreaming this is really important to us, as we plan to further expand our investment into Frontier codebase and having too many custom patches is starting to slow us down. For instance, we'd be interested in working on proper EVM tracing support - but currently we're stalled by this upstreaming.

The Frontier repo lacks reviewers and maintainers. I'm also quite frustrated about this. We need more people involved in the issue and PR discussions.

We could aid acting act in this capacity at some point in the future as our involvement in Frontier and resource capacity grows.

@boundless-forest
Copy link
Collaborator

boundless-forest commented Aug 23, 2024

This is for the new chains, and for Humanode which is running this setup in mainnet for a couple years now.

Considering that many chains have been setup based on the current path and have been running in production for several years. I suggest we merge these traits so your team doesn't need to maintain and pick during the frontier upgrade process. For the pallet-evm-system and pallet-evm-balances, you can place them into your node runtime repo to reduce your maintenance burden. Is this approach acceptable for your team?

I suggest submitting an issue to gather feedback from the community members regarding your team's solution the evm-system and evm-balances. If many believe this is a preferable, we can consider including it in this repo.

@dmitrylavrenov
Copy link
Contributor Author

dmitrylavrenov commented Sep 10, 2024

@boundless-forest @MOZGIII Just merged master including conflict resolution. Looks like we've agreed on merging account abstraction.

For the pallet-evm-system and pallet-evm-balances we can create an issue and discuss separately to avoid blocking this PR. What do you think?

@dmitrylavrenov dmitrylavrenov force-pushed the introduce-external-account-provider branch from 157661e to 6c97cbc Compare September 10, 2024 09:07
frame/evm/src/lib.rs Outdated Show resolved Hide resolved
primitives/evm/src/account_provider.rs Show resolved Hide resolved
@boundless-forest boundless-forest merged commit d21ddc2 into polkadot-evm:master Sep 11, 2024
4 checks passed
dnjscksdn98 added a commit to bifrost-platform/bifrost-frontier that referenced this pull request Sep 23, 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.

3 participants