-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Aptos - Account Abstraction #15219
Aptos - Account Abstraction #15219
Conversation
⏱️ 24m total CI duration on this PR
|
502e9cb
to
b67b84f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we assert that signer AA returns is for the same account (i.e. account addresses match), to potentially avoid some class of problems
@@ -251,6 +251,7 @@ crate::gas_schedule::macros::define_gas_parameters!( | |||
[function_info_check_dispatch_type_compatibility_impl_base: InternalGas, { RELEASE_V1_13.. => "function_info.check_dispatch_type_compatibility_impl.base" }, 1002], | |||
[function_info_load_function_base: InternalGas, { RELEASE_V1_13.. => "function_info.load_function.base" }, 551], | |||
[dispatchable_fungible_asset_dispatch_base: InternalGas, { RELEASE_V1_13.. => "dispatchable_fungible_asset.dispatch.base" }, 551], | |||
[dispatchable_authenticate_dispatch_base: InternalGas, { RELEASE_V1_23.. => "dispatchable_authenticate.dispatch.base" }, 551], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's 25 now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.26 now
aptos-move/aptos-vm/src/aptos_vm.rs
Outdated
|
||
let vm_gas_params = self.gas_params(log_context)?.vm.clone(); | ||
let storage_gas_params = self.storage_gas_params(log_context)?.clone(); | ||
let gen_gas_meter = || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vgao1996 can you take a look at the gas metering logic here, if it makes sense?
e83063d
to
45b271c
Compare
@@ -12,6 +12,7 @@ async fn test_mint_transfer() { | |||
let swarm = new_local_swarm_with_aptos(1).await; | |||
let mut info = swarm.aptos_public_info(); | |||
|
|||
println!("1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated
secondary_signer_public_key_hashes[j] == account::get_authentication_key(secondary_signer_addresses[j]) || | ||
(features::spec_simulation_enhancement_enabled() && is_simulation && vector::is_empty(secondary_signer_public_key_hashes[j])); | ||
}; | ||
// spec { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @rahxephon89
serialized_signers.sender(), | ||
serialized_signers | ||
.fee_payer() | ||
.expect("fee_payer signer must exist"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid expect and unwrap here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this ever happen? If so - i.e. on bad user input - expect will crash the node, while returning error VMStatus will abort/discard transaction.
probably safer to return error VMStatus - as even if this is invariant, you depend on a lot of callers passing in correct signers for this not to crash
on the other hand, things like:
MoveValue::U64(txn_sequence_number)
.simple_serialize()
.unwrap(),
are fine as you can easily validate it will never crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit concerned over the changes in the transaction validation. There's a bunch of code reordering and I think we need to reason about the changes there very carefully...
45b271c
to
74b6e65
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
74b6e65
to
e4255f5
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
/// Note: it is a private entry function that can only be called directly from transaction. | ||
public entry fun add_dispatchable_authentication_function( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment says it's a private entry function but it's not. same for remove_dispatchable_authenticator
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
❌ Forge suite
|
❌ Forge suite
|
✅ Forge suite
|
Description
a function with the following signature can be used to authorize account..
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Key Areas to Review
lite_account.move
signing_data.move
aptos_vm.rs