-
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
Remove the "remove_error_details" change #15331
base: main
Are you sure you want to change the base?
Conversation
⏱️ 7h 46m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
6257556
to
86056a3
Compare
632487e
to
0f9c1d0
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.
@@ -489,6 +483,7 @@ impl AptosDB { | |||
Ok(root_hash) | |||
} | |||
|
|||
#[allow(dead_code)] |
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 #[allow(dead_code)]
annotation appears incorrect since commit_transaction_auxiliary_data()
is called in the commit_chunk_impl()
method. Consider removing this annotation or documenting why this function appears unused if there are special circumstances around its usage pattern.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
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.
@@ -163,7 +163,6 @@ impl FeatureFlag { | |||
FeatureFlag::COIN_TO_FUNGIBLE_ASSET_MIGRATION, | |||
FeatureFlag::OBJECT_NATIVE_DERIVED_ADDRESS, | |||
FeatureFlag::DISPATCHABLE_FUNGIBLE_ASSET, | |||
FeatureFlag::REMOVE_DETAILED_ERROR_FROM_HASH, |
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 we also remove is_remove_detailed_error_from_hash_enabled
? Also we can rename the feature flag to indicate this is unused?
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.
we probably shouldn't remove feature and thus its corresponding functions. I added a comment to indicate this feature is not used.
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.
nit: I think you can just call the feature flag as _DEPRECATED_...
- we already have this for transaction fees and DKG feature flags. Better than the comment because it is explicitly deprecated. I would still prefer to remove is_remove_detailed_error_from_hash_enabled
because if feature is removed, it should not have public APIs
aptos-move/e2e-testsuite/proptest-regressions/tests/account_universe/peer_to_peer.txt
Outdated
Show resolved
Hide resolved
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.
Approved by mistake, want to comment only😄 requesting changes to override
This comment has been minimized.
This comment has been minimized.
0e6393e
to
fd5f4ba
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
fd5f4ba
to
e5d8752
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.
e5d8752
to
0c7efcb
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.
0c7efcb
to
ec70789
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.
❌ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
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.
LGTM, just one small comment. Please make sure all tests pass - I see some Move related tests still fail due to missing error statuses
@@ -163,7 +163,6 @@ impl FeatureFlag { | |||
FeatureFlag::COIN_TO_FUNGIBLE_ASSET_MIGRATION, | |||
FeatureFlag::OBJECT_NATIVE_DERIVED_ADDRESS, | |||
FeatureFlag::DISPATCHABLE_FUNGIBLE_ASSET, | |||
FeatureFlag::REMOVE_DETAILED_ERROR_FROM_HASH, |
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.
nit: I think you can just call the feature flag as _DEPRECATED_...
- we already have this for transaction fees and DKG feature flags. Better than the comment because it is explicitly deprecated. I would still prefer to remove is_remove_detailed_error_from_hash_enabled
because if feature is removed, it should not have public APIs
Description
withdrew this change due to negative ecosystem impact after the discussion
How Has This Been Tested?
Existing tests
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist