-
Notifications
You must be signed in to change notification settings - Fork 76
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
Conversation
.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) |
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.
Should bridge sudo change action be deducting the fee from the signer as opposed to the bridge address?
52ed4fe
to
e84060e
Compare
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.
I have two notes:
- I don't see an advantage in changing
AppHandler
itself. The same thing can be done by creating a function and it keeps theAppHandler
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)?;
}
- I think it would be nice to make a
fees
component and move all state accesses there.
} | ||
|
||
#[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
pub(crate) struct Fee { |
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.
I'd like to make the fields of this type private. All the impl FeeHandler for {Action}
can/should subsequently be moved here.
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.
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); |
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.
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.
52cfacc
to
b86bf58
Compare
Summary
Added new
fees
component to hold all fee handling, which implementscheck_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 toVec<Event>
, but I think this would be best done in a followup, since deposit ABCI events are also recorded viaapply()
.Changes
fees
with traitFeeHandler
and structFee
.check_execute_and_pay_fees()
, which first callscheck_and_execute()
and then callsFeeHandler::check_and_pay_fees()
. Changed all actions to use this new function in place ofcheck_and_execute()
check_and_execute()
tocheck_and_pay_fees()
.Vec<Fee>
. This allows forend_block()
to still pay out all fees at the end of the block, and create fee ABCI events at that time. Moved these tofees::state_ext.rs
.Testing
All previous tests passing.
Breaking Changelist
app:state_ext::storage_keys_are_unchanged
snapshotRelated Issues
closes #1369
closes #1145