Attempt to address merge conflicts related to automation feature #291
Conversation
isaacdoidge
left a comment
isaacdoidge
left a comment
There was a problem hiding this comment.
I wasn't able to review everything deeply, but looks good for the most part. There are a few things that we need to check and adjust r.e. the feature flags though.
| APTOS_STD_CHAIN_ID_NATIVES = 4, | ||
| VM_BINARY_FORMAT_V6 = 5, | ||
| COLLECT_AND_DISTRIBUTE_GAS_FEES = 6, | ||
| _DEPRECATED_COLLECT_AND_DISTRIBUTE_GAS_FEES = 6, |
There was a problem hiding this comment.
Probably worth checking the Move code related to this too.
There was a problem hiding this comment.
The flag was not enabled by default. In move it was used in scope of block, stake and transaction_validation modules.
| (_LimitTypeTagSize, TESTNET) => BEGINNING_OF_TIME, | ||
| (_LimitTypeTagSize, MAINNET) => BEGINNING_OF_TIME, | ||
|
|
||
| // TODO clarify whether it is June 21 or June 25 for supra, below is the old code from our branch |
There was a problem hiding this comment.
We started testnet on Tuesday, September 17, 2024 12:00:00.000 PM (UTC). If this code was not present in our code when we launched then we will need to increase the following trigger dates to a date after the release that contains this code (which date hasn't been set yet).
There was a problem hiding this comment.
Line 81 was part of the our code-base, so I guess we had those values when test net started.
| }; | ||
| use aptos_crypto::{ed25519, hash::HashValue}; | ||
| use aptos_crypto::{ | ||
| bls12381, |
| ABORT_IF_MULTISIG_PAYLOAD_MISMATCH = 70, | ||
| // Keeping 16 bit reserved to have graceful updated from aptos-mainstream in case of new flags have been added | ||
| /// Enabled on mainnet, cannot be disabled. | ||
| _DISALLOW_USER_NATIVES = 71, |
There was a problem hiding this comment.
Can you please confirm that we have already enabled all feature flags that have been deprecated and are documented as having been enabled by Aptos?
There was a problem hiding this comment.
All the flags after 70 are new and were introduced after aptos-v1.16.
So they are for sure were not enabled by default at genesis.
Note that 88-96 where introduced by us.
| // Feature flag V6 is used to enable metadata v1 format and needs to stay on, even | ||
| // if we enable a higher version. | ||
| FeatureFlag::VM_BINARY_FORMAT_V6, | ||
| FeatureFlag::VM_BINARY_FORMAT_V7, |
There was a problem hiding this comment.
Let's not add any features to the default feature set that we have yet to activate in production. Otherwise the localnet might behave unexpectedly. Please add some code comments to indicate which feature flags have been introduced during this upgrade and need to considered for adoption in production.
There was a problem hiding this comment.
Reverted default set to the one that we have in dev branch
| txn_expiration_time <= timestamp::now_seconds() + MAX_EXPIRATION_TIME_SECONDS_FOR_ORDERLESS_TXNS, | ||
| error::invalid_argument(PROLOGUE_ETRANSACTION_EXPIRATION_TOO_FAR_IN_FUTURE), | ||
| ); | ||
| assert!(nonce_validation::check_and_insert_nonce(sender, nonce, txn_expiration_time), error::invalid_argument(PROLOGUE_ENONCE_ALREADY_USED)); |
There was a problem hiding this comment.
Where is this function defined? I can't see nonce_validation.move in the PR files.
There was a problem hiding this comment.
nonce_validation.move was added in scope of the base branch issue/framework_upgrade_v1.34.0
aregng
force-pushed
the
issue/framework_upgrade_v1.34.0
branch
from
4a034ca to
42019bc
Compare
aregng
force-pushed
the
issue/framework_upgrade_v1.34.0-automation
branch
from
580291a to
9073faf
Compare
| KEYLESS_BUT_ZKLESS_ACCOUNTS = 47, | ||
| REMOVE_DETAILED_ERROR_FROM_HASH = 48, | ||
| /// This feature was never used. | ||
| _DEPRECATED_REMOVE_DETAILED_ERROR_FROM_HASH = 48, |
There was a problem hiding this comment.
Was not enabled by default and is not used anywhere in scope of move, only one reference in this file:
aptos-core/types/src/transaction/mod.rs
Line 1040 in 9a9bd28
No description provided.