AccountAbstraction that works per domain #15899

igor-aptos · 2025-02-06T06:10:26Z

Current AA requires registration for each account, and user has to have private key for an address to be able to do so.

This change allows for AA to be per domain - for example for Solana we need "ed25519_hex", and domain-separated addresses to automatically accept it.

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2025-02-06T06:10:29Z

⏱️ 27m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
check-dynamic-deps	11m	🟩 🟩 🟩 🟩 🟩
rust-cargo-deny	10m	🟩 🟩 🟩 🟩 🟩
general-lints	2m	🟩 🟩 🟩 🟩 🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩
file_change_determinator	54s	🟩 🟩 🟩 🟩 🟩
permission-check	18s	🟩 🟩 🟩 🟩 🟩
permission-check	11s	🟩 🟩 🟩 🟩 🟩

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

lightmark · 2025-02-06T10:24:52Z

aptos-move/framework/aptos-framework/sources/account/account_abstraction.move

@@ -45,6 +53,10 @@ module aptos_framework::account_abstraction {
        V1 { auth_functions: OrderedMap<FunctionInfo, bool> }
    }

+    enum DomainDispatchableAuthenticator has key {
+        V1 { auth_functions: BigOrderedMap<FunctionInfo, String> }


It's more intuitive to have domain -> function_info mapping.

Feels like the domain is the FunctionInfo itself and this isn't needed.

logically you are right. it's more like an UX question. If we do that, auth_data would carry this, so instead of evm, users would attach sth like 0x1::aa::evm_auth instead.

or we create a module only for this, e.g., 0x1::domain_auth. Then it has different auth functions as evm, solana, etc... Then user just use the domain name in the auth_data and we generate the function info according to this rule. It's a combination of static naming and dynamic dispatch.

I don't actually want this in the framework, I think the idea of domain separated AA addresses makes sense, but I think how those are computed should be independent. We likely want Alin's blessing.

independent of?
Who should be responsible for this auth function implementation? Since it is critical to many people who use this feature.

davidiw

I think we would want a view function that supports the following:

fun extract_authentication_address(aa_fi: FunctionInfo, user_data: vector<u8>): address
which internally calls
fun aa_authentication_address(aa_fi: FunctionInfo, metadata: vector<u8>): address
which effectively just does sha256(0xFX | aa_fi | metadata)

thus it would be up to the AA to suggest deterministic, verifiable metadata into the user_data... such as the Ethereum address.

davidiw · 2025-02-06T22:18:33Z

aptos-move/framework/aptos-framework/sources/account/account_abstraction.move

@@ -45,6 +53,10 @@ module aptos_framework::account_abstraction {
        V1 { auth_functions: OrderedMap<FunctionInfo, bool> }
    }

+    enum DomainDispatchableAuthenticator has key {
+        V1 { auth_functions: BigOrderedMap<FunctionInfo, String> }


Feels like the domain is the FunctionInfo itself and this isn't needed.

igor-aptos · 2025-02-07T07:20:32Z

I have to see how Solana, evm, etc sign things, and what would be in the signing data. Probably to have a test for both, to confirm they would work.

For domain name vs fully qualified function name - only reason to have it is if implementation is immutable and not in framework and needs security fix (so it needs to switch to a new function.

If we name them in a chain agnostic way - like "ed25519_hex" instead of Solana, then impl could go into the framework as a general signing scheme, I think.

davidiw · 2025-02-07T16:20:25Z

I think it would be better to not add other chain signature verifications directly to Aptos and instead leave that for implementations to figure out. If we see something that makes sense, we could definitely add it, but signature verification also implies we have clarity on what we're verifying. I think this is something that is better left to evolve a bit more before injecting into the framework.

At the same time, there is a real problem of deterministic name address lookup.

My proposal is that we introduce a means of converting the AA authenticators into distinct domains:

When we instantiate a new authenticator given an FI, we check address::module::function_address, if it exists, it is expected to be of the format function_address(aa_fi: FunctionInfo, user_data: AbstractionAuthData): vector<u8>
If it doesn't exist, then there is no deterministic address computation for AA accounts.
During authentication, we can see query something like aptos_framework::account_abstraction::derive_address(aa_fi: FunctionInfo, user_data: AbstractionAuthData): Option<address> -- which returns none if nothing is implemented or some if it is. We can also leverage the flag that Igor added to optimize away unnecessary lookups.

Then we move this problem out of adding new permissioned elements and leave it to user space code for the time being.

Note, derive_address calls function_address, then uses a new domain separated address computation, e.g., 5 -- not sure i understand how 4 is being used. https://github.com/aptos-labs/aptos-core/blob/main/types/src/transaction/authenticator.rs#L484

Also note, for safety the format would probably be like: sha3_256(FI::data::5)

lightmark · 2025-02-07T18:55:54Z

aptos-move/framework/aptos-framework/sources/account/auth_data.move

+    enum DomainAccount has copy, drop {
+        V1 {
+            domain_name: String,
+            account_authentication_key: vector<u8>,


What's this? @igor-aptos

this is the identifier of the source account.

i.e. in what David wrote above - for the account address account will have - sha3_256(FI::data::5), that is the data.

It can be an address of the original chain for example, in the format implementation of AA function will use

Then it seems inappropriate to let user decide this field. Maybe it should be derived.

I meant - the implementation of AA function needs to verify that this matches with the signature.

I am not sure what "derived" means?

ignore my "derived" words.

the implementation of AA function needs to verify that this matches with the signature.
Then for a key pair, this should be the public key part which is part of the authenticator itself.

Then for a key pair, this should be the public key part which is part of the authenticator itself.

yes, but aa dispatch cannot look where in authenticator is, right? so it seems like it needs to be here, and implementation of AA needs to check that it matches what is in the authenticator?

oh yeah, the implemetation(AA function info) actually knows, and it can represent the domain itself.

lightmark · 2025-02-07T20:06:18Z

I had a discussion with David and took some time to reflect on this.

Indeed, auth_function: FunctionInfo itself determines the domain, making it a suitable identifier for a specific domain.
Our objective is to seamlessly map an external account to Aptos. To achieve this, we must requisition an unowned account using the derived address from user data. For example, if the key pair is ed25519, the public key should be encoded in the authenticator, which will then be used to derive the corresponding Aptos account address. This account must be set up to support domain-specific AA authentication.
The logic that derives and requisitions an Aptos account resides within the Aptos framework. It is essential that this logic remains as strict as possible to avoid polluting on-chain storage. If the customized function returns incorrect data, the account derivation and requisition process may malfunction, potentially wasting on-chain account space. Therefore, we do not leave everything to the user-space code but delegate only part of it. However, this delegation can be problematic since the framework depends on the output. My suggestion is that the principle for allowing customized logic should be to limit its scope to user space as much as possible. For example, if account A's AA function is B, if B has a bug, only A can be hacked. But if we allow everyone define a domain specific AA and the framework will derive and requisition account for them, is that too much?

graphite-app · 2025-02-10T22:33:16Z

aptos-move/framework/aptos-framework/sources/account/domain_aa/common_domain_aa_auths.move

@@ -0,0 +1,56 @@
+module aptos_framework::common_domain_aa_auths {


Missing error constant definitions at the start of the module. Please add:

const EINVALID_ACCOUNT_IDENTITY: u64 = 1; const EINVALID_SIGNATURE: u64 = 2;

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-02-10T22:33:17Z

aptos-move/framework/aptos-framework/sources/account/domain_aa/common_domain_aa_auths.move

+    fun bytes_to_hex(data: &vector<u8>): vector<u8> {
+        // let result = vector::empty();
+        // let i = 0;
+        // while (i < data.length()) {
+        //     let cur = data[i];
+        //     vector::push_back(&mut result, high);
+        //     vector::push_back(&mut result, low);
+        //     i = i + 1;
+        // };
+        // result
+    }


The bytes_to_hex function is currently stubbed out but is a critical dependency for authenticate_ed25519_hex. This function needs to be implemented to convert raw bytes to their hexadecimal string representation. Consider implementing it using a lookup table for efficiency, with each byte being converted to two hex characters. The current commented implementation shows the right approach but is missing the hex conversion logic.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-02-10T22:33:18Z

aptos-move/framework/aptos-framework/sources/account/account_abstraction.move

+            let func_infos = &borrow_global<DomainDispatchableAuthenticator>(@aptos_framework).auth_functions;
+            assert!(func_infos.contains(&func_info), error::not_found(EFUNCTION_INFO_EXISTENCE));
+


The code needs to verify that the domain name in signing_data matches the domain registered for this authentication function. Currently, it only checks that the function exists in the registry but not that it's associated with the correct domain. This could allow an attacker to use any registered authentication function with any registered domain. Consider adding:

assert!(func_infos.borrow(&func_info) == domain, error::not_found(EFUNCTION_INFO_EXISTENCE));

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

igor-aptos · 2025-02-10T22:43:18Z

updated to use FunctionInfo as domain itself.
Added common_domain_aa_auths as example of what would be needed for a chains with ed25519 that only sign strings.

I cannot see how to extract account_identity from authenticator generically, so implementation of domain AA needs to do that - and I need to add it to AbstractionAuthData.

It is essential that this logic remains as strict as possible to avoid polluting on-chain storage

as long as namespace is separate, I don't see this being any different than creating object at AUID address?

For example, if account A's AA function is B, if B has a bug, only A can be hacked. But if we allow everyone define a domain specific AA and the framework will derive and requisition account for them, is that too much?

what does this mean?
If B has a bug, then all accounts that used B can be hacked.
In domain specific AA - if function for one domain has bug - all accounts in that domain can be hacked - but no other accounts

lightmark · 2025-02-11T09:49:46Z

aptos-move/framework/aptos-framework/sources/account/account_abstraction.move

+    /// TODO: probably worth creating some module with all these derived functions,
+    /// and do computation/caching in rust to avoid recomputation, as we do for objects.
+    public fun domain_aa_account_address(domain_func_info: FunctionInfo, account_identity: &vector<u8>): address {
+        /// using bcs serialized structs here - this allows for no need for separators.


because the length encoding?

lightmark · 2025-02-11T09:51:26Z

aptos-move/framework/aptos-framework/sources/account/account_abstraction.move

@@ -214,18 +218,19 @@ module aptos_framework::account_abstraction {
        func_info: FunctionInfo,
        signing_data: AbstractionAuthData,
    ): signer acquires DispatchableAuthenticator, DomainDispatchableAuthenticator {
+        assert!(using_dispatchable_authenticator(@aptos_framework), error::not_found(EDISPATCHABLE_AUTHENTICATOR_IS_NOT_USED));


what about normal aa? is this required?

lightmark · 2025-02-11T16:33:52Z

what does this mean? If B has a bug, then all accounts that used B can be hacked. In domain specific AA - if function for one domain has bug - all accounts in that domain can be hacked - but no other accounts

The difference is A choose to use B by themselves registering B for their accounts. But here, if B is not written/reviewed by us and we mapped account in aptos framework for them, it is a risk since B can have shitty code.

igor-aptos · 2025-02-12T22:49:57Z

updated, and added an end-to-end smoke test showing the flow.

Current AA requires registration for each account, and user has to have private key for an address to be able to do so. This change allows for AA to be per domain - for example for Solana we need "ed25519_hex", and domain-separated addresses to automatically accept it.

simplifying the implementation of common_domain_aa_auths::authenticate_ed25519_hex, as we don't need public_key in both account_identity and authenticator

lightmark · 2025-02-14T16:18:31Z

aptos-move/aptos-vm/src/aptos_vm.rs

+                    let enabled = if let AbstractionAuthData::V1 { .. } = auth_data {
+                        self.features().is_account_abstraction_enabled()
+                    } else {
+                        self.features().is_domain_account_abstraction_enabled()


let's specify domain V1 here instead.

lightmark · 2025-02-14T16:19:34Z

aptos-move/framework/aptos-framework/sources/account/account_abstraction.move

@@ -23,6 +29,9 @@ module aptos_framework::account_abstraction {
    const ENOT_MASTER_SIGNER: u64 = 4;
    const EINCONSISTENT_SIGNER_ADDRESS: u64 = 5;
    const EDEPRECATED_FUNCTION: u64 = 6;
+    const EDOMAIN_AA_NOT_INITIALIZED: u64 = 6;
+
+    const DOMAIN_ABSTRACTION_DERIVED_SCHEME: u8 = 5;


where does this 5 come from?

lightmark · 2025-02-14T16:22:53Z

aptos-move/framework/aptos-framework/sources/account/account_abstraction.move

+    }
+
+    #[view]
+    public fun domain_aa_account_address_view(


can we change the name? the module itself is account abstraction, so having aa in the name is not very decent.
how about derive_domain_address()

lightmark · 2025-02-14T16:24:54Z

aptos-move/framework/aptos-framework/sources/account/account_abstraction.move

@@ -45,6 +54,53 @@ module aptos_framework::account_abstraction {
        V1 { auth_functions: OrderedMap<FunctionInfo, bool> }
    }

+    /// The dispatchable domain-scoped authenticator, that defines how to authenticate
+    enum DomainDispatchableAuthenticator has key {
+        V1 { auth_functions: BigOrderedMap<FunctionInfo, bool> }


can we make the value an enum in case of upgrade.

lightmark · 2025-02-14T16:27:26Z

aptos-move/framework/aptos-framework/sources/account/account_abstraction.move

+        );
+    }
+
+    entry fun register_domain_with_authentication_function_test_network_only(


possibly move to a test file?

lightmark · 2025-02-14T16:31:42Z

aptos-move/framework/aptos-framework/sources/account/domain_aa/common_domain_aa_auths.move

@@ -0,0 +1,32 @@
+module aptos_framework::common_domain_aa_auths {


The name is not decent imho...
could we make an account_abstraction folder and put both files inside?

lightmark · 2025-02-14T16:33:05Z

aptos-move/framework/aptos-framework/sources/transaction_validation.move

        txn_sender_public_key: Option<vector<u8>>,
+        // None means no need to check, i.e. either AA (where it is already checked) or simulation


lightmark · 2025-02-14T17:01:09Z

aptos-move/framework/aptos-framework/sources/account/account_abstraction.move

+            // so we need to have it separate, and require authenticate function to confirm it matches.
+            assert!(master_signer_addr == domain_aa_account_address(func_info, signing_data.domain_account_identity()), error::invalid_state(EINCONSISTENT_SIGNER_ADDRESS));
+        } else {
+            assert!(using_dispatchable_authenticator(@aptos_framework), error::not_found(EDISPATCHABLE_AUTHENTICATOR_IS_NOT_USED));


Suggested change

assert!(using_dispatchable_authenticator(@aptos_framework), error::not_found(EDISPATCHABLE_AUTHENTICATOR_IS_NOT_USED));

assert!(using_dispatchable_authenticator(master_signer_addr), error::not_found(EDISPATCHABLE_AUTHENTICATOR_IS_NOT_USED));

igor-aptos requested review from alinush, davidiw, movekevin and wrwg as code owners February 6, 2025 06:10

lightmark reviewed Feb 6, 2025

View reviewed changes

davidiw reviewed Feb 6, 2025

View reviewed changes

lightmark reviewed Feb 7, 2025

View reviewed changes

igor-aptos requested a review from BriungRi February 7, 2025 19:47

igor-aptos force-pushed the igor/aa_per_domain branch from 3cff6bd to ed074aa Compare February 10, 2025 22:32

graphite-app bot reviewed Feb 10, 2025

View reviewed changes

igor-aptos force-pushed the igor/aa_per_domain branch from ed074aa to e7be2c8 Compare February 10, 2025 22:36

lightmark reviewed Feb 11, 2025

View reviewed changes

igor-aptos changed the title ~~[RFC] AccountAbstraction that works per domain~~ AccountAbstraction that works per domain Feb 12, 2025

igor-aptos requested review from davidiw and lightmark February 12, 2025 22:50

lightmark mentioned this pull request Feb 13, 2025

Domain AA aptos-foundation/AIPs#559

Open

igor-aptos added 7 commits February 13, 2025 23:30

addressing some changes, showcasing move code

9a19dcc

adding rust changes

8235106

Making the full flow work.

a3ba5f1

simplifying the implementation of common_domain_aa_auths::authenticate_ed25519_hex, as we don't need public_key in both account_identity and authenticator

fix test and allowing fee payer to create account for domain aa

bf3bdc5

lints

45c89a4

feature flag

8c611e0

igor-aptos force-pushed the igor/aa_per_domain branch from 7a23d84 to 8c611e0 Compare February 14, 2025 08:12

igor-aptos requested review from zekun000, vgao1996 and georgemitenkov as code owners February 14, 2025 08:12

lightmark reviewed Feb 14, 2025

View reviewed changes

		@@ -0,0 +1,56 @@
		module aptos_framework::common_domain_aa_auths {

		let func_infos = &borrow_global<DomainDispatchableAuthenticator>(@aptos_framework).auth_functions;
		assert!(func_infos.contains(&func_info), error::not_found(EFUNCTION_INFO_EXISTENCE));

		@@ -0,0 +1,32 @@
		module aptos_framework::common_domain_aa_auths {

		txn_sender_public_key: Option<vector<u8>>,
		// None means no need to check, i.e. either AA (where it is already checked) or simulation

	assert!(using_dispatchable_authenticator(@aptos_framework), error::not_found(EDISPATCHABLE_AUTHENTICATOR_IS_NOT_USED));
	assert!(using_dispatchable_authenticator(master_signer_addr), error::not_found(EDISPATCHABLE_AUTHENTICATOR_IS_NOT_USED));

AccountAbstraction that works per domain #15899

Are you sure you want to change the base?

AccountAbstraction that works per domain #15899

Conversation

igor-aptos commented Feb 6, 2025

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

trunk-io bot commented Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightmark Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidiw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-aptos commented Feb 7, 2025

davidiw commented Feb 7, 2025

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

lightmark commented Feb 7, 2025

graphite-app bot Feb 10, 2025

Choose a reason for hiding this comment

graphite-app bot Feb 10, 2025

Choose a reason for hiding this comment

graphite-app bot Feb 10, 2025

Choose a reason for hiding this comment

igor-aptos commented Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightmark commented Feb 11, 2025

igor-aptos commented Feb 12, 2025

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

trunk-io bot commented Feb 6, 2025 •

edited

Loading

lightmark Feb 7, 2025 •

edited

Loading

igor-aptos commented Feb 10, 2025 •

edited

Loading