-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Cleanup after aggregator_v2_api and concurrent token features #13247
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13247 +/- ##
=========================================
- Coverage 57.5% 57.5% -0.1%
=========================================
Files 832 833 +1
Lines 198522 198662 +140
=========================================
+ Hits 114269 114280 +11
- Misses 84253 84382 +129 ☔ View full report in Codecov by Sentry. |
01d8c4c
to
2bb15be
Compare
2bb15be
to
59de22a
Compare
Those two features will not be reverted. If there is any issue with the aggregators, feature that controls the fallback will be used, but API will still work.
59de22a
to
26b579d
Compare
public fun get_auids(): u64 { APTOS_UNIQUE_IDENTIFIERS } | ||
public fun get_auids(): u64 { | ||
// Deployed to production, and disabling deprecated. | ||
error::invalid_argument(EINVALID_FEATURE) |
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.
Probably permission_denied or something like this would be better? With a new error code which is like EFEATURE_CANNOT_BE_DISABLED or something like that
public fun get_aggregator_v2_api_feature(): u64 { AGGREGATOR_V2_API } | ||
public fun get_aggregator_v2_api_feature(): u64 { | ||
// API fully rolled out, cannot be reverted any more | ||
abort error::invalid_argument(EINVALID_FEATURE) |
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.
Same here
let feature = features::get_concurrent_token_v2_feature(); | ||
let agg_feature = features::get_aggregator_v2_api_feature(); | ||
let auid_feature = features::get_auids(); | ||
let module_event_feature = features::get_module_event_feature(); |
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.
module event feature also cannot be reverted?
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 can be reverted, but default is true - so we don't need to enable it in this test.
though that can potentially also be cleaned up @lightmark
}; | ||
|
||
(index, name) | ||
// If aggregator_api_enabled, we always populate newly added fields |
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.
The comment seems off? aggregator_api_enabled is always enabled
addressed comments, ping to other reviewers |
45f0ee3
to
b889ad1
Compare
Those two features will not be reverted.
If there is any issue with the aggregators, feature that controls the fallback will be used, but API will still work.
This simplifies the complicated nested if conditions in collection.move and token.move
Description
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Key Areas to Review
Checklist