feat(sequencer)!: rework all fees by ethanoroshiba · Pull Request #1647 · astriaorg/astria

ethanoroshiba · 2024-10-08T22:31:24Z

Summary

Gutted all current fee handling and replaced all with single fee calculation method in new trait.

Background

This implementation will simplify not only our fee calculation, but the process for adding new actions in the future.

Changes

Created FeeComponents types for all fee-bearing transactions which all have a base fee component and a computed cost multiplier component.
Moved all fee checks and payment to one single function within the new sequencer fees component. Fee calculation is now always the following formula: base_fee + computed_cost_base * computed_cost_multiplier
Moved all state reads/writes for fees to the new fees component.
Initialized all fees in the fee component's init_chain().
Changed FeeChange to be an enum which takes any fee-bearing action's fee components.
Moved FeeChange and FeeAssetChange's ActionHandler impls to the fees component.
Allowed fee assets now stored in verifiable storage instead of non-verifiable.

Testing

All previous tests passing, added new tests for all state fee reads/writes.

Breaking Changelist

Changed shape of FeeChange action.
Added storage of all the action fees, breaking the app hash.
Changed app genesis.
Removed a bunch of storage keys, breaking these snapshot tests
Allowed fee assets moved from non-verifiable to verifiable storage.

Related Issues

closes #1369
closes #1145

charts/sequencer/files/cometbft/config/genesis.json

Co-authored-by: Jordan Oroshiba <jordan@astria.org>

joroshiba · 2024-10-15T23:00:11Z

crates/astria-sequencer/src/fees/component.rs

+        let transfer_fees = app_state.fees().transfer;
+        state
+            .put_transfer_fees(transfer_fees)
+            .wrap_err("failed to store transfer fee components")?;
+
+        let sequence_fees = app_state.fees().sequence;
+        state
+            .put_sequence_fees(sequence_fees)
+            .wrap_err("failed to store sequence action fee components")?;
+
+        let ics20_withdrawal_fees = app_state.fees().ics20_withdrawal;
+        state
+            .put_ics20_withdrawal_fees(ics20_withdrawal_fees)
+            .wrap_err("failed to store ics20 withdrawal fee components")?;
+
+        let init_bridge_account_fees = app_state.fees().init_bridge_account;
+        state
+            .put_init_bridge_account_fees(init_bridge_account_fees)
+            .wrap_err("failed to store init bridge account fee components")?;
+
+        let bridge_lock_fees = app_state.fees().bridge_lock;
+        state
+            .put_bridge_lock_fees(bridge_lock_fees)
+            .wrap_err("failed to store bridge lock fee components")?;
+
+        let bridge_unlock_fees = app_state.fees().bridge_unlock;
+        state
+            .put_bridge_unlock_fees(bridge_unlock_fees)
+            .wrap_err("failed to store bridge unlock fee components")?;
+
+        let bridge_sudo_change_fees = app_state.fees().bridge_sudo_change;
+        state
+            .put_bridge_sudo_change_fees(bridge_sudo_change_fees)
+            .wrap_err("failed to store bridge sudo change fee components")?;
+
+        let ibc_relay_fees = app_state.fees().ibc_relay;
+        state
+            .put_ibc_relay_fees(ibc_relay_fees)
+            .wrap_err("failed to store ibc relay fee components")?;
+
+        let validator_update_fees = app_state.fees().validator_update;
+        state
+            .put_validator_update_fees(validator_update_fees)
+            .wrap_err("failed to store validator update fee components")?;
+
+        let fee_asset_change_fees = app_state.fees().fee_asset_change;
+        state
+            .put_fee_asset_change_fees(fee_asset_change_fees)
+            .wrap_err("failed to store fee asset change fee components")?;
+
+        let fee_change_fees = app_state.fees().fee_change;
+        state
+            .put_fee_change_fees(fee_change_fees)
+            .wrap_err("failed to store fee change fee components")?;
+
+        let ibc_relayer_change_fees = app_state.fees().ibc_relayer_change;
+        state
+            .put_ibc_relayer_change_fees(ibc_relayer_change_fees)
+            .wrap_err("failed to store ibc relayer change fee components")?;
+
+        let sudo_address_change_fees = app_state.fees().sudo_address_change;
+        state
+            .put_sudo_address_change_fees(sudo_address_change_fees)
+            .wrap_err("failed to store sudo address change fee components")?;
+
+        let ibc_sudo_change_fees = app_state.fees().ibc_sudo_change;
+        state
+            .put_ibc_sudo_change_fees(ibc_sudo_change_fees)
+            .wrap_err("failed to store ibc sudo change fee components")?;


Should it fail if we have not set a single asset? It seems like the only action which needs to be required to be set in genesis (and thus enabled) is the FeeChange itself? If we can't set the fee then we can't do it for any other actions either?

joroshiba · 2024-10-15T23:01:37Z

crates/astria-core/src/protocol/genesis/v1alpha1.rs

+    pub sequence: SequenceFeeComponents,
+    pub transfer: TransferFeeComponents,
+    pub ics20_withdrawal: Ics20WithdrawalFeeComponents,
+    pub init_bridge_account: InitBridgeAccountFeeComponents,
+    pub bridge_lock: BridgeLockFeeComponents,
+    pub bridge_unlock: BridgeUnlockFeeComponents,
+    pub bridge_sudo_change: BridgeSudoChangeFeeComponents,
+    pub ibc_relay: IbcRelayFeeComponents,
+    pub validator_update: ValidatorUpdateFeeComponents,
+    pub fee_asset_change: FeeAssetChangeFeeComponents,
+    pub fee_change: FeeChangeFeeComponents,
+    pub ibc_relayer_change: IbcRelayerChangeFeeComponents,
+    pub sudo_address_change: SudoAddressChangeFeeComponents,
+    pub ibc_sudo_change: IbcSudoChangeFeeComponents,


Should most of these be Option? It seems the only which stricly must be set is fee_change?

Tracking issue: #1662

joroshiba

Approval for infra, couple notes on the genesis that I'm not blocking on.

* main: feat(sequencer)!: rework all fees (#1647) fix(sequencer): fix app hash in horcrux sentries (#1646) fix(composer)!: update to work with appside mempool (#1643) fix(charts): rollup application monitoring (#1601)

## Summary Removes an unused variant of the internal storage implementation of sequencer asset component.t ## Background As part of #1647 fees were moved from the assets to the fees component. The present variant was overlooked. ## Changes - Remove a storage variant from the assets component. ## Testing Not applicable. Dead code. ## Breaking Changelist This is not a breaking change because a) the variant was never used (there was not even a theoretic way to write it after #1647) and b) no variants existed after the variant. If they had, this change would lead to a shift in the variants, causing breakage. --------- Co-authored-by: Fraser Hutchison <fraser@astria.org>

## Summary Made all fees except for `FeeChange` action optional at genesis. ## Background Tracking PR for [this comment](#1647 (comment)). Allows for more flexibility at genesis. Post #1647, an action with no fees in the state is by default deactivated until it is explicitly given fees, even if they are 0. This is to allow for actions to be added in the future without breaking consensus, but also allows for more flexibility at genesis. Now, all actions can be disabled at genesis by making the fees optional, except for `FeeChange`, which is needed in order to activate actions by giving them fees. ## Changes - Made all fees in `GenesisFees` optional (except for fee_change). ## Testing Passing all tests. ## Related Issues closes #1662

## Summary Moves the initialization of allowed assets from the overall app to the fees component. ## Background #1647 moved fee asset state handling to the new `fees` component. It was overlooked that writing allowed fee assets should also happen in its init chain logic. ## Changes - Move initialization of app init state to the fees component. ## Testing Nothing has changed in terms of how the state is represented. Tests that rely on init chain still work. ## Breaking Changelist This is not a breaking change because the state after this change looks the same.

## Summary Removes an unused variant of the internal storage implementation of sequencer asset component.t ## Background As part of astriaorg/astria#1647 fees were moved from the assets to the fees component. The present variant was overlooked. ## Changes - Remove a storage variant from the assets component. ## Testing Not applicable. Dead code. ## Breaking Changelist This is not a breaking change because a) the variant was never used (there was not even a theoretic way to write it after astriaorg/astria#1647) and b) no variants existed after the variant. If they had, this change would lead to a shift in the variants, causing breakage. --------- Co-authored-by: Fraser Hutchison <fraser@astria.org>

## Summary Made all fees except for `FeeChange` action optional at genesis. ## Background Tracking PR for [this comment](astriaorg/astria#1647 (comment)). Allows for more flexibility at genesis. Post #1647, an action with no fees in the state is by default deactivated until it is explicitly given fees, even if they are 0. This is to allow for actions to be added in the future without breaking consensus, but also allows for more flexibility at genesis. Now, all actions can be disabled at genesis by making the fees optional, except for `FeeChange`, which is needed in order to activate actions by giving them fees. ## Changes - Made all fees in `GenesisFees` optional (except for fee_change). ## Testing Passing all tests. ## Related Issues closes #1662

## Summary Moves the initialization of allowed assets from the overall app to the fees component. ## Background astriaorg/astria#1647 moved fee asset state handling to the new `fees` component. It was overlooked that writing allowed fee assets should also happen in its init chain logic. ## Changes - Move initialization of app init state to the fees component. ## Testing Nothing has changed in terms of how the state is represented. Tests that rely on init chain still work. ## Breaking Changelist This is not a breaking change because the state after this change looks the same.

## Summary Removes an unused variant of the internal storage implementation of sequencer asset component.t ## Background As part of astriaorg/astria#1647 fees were moved from the assets to the fees component. The present variant was overlooked. ## Changes - Remove a storage variant from the assets component. ## Testing Not applicable. Dead code. ## Breaking Changelist This is not a breaking change because a) the variant was never used (there was not even a theoretic way to write it after astriaorg/astria#1647) and b) no variants existed after the variant. If they had, this change would lead to a shift in the variants, causing breakage. --------- Co-authored-by: Fraser Hutchison <fraser@astria.org>

## Summary Made all fees except for `FeeChange` action optional at genesis. ## Background Tracking PR for [this comment](astriaorg/astria#1647 (comment)). Allows for more flexibility at genesis. Post #1647, an action with no fees in the state is by default deactivated until it is explicitly given fees, even if they are 0. This is to allow for actions to be added in the future without breaking consensus, but also allows for more flexibility at genesis. Now, all actions can be disabled at genesis by making the fees optional, except for `FeeChange`, which is needed in order to activate actions by giving them fees. ## Changes - Made all fees in `GenesisFees` optional (except for fee_change). ## Testing Passing all tests. ## Related Issues closes #1662

## Summary Moves the initialization of allowed assets from the overall app to the fees component. ## Background astriaorg/astria#1647 moved fee asset state handling to the new `fees` component. It was overlooked that writing allowed fee assets should also happen in its init chain logic. ## Changes - Move initialization of app init state to the fees component. ## Testing Nothing has changed in terms of how the state is represented. Tests that rely on init chain still work. ## Breaking Changelist This is not a breaking change because the state after this change looks the same.

ethanoroshiba added 13 commits September 19, 2024 10:41

rework fees

cb87de9

rustfmt

5662ba1

remove manual testing code

4db8306

rustfmt

34c3cf0

Minor improvements and verbage changes

1b5825e

Fix warning

aa2c9ff

Merge branch 'main' into ENG-718/rework_fees_again

a5363ec

Merge branch 'main' into ENG-718/rework_fees_again

2d97105

remove allow attributes

f55673c

Merge branch 'main' into ENG-718/rework_fees_again

e84060e

rustfmt

747c6be

requested changes

d69191a

rework all fees

4c44f11

github-actions bot added proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate labels Oct 8, 2024

ethanoroshiba changed the base branch from main to ENG-718/rework_fees_again October 8, 2024 22:32

ethanoroshiba added 2 commits October 9, 2024 01:21

Merge branch 'main' into ENG-718/rework_fees_again

d8f2a2f

Merge branch 'ENG-718/rework_fees_again' into eoroshiba/fee_finalization

ddad564

ethanoroshiba temporarily deployed to BUF October 8, 2024 23:42 — with GitHub Actions Inactive

couple minor fixes. todo: make state reads/writes type safe

4e35ad8

ethanoroshiba temporarily deployed to BUF October 8, 2024 23:49 — with GitHub Actions Inactive

make reads/writes type safe

864bf0c

ethanoroshiba temporarily deployed to BUF October 9, 2024 09:06 — with GitHub Actions Inactive

joroshiba mentioned this pull request Oct 9, 2024

feat(cli): feeChange commands #1622

Closed

more changes

a7c3656

ethanoroshiba temporarily deployed to BUF October 9, 2024 15:06 — with GitHub Actions Inactive

flattened fee components inner into all the fee types

cbb32c4

SuperFluffy temporarily deployed to BUF October 9, 2024 15:50 — with GitHub Actions Inactive

changes to sequencer after previous commit

0be4bf2

ethanoroshiba temporarily deployed to BUF October 9, 2024 17:13 — with GitHub Actions Inactive

revert devtag

e454ccc

ethanoroshiba temporarily deployed to BUF October 15, 2024 22:34 — with GitHub Actions Inactive

joroshiba reviewed Oct 15, 2024

View reviewed changes

charts/sequencer/files/cometbft/config/genesis.json Show resolved Hide resolved

requested change

661f116

ethanoroshiba temporarily deployed to BUF October 15, 2024 22:45 — with GitHub Actions Inactive

joroshiba reviewed Oct 15, 2024

View reviewed changes

charts/sequencer/files/cometbft/config/genesis.json Outdated Show resolved Hide resolved

Suggestion from joroshiba

e5e01f7

Co-authored-by: Jordan Oroshiba <jordan@astria.org>

ethanoroshiba temporarily deployed to BUF October 15, 2024 22:51 — with GitHub Actions Inactive

joroshiba reviewed Oct 15, 2024

View reviewed changes

joroshiba approved these changes Oct 15, 2024

View reviewed changes

ethanoroshiba added this pull request to the merge queue Oct 15, 2024

Merged via the queue into main with commit b677ce9 Oct 15, 2024

ethanoroshiba deleted the eoroshiba/fee_finalization branch October 15, 2024 23:30

This was referenced Oct 16, 2024

sequencer: make fees optional upon genesis #1662

Closed

sequencer: add more docs/comments to the fees component #1663

Closed

feat(sequencer): make fees optional at genesis #1664

Merged

SuperFluffy mentioned this pull request Oct 21, 2024

refactor(sequencer): remove unused asset storage variant #1704

Merged

SuperFluffy mentioned this pull request Oct 23, 2024

chore(sequencer): init allowed assets in fees component #1730

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(sequencer)!: rework all fees#1647

feat(sequencer)!: rework all fees#1647
ethanoroshiba merged 40 commits intomainfrom
eoroshiba/fee_finalization

ethanoroshiba commented Oct 8, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

joroshiba Oct 15, 2024

Uh oh!

joroshiba Oct 15, 2024

Uh oh!

ethanoroshiba Oct 16, 2024

Uh oh!

joroshiba left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ethanoroshiba commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background

Changes

Testing

Breaking Changelist

Related Issues

Uh oh!

Uh oh!

Uh oh!

joroshiba Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

joroshiba Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

ethanoroshiba Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

joroshiba left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ethanoroshiba commented Oct 8, 2024 •

edited

Loading