Skip to content
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

feat(sequencer): rework fee handling #1526

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

ethanoroshiba
Copy link
Contributor

@ethanoroshiba ethanoroshiba commented Sep 19, 2024

Summary

Added new fees component to hold all fee handling, which implements check_and_pay_fees() for each action.

Background

This is meant to streamline and standardize fee handling in a way that keeps the same functionality, but makes it more difficult to introduce bugs in fee handling in the future. Offline discussion suggested changing the return type of execute_transaction() to be our own type as opposed to Vec<Event>, but I think this would be best done in a followup, since deposit ABCI events are also recorded via apply().

Changes

  • Added new component fees with trait FeeHandler and struct Fee.
  • Created new function check_execute_and_pay_fees(), which first calls check_and_execute() and then calls FeeHandler::check_and_pay_fees(). Changed all actions to use this new function in place of check_and_execute()
  • Moved all actions' fee handling from check_and_execute() to check_and_pay_fees().
  • Changed block fee read/writes on the state delta to use the ephemeral cache with Vec<Fee>. This allows for end_block() to still pay out all fees at the end of the block, and create fee ABCI events at that time. Moved these to fees::state_ext.rs.

Testing

All previous tests passing.

Breaking Changelist

  • Changed block fees storage key, as it does not need to incorporate the asset name into the storage key anymore. This breaks the app:state_ext::storage_keys_are_unchanged snapshot

Related Issues

closes #1369
closes #1145

@github-actions github-actions bot added the sequencer pertaining to the astria-sequencer crate label Sep 19, 2024
.wrap_err("failed to add to block fees")?;
state
.decrease_balance(self.bridge_address, &self.fee_asset, fee)
.decrease_balance(from, &self.fee_asset, fee)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should bridge sudo change action be deducting the fee from the signer as opposed to the bridge address?

@ethanoroshiba ethanoroshiba marked this pull request as ready for review September 19, 2024 18:06
@ethanoroshiba ethanoroshiba requested a review from a team as a code owner October 1, 2024 19:51
@github-actions github-actions bot added ci issues that are related to ci and github workflows conductor pertaining to the astria-conductor crate composer pertaining to composer cd labels Oct 1, 2024
@ethanoroshiba ethanoroshiba removed ci issues that are related to ci and github workflows conductor pertaining to the astria-conductor crate composer pertaining to composer cd labels Oct 1, 2024
@ethanoroshiba ethanoroshiba removed the request for review from a team October 1, 2024 20:17
Copy link
Member

@SuperFluffy SuperFluffy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two notes:

  1. I don't see an advantage in changing AppHandler itself. The same thing can be done by creating a function and it keeps the AppHandler clean.
fn check_execute_and_pay_fees<T: AppHandler + FeeHandler, S: StateWrite>(action: &T, mut state: S) -> Result<()> {
    action.check_and_execute(state)?;
    action.pay_fees(state)?;
}
  1. I think it would be nice to make a fees component and move all state accesses there.

crates/astria-sequencer/src/app/action_handler.rs Outdated Show resolved Hide resolved
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub(crate) struct Fee {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to make the fields of this type private. All the impl FeeHandler for {Action} can/should subsequently be moved here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made all fields private but added accessors for asset and amount so that fees can be paid out during end_block

) -> Result<()>
where
TAsset: Sync + std::fmt::Display,
asset::IbcPrefixed: From<&'a TAsset>,
{
let tx_fee_event = construct_tx_fee_event(asset, amount, action_type);
let block_fees_key = block_fees_key(asset);
let mut current_fees: Option<Vec<Fee>> = self.object_get(BLOCK_FEES_PREFIX);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we start making use of this API, it has some obvious warts: we should be able to obtain a mutable reference to the object (if it exists), so that we don't need to clone it out and back into the ephemeral store.

CC @Fraser999 for the rework of the state delta construction.

crates/astria-sequencer/src/assets/state_ext.rs Outdated Show resolved Hide resolved
crates/astria-sequencer/src/assets/state_ext.rs Outdated Show resolved Hide resolved
crates/astria-sequencer/src/app/fee_handler.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sequencer pertaining to the astria-sequencer crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rework fee reporting sequencer: cleanup/refactor of action fee checks
2 participants