Refactor and fix auth key formula in account resortation processor #705
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #705 +/- ##
=======================================
+ Coverage 50.3% 50.5% +0.1%
=======================================
Files 253 253
Lines 28643 29244 +601
=======================================
+ Hits 14432 14791 +359
- Misses 14211 14453 +242 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks for the refactor! @heliuchuan
Utilizing AccountSignature and adding FeePayer support looks good to me.
Regarding auth key derivation logic, left some comments.
| @@ -28,8 +28,7 @@ impl AuthKeyScheme for Ed25519AuthKeyScheme { | |||
| const SCHEME: u8 = 0x00; | |||
|
|
|||
| fn auth_key(&self) -> Option<String> { | |||
| let mut preimage = self.public_key.clone(); | |||
| preimage.push(Self::SCHEME); | |||
There was a problem hiding this comment.
Shouldn't the preimage include SCHEME suffix here?
There was a problem hiding this comment.
the legacy ed25519 does not use scheme. scheme was added to abstract to diff auth schemes.
There was a problem hiding this comment.
https://github.com/aptos-labs/aptos-core/blob/main/types/src/transaction/authenticator.rs#L765
https://github.com/aptos-labs/aptos-core/blob/main/types/src/transaction/authenticator.rs#L744
Doesn't this mean the legacy ed25519 includes the 0x00 scheme?
There was a problem hiding this comment.
hmm yeah you are totally right. i messed up my test somehow. fixed
| let mut preimage = vec![key_type.to_u8()]; | ||
| match key_type { | ||
| AnyPublicKeyType::Ed25519 => preimage.push(0x20), | ||
| AnyPublicKeyType::Secp256k1Ecdsa => preimage.push(0x41), |
There was a problem hiding this comment.
Why do we need 0x20 or 0x41 here?
There was a problem hiding this comment.
this equates to 32 and 65 respectively. it encodes the length which happens to be used when hashing the public key.
There was a problem hiding this comment.
Gotcha, just to confirm - is the length prefix part of BCS encoding
There was a problem hiding this comment.
It unfortunately is not standardize between schemes. But it is for Ed25519 (which I believe is just a byproduct of the library used to serialize the pubkey) and Secp256k1Ecdsa (which is actually part of the standard way to represent the pubkey).
Keyless does not.
|
@heliuchuan could you make this change in this repo https://github.com/aptos-labs/aptos-indexer-processors-v2. we are trying to deprecate this repo starting next week |
No description provided.