The account abstraction pallet #130

akru · 2023-02-07T06:01:38Z

Pull Request Summary

Account abstraction pallet (pallet-account) makes possible to manage multiple origins and proxy call using desired account origin.

Check list

added unit tests
updated documentation

…allet-account

shunsukew · 2023-02-28T10:24:31Z

~~Can I ask what are purposes and use cases of call_as call to dispatch by using derived account? I'm interested in it.~~

* added unit tests * added meta transactions

shunsukew · 2023-03-28T06:41:20Z

frame/pallet-account/src/pallet/mod.rs

+            let who = ensure_signed(origin)?;
+
+            let magic = T::ChainMagic::get();
+            let nonce = frame_system::Pallet::<T>::account_nonce(signer.clone());


Signer usually doesn't have enough balance, and sometimes doesn't have it at all.
How do we hold nonce of accounts without Existential Deposit (ED)?

And because of ED, account info can be cleaned up, which means account nonce can be reset to 0, so I think it cannot ensure replay attack when that account is recreated later.

Being discussed here https://github.com/paritytech/substrate/issues/11988

Agree with Shunsuke, meta_call should be left for some other PR in the future, potentially.

Temporary removed

frame/pallet-account/src/lib.rs

frame/pallet-account/src/origins.rs

Dinonard · 2023-03-28T09:00:04Z

frame/pallet-account/src/origins.rs

+pub enum NativeAndEVMKind {
+    Native,
+    H160,
+}


Perhaps adding impl Into for enum NativeAndEVM that converts into NativeAndEVMKind is a good idea - you can test the conversion & ensure it's exhaustive.

Probably yes, but may be we can generate *Kind structure from custom origin somehow. I'll investigate later.

Dinonard · 2023-03-28T09:03:03Z

frame/pallet-account/src/pallet/mod.rs

+    #[pallet::pallet]
+    #[pallet::generate_store(pub(super) trait Store)]
+    #[pallet::storage_version(STORAGE_VERSION)]
+    #[pallet::without_storage_info]


This should be removed and pallet should correctly be bounded in relations to PoV size.

Rewritten to make it bounded.

Dinonard · 2023-03-28T09:04:06Z

frame/pallet-account/src/pallet/mod.rs

+    /// Account origins
+    #[pallet::storage]
+    pub type AccountOrigin<T: Config> =
+        StorageMap<_, Blake2_128Concat, T::AccountId, Vec<T::CustomOrigin>, ValueQuery>;


Vec<_> should not be used in storage. Instead, you should use BoundedVec. That way you can also get rid of the without_storage_info macro you used above. This item cannot be unlimited.

Used double-map instead of vector.

Dinonard · 2023-03-28T09:10:45Z

frame/pallet-account/src/pallet/mod.rs

+            origin: T::CustomOrigin,
+        },
+        ProxyCall {
+            payer: T::AccountId,


Why is payer needed here?

Dinonard · 2023-03-28T09:11:22Z

frame/pallet-account/src/pallet/mod.rs

+            let who = ensure_signed(origin)?;
+
+            let magic = T::ChainMagic::get();
+            let nonce = frame_system::Pallet::<T>::account_nonce(signer.clone());


Agree with Shunsuke, meta_call should be left for some other PR in the future, potentially.

Dinonard · 2023-03-28T09:16:49Z

frame/pallet-account/src/pallet/mod.rs

+            Self::deposit_event(Event::NewOrigin {
+                account: who,
+                origin: new_origin,
+            });


Just wondering - why do we even need to store the derived address?

If user would specify Origin kind & index, we could always derive the same address. Is there some security risk that comes with such approach?

Not sure whether it's more computationally expensive to derive the address or read the DB entry.

In case of store derived address we can charge origin creation fee, it looks more secure because it prevents address derivation manipulation using GPU, etc.

Dinonard · 2023-03-28T09:17:17Z

frame/pallet-account/Cargo.toml

+authors = ["Stake Technologies"]
+edition = "2021"


Please use workspace inheritance for relevant package info values.

Dinonard · 2023-03-28T09:18:04Z

frame/pallet-account/src/weights.rs

+// You should have received a copy of the GNU General Public License
+// along with Astar. If not, see <http://www.gnu.org/licenses/>.
+
+use frame_support::weights::Weight;


I don't see any TODO for this, but you should also add benchmarking code.

@akru this comment is unresolved.

Added TODO, I'll add weights in next PR.

Added benchmarks, next step generate weights

frame/pallet-account/src/lib.rs

frame/pallet-account/src/pallet/mod.rs

Co-authored-by: Dmitry <korvin@deeptown.org>

Co-authored-by: PierreOssun <35110271+PierreOssun@users.noreply.github.com>

Co-authored-by: Dmitry <korvin@deeptown.org>

akru · 2023-04-07T12:41:39Z

@Dinonard please check changes, I'll happy to merge it soon

Dinonard · 2023-04-10T07:34:23Z

frame/pallet-account/src/tests.rs

+            if origin == NativeAndEVM::Native(ALICE_D1_NATIVE.into())
+        );
+    })
+}


You should also add some negative tests to cover the possible errors.

Dinonard · 2023-04-10T07:34:32Z

frame/pallet-account/src/weights.rs

+// You should have received a copy of the GNU General Public License
+// along with Astar. If not, see <http://www.gnu.org/licenses/>.
+
+use frame_support::weights::Weight;


@akru this comment is unresolved.

Dinonard · 2023-04-10T07:36:36Z

frame/pallet-account/src/pallet/mod.rs

+            let new_origin = origin_kind.derive(&who, next_index as u32);
+
+            origins.push(new_origin.clone());
+            AccountOrigin::<T>::insert(who.clone(), origins);


@akru This comment is still relevant.

Dinonard · 2023-04-10T07:37:08Z

frame/pallet-account/src/pallet/mod.rs

+			(T::WeightInfo::proxy_call()
+				.saturating_add(T::DbWeight::get().reads_writes(1, 1))


@akru This comment is still relevant.

frame/pallet-account/src/lib.rs

shaunxw · 2023-05-18T11:29:05Z

frame/pallet-account/src/lib.rs

+                .saturating_add(T::DbWeight::get().reads_writes(1, 2))
+        )]
+        #[pallet::call_index(0)]
+        pub fn new_origin(


As the origin derivation algorithm is deterministic, could we skip the origin registering? The derived origin could be derived just in time in a proxy_call, like:

Account::proxy_call(alice, derive_kind, index, call);

If we do need to cache the derived origin to reduce gas costs, the cache should be cleaned on kill account.

I believe derivation algorithm is just a sample, it not restricted to be deterministic. Account registration helps to prevent spamming and account address manipulation.

@akru what is the usecase behind allowing single account to have multiple derived address of the same type?

The way I see it, it just adds unnecessary complexity.
If we only allow one account type to be derived from the origin, then it's much simpler.
If an account wants to control multiple different derived accounts, it can use proxy to achieve that.

Shaun also commented above(here) about what happens when account is killed. Without any locked or reserved funds, these storage items will remain in the DB even without an owner.

shaunxw · 2023-05-18T11:35:49Z

frame/pallet-account/src/origins.rs

+    fn derive(&self, source: &AccountId32, index: u32) -> NativeAndEVM {
+        let salted_source = [source.as_ref(), &index.encode()[..]].concat();
+        let derived = sp_io::hashing::blake2_256(&salted_source);
+        match self {
+            NativeAndEVMKind::Native => NativeAndEVM::Native(derived.into()),
+            NativeAndEVMKind::H160 => NativeAndEVM::H160(sp_core::H160::from_slice(&derived[..20])),
+        }
+    }


Dumb question: is our custom origin deriving meant to derive between H160 and Native? The impl here seems only to derive sub-accounts from their own type.

Yes, to create new origin the call origin must be substrate-compatible.

shaunxw · 2023-05-18T11:38:25Z

frame/pallet-account/src/origins.rs

+
+impl OriginDeriving<AccountId32, NativeAndEVM> for NativeAndEVMKind {
+    fn derive(&self, source: &AccountId32, index: u32) -> NativeAndEVM {
+        let salted_source = [source.as_ref(), &index.encode()[..]].concat();


I think we can add module id as entropy, probably same to module id in pallet-utilities, to be compatible. https://github.com/paritytech/substrate/blob/f4a2e84ee5974b219f2a03cd195105060c41e3cd/frame/utility/src/lib.rs#LL506C28-L506C28

EDIT: probably not a good idea to be compatible with pallet-utilities. Compatibility with main stream derivation standard is more important

Honestly, at first stage I prefer to give some sample of usage and no more. What’s about derivation standard, it could be easily implemented outside of the pallet.

frame/pallet-account/src/tests.rs

Co-authored-by: Shaun Wang <spxwang@gmail.com>

github-actions · 2023-05-30T15:14:09Z

Package	Line Rate	Branch Rate	Health
precompiles/dapps-staking/src	93%	0%	✔
frame/collator-selection/src	76%	0%	➖
frame/custom-signatures/src	52%	0%	➖
frame/dapps-staking/src/pallet	87%	0%	✔
primitives/xcm/src	62%	0%	➖
frame/pallet-xvm/src	5%	0%	❌
chain-extensions/dapps-staking/src	0%	0%	❌
chain-extensions/types/dapps-staking/src	0%	0%	❌
frame/pallet-xvm/src/pallet	8%	0%	❌
precompiles/sr25519/src	79%	0%	➖
precompiles/assets-erc20/src	76%	0%	➖
precompiles/xcm/src	85%	0%	✔
frame/dapps-staking/src	82%	0%	✔
chain-extensions/types/assets/src	0%	0%	❌
frame/contracts-migration/src	0%	0%	❌
frame/xc-asset-config/src	50%	0%	➖
chain-extensions/types/xvm/src	0%	0%	❌
chain-extensions/xvm/src	0%	0%	❌
frame/pallet-account/src	73%	0%	➖
precompiles/substrate-ecdsa/src	78%	0%	➖
precompiles/utils/macro/src	0%	0%	❌
precompiles/utils/src	69%	0%	➖
precompiles/utils/macro/tests	0%	0%	❌
frame/block-reward/src	80%	0%	✔
precompiles/xvm/src	61%	0%	➖
frame/pallet-xcm/src	53%	0%	➖
chain-extensions/pallet-assets/src	0%	0%	❌
Summary	57% (2738 / 4831)	0% (0 / 0)	➖

Minimum allowed line rate is 50%

Dinonard · 2023-05-30T17:10:18Z

frame/pallet-account/src/lib.rs

+            AccountOrigin::<T>::insert(&who, next_index, new_origin.clone());
+            AccountLastOrigin::<T>::insert(&who, next_index + 1);


This is still free - so my comment from 2 months ago is still valid.
You should charge rent fee for these two items since you're inflating the database.

Check how pallet-assets is doing it, or any other pallet in substrate that creates some storage items (e.g. pallet-identity, pallet-multisig, etc.).

Technically, you pay transaction fee to cover it (prevent spam), it’s not required at all, just one of ways to do it. I’m not sure it’s required for the prototype.

My personal opinion - better charge fee than just lock token forever. Charged fee much more controllable, it could be burn, redistributed or whatever you want. But locked deposit is just locked.

The way it's implemented now - sure, that's viable option. It still needs to be added though since it's missing now.

Not sure something is missing now, what precisely you are talking about?

I'm talking about payment for the created storage.

Technically, you pay transaction fee to cover it (prevent spam), it’s not required at all, just one of ways to do it. I’m not sure it’s required for the prototype.

Given that our handling of fees is copy/paste of Polkadot, it is how we do things, isn't it?
Don't understand why this pallet should be any different.
Are you proposing we mix it up, some pallets charging for rent fee, while some don't? 🤷‍♂️

It's been on review for months, and better to do it right immediately then have someone else fix it later, add storage migration logic, etc. Doesn't matter if it's prototype or not, IMO.

akru · 2023-05-31T01:41:26Z

Thank you for your feedback, @shaunxw! If you have no more concerns, then let’s approve and merge this PR.

akru added 2 commits February 7, 2023 08:58

Init pallet-account

9856d9e

Merge remote-tracking branch 'origin/polkadot-v0.9.37' into feature/p…

8e049e6

…allet-account

akru changed the base branch from polkadot-v0.9.33 to polkadot-v0.9.37 February 20, 2023 08:29

Added simple salt implementation & mocks

a909f8e

akru added 3 commits March 7, 2023 07:44

Custom origin driven pallet-account

3619ac4

Merge remote-tracking branch 'origin/polkadot-v0.9.37'

2cec263

proxy call implementation

d810947

* added unit tests * added meta transactions

akru requested review from shunsukew and 0x7CFE March 28, 2023 05:15

akru marked this pull request as ready for review March 28, 2023 05:15

shunsukew reviewed Mar 28, 2023

View reviewed changes

Dinonard suggested changes Mar 28, 2023

View reviewed changes

0x7CFE reviewed Mar 28, 2023

View reviewed changes

frame/pallet-account/src/lib.rs Outdated Show resolved Hide resolved

frame/pallet-account/src/lib.rs Outdated Show resolved Hide resolved

frame/pallet-account/src/lib.rs Outdated Show resolved Hide resolved

frame/pallet-account/src/lib.rs Outdated Show resolved Hide resolved

PierreOssun reviewed Apr 4, 2023

View reviewed changes

frame/pallet-account/src/pallet/mod.rs Outdated Show resolved Hide resolved

akru and others added 8 commits April 6, 2023 17:10

Update frame/pallet-account/src/lib.rs

6e71c60

Co-authored-by: Dmitry <korvin@deeptown.org>

Update frame/pallet-account/src/pallet/mod.rs

c1a740e

Co-authored-by: PierreOssun <35110271+PierreOssun@users.noreply.github.com>

Update frame/pallet-account/src/lib.rs

6f5aa27

Co-authored-by: Dmitry <korvin@deeptown.org>

Update frame/pallet-account/src/lib.rs

ee7e727

Co-authored-by: Dmitry <korvin@deeptown.org>

Update frame/pallet-account/src/lib.rs

0973c5a

Co-authored-by: Dmitry <korvin@deeptown.org>

Temporary remove meta_call

98700fa

Rewrite origin storage to make it storage info compatible

f8149af

Use workspace authors

b0c4bb0

Dinonard reviewed Apr 10, 2023

View reviewed changes

akru added 3 commits April 20, 2023 08:22

Added TODO for weights

11ec601

Fix weights setting

eb56aa2

Fix tests

b494247

akru changed the base branch from polkadot-v0.9.37 to polkadot-v0.9.39 April 20, 2023 06:07

Added negative tests

0307dcd

akru added 5 commits April 20, 2023 15:45

Fix cargo fmt

34b2d24

Added benchmarking

c829786

Fix cargo fmt

564a2e9

Use runtime level compatible hashing

83fa23b

Merge branch 'polkadot-v0.9.39' into feature/pallet-account

ca3bddc

akru mentioned this pull request May 18, 2023

Enable pallet-account on shibuya AstarNetwork/Astar#913

Closed

5 tasks

shaunxw reviewed May 18, 2023

View reviewed changes

akru and others added 2 commits May 30, 2023 17:42

Update frame/pallet-account/src/lib.rs

3a1ae8b

Co-authored-by: Shaun Wang <spxwang@gmail.com>

Added event checks into tests

7aca6a1

Dinonard suggested changes May 30, 2023

View reviewed changes

Added OnKillAccount handler & creation deposit functionality

81f533d

		(T::WeightInfo::proxy_call()
		.saturating_add(T::DbWeight::get().reads_writes(1, 1))

		AccountOrigin::<T>::insert(&who, next_index, new_origin.clone());
		AccountLastOrigin::<T>::insert(&who, next_index + 1);

The account abstraction pallet #130

Are you sure you want to change the base?

The account abstraction pallet #130

Conversation

akru commented Feb 7, 2023 • edited

shunsukew commented Feb 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akru commented Apr 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaunxw May 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaunxw May 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akru commented May 31, 2023

akru commented Feb 7, 2023 •

edited

shunsukew commented Feb 28, 2023 •

edited

shaunxw May 18, 2023 •

edited

shaunxw May 18, 2023 •

edited