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

feat: pallet_unified_accounts implementation #1019

Merged
merged 30 commits into from
Sep 27, 2023

Conversation

ashutoshvarma
Copy link
Member

@ashutoshvarma ashutoshvarma commented Aug 30, 2023

Overview

Initial implementation for Account Unification pallet - pallet_unified_accounts. The Accounts module provide functionality for native account holders to connect their EVM accounts to have a unified experience across the different VMs. Inspired from Acala's evm-accounts.

  • Create double mapping between EVM and Native address by providing a EVM signed EIP721 payload.
  • Create double mapping between SS58 and a default derived EVM address.

This PR also introduce a small script that can be used as reference for claim accounts using ethers.js - claim.mjs

Assets Handling

In the scenario when EVM address already holds either native balance or XC20 assets then those funds needs to be transferred to the new SS58 account that is being connected to it.

Native Balance

The pallet will handle the transfer of native balance to new connected account since it is not very high maintenance (one dispatch call) and will in-fact improve the overall UX.
If we don't do this on pallet level then, when user connect the accounts, they first need to sign batch TX to transfer the XC20s and then sign another TX to for moving native balance before finally connecting the account.
The last tx can be skipped with this

XC20 (pallet-assets)

The XC20 (pallet-assets) will be handled by UI purely since it'll unnecessarily increase the complexity in runtime code.
Mainly because it add complex logic to runtime that we would have to maintain for the same things that UI can also handle.
We don't have a easy way to transfer all of user funds from pallet-assets. (no good way to enumerate over user holdings in pallet-assets, no TransferAll from orml)

EVM DApp Staking

If EVM address participated in DApp Staking via use of precompiles then he/she needs to claim and unstake everything since underlying SS58 would be changed.
This would also be handled purely on UI side as it'll also add complex logic to runtime which can be very well handled by UI.

TODO

  • Research if we can allow users to change mapping (if no security concern)
    • We don't for now, need more research whether this'll be safe or not (followup task?)
  • Add pallet rustdoc
  • Write unit tests (mapping, lookup, xvm, etc)
  • Update XVM Integration tests
  • Write RPC tests (follow-up task after integration in Shiden & Astar)
  • Benchmarks
    • run pallet benchmarks after review
    • re-run pallet-balances benchmark

@ashutoshvarma ashutoshvarma added the runtime This PR/Issue is related to the topic “runtime”. label Aug 30, 2023
@ashutoshvarma ashutoshvarma marked this pull request as ready for review September 13, 2023 15:56
Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed PR and the summary!

One suggestion for the summary is that you don't need to duplicate architecture information if it's also present in the pallet's rustdoc. Better focus on higher level info related to the PR.

Also, try to align naming with what the purpose of struct/trait/function is. It helps a lot when reading, reviewing and maintaining code later on.

pallets/account/src/lib.rs Outdated Show resolved Hide resolved
pallets/account/src/lib.rs Outdated Show resolved Hide resolved
pallets/account/src/lib.rs Outdated Show resolved Hide resolved
pallets/account/src/lib.rs Outdated Show resolved Hide resolved
pallets/account/src/lib.rs Outdated Show resolved Hide resolved
pallets/account/src/impls.rs Outdated Show resolved Hide resolved
pallets/account/src/tests.rs Outdated Show resolved Hide resolved
pallets/account/src/tests.rs Outdated Show resolved Hide resolved
tests/integration/src/account.rs Outdated Show resolved Hide resolved
tests/integration/src/setup.rs Outdated Show resolved Hide resolved
pallets/account/src/mock.rs Outdated Show resolved Hide resolved
pallets/account/Cargo.toml Outdated Show resolved Hide resolved
@HyunggyuJang
Copy link

Will follow-up PR enable to call evm pallet methods using substrate account? By modifying CallOrigin of pallet_evm config?

pallets/unified-accounts/src/impls.rs Outdated Show resolved Hide resolved
pallets/unified-accounts/src/impls.rs Outdated Show resolved Hide resolved
pallets/unified-accounts/src/impls.rs Outdated Show resolved Hide resolved
pallets/unified-accounts/src/impls.rs Outdated Show resolved Hide resolved
pallets/unified-accounts/src/impls.rs Outdated Show resolved Hide resolved
pallets/unified-accounts/src/lib.rs Outdated Show resolved Hide resolved
pallets/unified-accounts/src/lib.rs Outdated Show resolved Hide resolved
pallets/unified-accounts/src/lib.rs Outdated Show resolved Hide resolved
tests/integration/src/setup.rs Show resolved Hide resolved
tests/integration/src/xvm.rs Outdated Show resolved Hide resolved
@ashutoshvarma
Copy link
Member Author

/bench shibuya-dev pallet_unified_accounts

@github-actions
Copy link

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/6313707837.
Please wait for a while.
Branch: feat/account-unifification
SHA: 2609aec

Dinonard
Dinonard previously approved these changes Sep 26, 2023
Copy link
Member

@Dinonard Dinonard left a comment

Choose a reason for hiding this comment

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

Just 2 minor comments, otherwise LGTM.
I only checked the delta since the last review.

Good test coverage! 👍

pallets/unified-accounts/src/lib.rs Outdated Show resolved Hide resolved
pallets/unified-accounts/src/lib.rs Show resolved Hide resolved
@github-actions
Copy link

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/6313707837.

@ashutoshvarma
Copy link
Member Author

/bench shibuya-dev pallet_balances

@ashutoshvarma ashutoshvarma changed the title feat: pallet_account implementation feat: pallet_unified_accounts implementation Sep 26, 2023
@github-actions
Copy link

Benchmarks job is scheduled at https://github.com/AstarNetwork/Astar/actions/runs/6314586382.
Please wait for a while.
Branch: feat/account-unifification
SHA: 9fcc5a6

Dinonard
Dinonard previously approved these changes Sep 26, 2023
@github-actions
Copy link

Benchmarks have been finished.
You can download artifacts if exists https://github.com/AstarNetwork/Astar/actions/runs/6314586382.

@github-actions
Copy link

Code Coverage

Package Line Rate Branch Rate Health
chain-extensions/dapps-staking/src 0% 0%
precompiles/substrate-ecdsa/src 78% 0%
chain-extensions/types/assets/src 0% 0%
chain-extensions/xvm/src 0% 0%
precompiles/batch/src 80% 0%
precompiles/dapps-staking/src 93% 0%
pallets/unified-accounts/src 80% 0%
precompiles/sr25519/src 79% 0%
primitives/src/xcm 66% 0%
pallets/xvm/src 40% 0%
pallets/block-reward/src 85% 0%
precompiles/utils/src/testing 62% 0%
pallets/pallet-xcm/src 53% 0%
chain-extensions/types/dapps-staking/src 0% 0%
pallets/collator-selection/src 69% 0%
pallets/contracts-migration/src 0% 0%
precompiles/assets-erc20/src 76% 0%
precompiles/utils/src 68% 0%
chain-extensions/types/xvm/src 0% 0%
primitives/src 66% 0%
precompiles/xvm/src 75% 0%
pallets/dapps-staking/src/pallet 85% 0%
pallets/ethereum-checked/src 48% 0%
precompiles/xcm/src 84% 0%
precompiles/utils/macro/src 0% 0%
pallets/dapps-staking/src 81% 0%
pallets/xc-asset-config/src 53% 0%
chain-extensions/pallet-assets/src 0% 0%
Summary 58% (2618 / 4536) 0% (0 / 0)

Minimum allowed line rate is 50%

@ashutoshvarma ashutoshvarma merged commit 51a1b84 into master Sep 27, 2023
9 checks passed
@ashutoshvarma ashutoshvarma deleted the feat/account-unifification branch September 27, 2023 02:50
@ashutoshvarma ashutoshvarma added the shibuya related to shibuya label Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime This PR/Issue is related to the topic “runtime”. shibuya related to shibuya
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants