deprecate stale fee collection implementation #12800
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #12800 +/- ##
===========================================
- Coverage 72.8% 59.8% -13.1%
===========================================
Files 2397 852 -1545
Lines 483043 207730 -275313
===========================================
- Hits 352056 124362 -227694
+ Misses 130987 83368 -47619
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| public fun collect_and_distribute_gas_fees(): bool acquires Features { | ||
| is_enabled(COLLECT_AND_DISTRIBUTE_GAS_FEES) | ||
| /// Deprecated feature | ||
| public fun get_collect_and_distribute_gas_fees_feature(): u64 { abort error::invalid_argument(EINVALID_FEATURE) } |
There was a problem hiding this comment.
Should we use #[deprecated] as well here?
| // is tested and is fully proven to work well. | ||
| transaction_fee_amount | ||
| }; | ||
| let amount_to_burn = transaction_fee_amount; |
There was a problem hiding this comment.
Let's just replace amount_to_burn with transaction_fee_amount? No need to create extra variables because we do not have an optimizer right now :/
| } else { | ||
| transaction_fee_amount - storage_fee_refunded | ||
| }; | ||
| let amount_to_burn= transaction_fee_amount - storage_fee_refunded; |
There was a problem hiding this comment.
amount_to_burn= ==> amount_to_burn =
aptos-move/framework/aptos-framework/sources/transaction_fee.spec.move
Outdated
Show resolved
Hide resolved
| @@ -285,23 +285,14 @@ spec aptos_framework::transaction_validation { | |||
|
|
|||
|
|
|||
| // Check fee collection. | |||
| let collect_fee_enabled = features::spec_is_enabled(features::COLLECT_AND_DISTRIBUTE_GAS_FEES); | |||
| let collected_fees = global<CollectedFeesPerBlock>(@aptos_framework).amount; | |||
There was a problem hiding this comment.
You can replace this with assertion that !features::spec_is_enabled(features::COLLECT_AND_DISTRIBUTE_GAS_FEES) or just remove it completely
| @@ -78,122 +78,20 @@ module aptos_framework::transaction_fee { | |||
| /// distribution. Should be called by on-chain governance. | |||
| public fun initialize_fee_collection_and_distribution(aptos_framework: &signer, burn_percentage: u8) { | |||
| system_addresses::assert_aptos_framework(aptos_framework); | |||
There was a problem hiding this comment.
Why not replace the whole body with abort error::invalid_argument(ENO_LONGER_SUPPORTED)? or even better EINVALID_FEATURE is defined in featres.move. Maybe we can add a public function unsupported_feature(code: u8) which will be more similar to the already used convention?
aptos-move/framework/aptos-framework/sources/transaction_fee.move
Outdated
Show resolved
Hide resolved
| @@ -247,169 +134,4 @@ module aptos_framework::transaction_fee { | |||
|
|
|||
| #[test_only] | |||
| use aptos_framework::aggregator_factory; | |||
There was a problem hiding this comment.
We do not need this either?
| @@ -1242,14 +1242,6 @@ module aptos_framework::stake { | |||
| }; | |||
|
|
|||
| let cur_fee = 0; | |||
There was a problem hiding this comment.
Let's remove cur_fee?
| /// the coin in every transaction avoiding read-modify-write conflicts. Only | ||
| /// used for gas fees distribution by Aptos Framework (0x1). | ||
| #[deprecated] | ||
| /// DEPRECATED | ||
| struct AggregatableCoin<phantom CoinType> has store { |
There was a problem hiding this comment.
why this is incompatible to remove, I wonder. Structs are private, so if there are no functions which can create one, they cannot be used outside. So we should be able to remove resources if they only have private or friend APIs, in theory?
There was a problem hiding this comment.
If this is truly private that only bypass function calls can access it, it might be, but that's such a special exception, I can see why no one would code for that case :P
There was a problem hiding this comment.
it is not safe to remove private struct that has "store", as you would be able to create a new one with different fields, and deserialize into it - breaking all type invariants
There was a problem hiding this comment.
it is not safe to remove private struct that has "store", as you would be able to create a new one with different fields, and deserialize into it - breaking all type invariants
I think my original statement was in context to it being part of the framework, but the general statement is definitely true! Type confusion can happen sooo easily...
There was a problem hiding this comment.
Yes, that's why it seemed safe for me to remove: it is part of framework, and has private APIs, so even if you deserialise into it what do you do after? And in any case you cannot deserialise because it is a private type
|
This issue is stale because it has been open 45 days with no activity. Remove the |
igor-aptos
force-pushed
the
igor/deprecate_fee_collection
branch
from
1a77376 to
34ee1b0
Compare
igor-aptos
force-pushed
the
igor/deprecate_fee_collection
branch
from
34ee1b0 to
501391f
Compare
igor-aptos
force-pushed
the
igor/deprecate_fee_collection
branch
from
501391f to
abd907a
Compare
igor-aptos
force-pushed
the
igor/deprecate_fee_collection
branch
from
abd907a to
f856e1f
Compare
aptos-move/framework/aptos-framework/sources/transaction_fee.move
Outdated
Show resolved
Hide resolved
| // If checks did not pass, simply burn all collected coins and return none. | ||
| burn_coin_fraction(&mut coin, 100); | ||
| coin::destroy_zero(coin) | ||
| _aptos_framework: &signer, |
There was a problem hiding this comment.
#[deprecated] is missing here, maybe remove the comment and use /// DEPRECATED.
| @@ -85,126 +85,19 @@ module aptos_framework::transaction_fee { | |||
| storage_fee_refund_octas: u64, | |||
| } | |||
|
|
|||
| #[deprecated] | |||
| /// Initializes the resource storing information about gas fees collection and | |||
There was a problem hiding this comment.
Add /// DEPRECATED.?
|
I think it's fine to remove and add it back |
| @@ -44,6 +43,7 @@ module aptos_framework::transaction_fee { | |||
| mint_cap: MintCapability<AptosCoin>, | |||
| } | |||
|
|
|||
| #[deprecated] | |||
There was a problem hiding this comment.
could we move the deprecated fields to the bottom of the file?
| public fun get_collect_and_distribute_gas_fees_feature(): u64 { COLLECT_AND_DISTRIBUTE_GAS_FEES } | ||
| #[deprecated] | ||
| /// Deprecated feature | ||
| public fun get_collect_and_distribute_gas_fees_feature(): u64 { |
There was a problem hiding this comment.
I am not sure abort here is a good practice since anyone can call this func even it is not used by us.
igor-aptos
force-pushed
the
igor/deprecate_fee_collection
branch
from
3ef1e13 to
3ee5b46
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.
igor-aptos
force-pushed
the
igor/deprecate_fee_collection
branch
from
3ee5b46 to
df83de3
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
|
Current implementation is stale, and cannot be used as defined. So better to remove it, and add it back later if needed.
Let me know if you have preference on whether to keep or remove stake related changes, as those might be more similar for future stake changes. Removed them in this PR, as adding them back should be as easy as this removal is.
Description
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Key Areas to Review
Checklist