feat(sequencer): rework fee reporting#1426
Conversation
There was a problem hiding this comment.
This snapshot test suggests that we are changing actual state of acocunt balances with this PR, which I don't believe should be happening?
There was a problem hiding this comment.
Would the snapshot also change if all the functionality regarding block fees was taken out? That information is no longer recorded to the state (and the functionality to do so was taken out as well), instead just the account balances are increased/decreased and the fee events recorded to the state.
There was a problem hiding this comment.
Perhaps this is an issue with the test, or a misunderstanding in how non-verified state works, but I believe this test should only be affected by verified state.
There was a problem hiding this comment.
Hmm, that's very strange. My logic may be wrong somewhere down the line but for all transactions with fees I implemented tests which check the account balances on the back end after paying, so I think if something is wrong it's probably in the fee calculation itself?
There was a problem hiding this comment.
the reason this changes is because of the app_execute_transaction_with_every_action_snapshot test - it doesn't call end_block via finalize_block, just commit. it probably should have :/ previously, fees were being updated only in end_block, but now they're updated when the transaction is executed, so the state at the end of the test is different (even though the changes in this PR may not be a breaking change)
There was a problem hiding this comment.
we can check if this is breaking by updating the test to call end_block on main and this branch and seeing if it matches
| if return_payment_map { | ||
| Ok((fees_by_asset, Some(fee_payment_map))) | ||
| } else { | ||
| Ok((fees_by_asset, None)) | ||
| } |
There was a problem hiding this comment.
the fee_payment_map is generated every time no matter what return_payment_map is - remove the return_payment_map param (also from check_balance_and_get_fees) and just return the payment map everytime, the caller can decide to drop or not
There was a problem hiding this comment.
nvm, i see you are using it to populate the map in the _update_fees functions. not really a fan of this method - i would prefer if each action had two separate functions, one for updating fees_by_asset and one for updating the payment map.
There was a problem hiding this comment.
I originally had it set up the way you're describing, but changed it for a couple reasons: 1) get_and_report_tx_fees() was over the line limit, and this was a way to cut down on line count quite significantly, and 2) some of the actions require fee calculation beyond just the base transfer fees, and this way the calculation would only need to be done once within the _update_fees functions and then passed to increase_fees(). If you think it's important to have it set up the other way, I can revert this and put a clippy allow in. Thanks for taking a look over!
| fn warn_if_asset_type_not_allowed(asset: &asset::Denom, allowed_fee_assets: &[IbcPrefixed]) { | ||
| if !allowed_fee_assets.contains(&IbcPrefixed::from(asset)) { | ||
| warn!("Transaction execution will fail, asset type not allowed for fee payment: {asset}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
this should be a hard error, not just a log
There was a problem hiding this comment.
Should this be a hard error even if the transaction is not being executed? This function may be called by transaction_fee_request, in which case I had thought it might be important to differentiate between warning when get_and_report_tx_fees() is called for a fee request, versus erroring when it is called during transaction execution.
…y_action_snapshot` (#1455) ## Summary Added call to `end_block()` within the snapshot test `app_execute_transaction_with_every_action_snapshot` ## Background The snapshot test previously did not account for fees being paid out, as this is done currently in `end_block()`. As such, changes to fee handling may break the snapshot test even though they are not breaking changes (such as in #1426). ## Changes - Added `endblock()` call to `app_execute_transaction_with_every_action_snapshot` - Regenerated snapshot ## Testing Passing all other tests ## Related Issues closes #1454
|
I feel this change breaks with the current architecture of sequencer too much and makes it more difficult to reason about. I don't want to merge this in as is. My main issue is that I can no longer understand the state transitions that a single action will cause just by looking at its I would rather there be one source of truth. But to take a step back, this PR attempts to fix several things at the same time which I think could (and should be) handled in separate PRs (taking these from the PR description):
I don't immediately see anything bad about this. The execution of an action should result in 2 state changes: 1. the decrease of a balance for a given asset on the payer side, and 2. an increase of a balance for a given asset on the payee side (or in this case, the receiver of a payment). However, since the cost of an action should be defined by its contents, I think With that said, I can see how the actual implementation of how payments are made and reported could be changed to become clearer. But I think that can be done with a lighter touch then what this does. For example, by making use of the transaction context ID and action index that was recently added in a different PR.
I agree that this is problematic and I stumbled over this myself. However, this particular problem should be addressed in a different PR.
Can you point to that line? It does sound repetitive (and should indeed be handled on a per-action level). |
|
Closing due to offline discussion. Will open new PR with a design which breaks less from the current sequencer logic. |
Summary
Reworked fee reporting based on new design. The design has changed slightly from its original form, since the current fee calculation functions were also used on read-only states, which presented a challenge we hadn't anticipated.
Background
Fee reporting was previously handled in a very roundabout way, with a few issues:
BridgeLockandBridgeUnlockactions both instantiated and executedTransferActions within theircheck_and_execute()methods. Because fees are recorded based on the name of the action type, this would incorrectly report associated lock and unlock fees as transfer fees.check_balance_for_total_fees_and_transfers(). Hence, we were calculating the cost of fees more than once, which should not be the case.Changes
astria-sequencer::transaction::fees, which handles all fee calculation and reporting.get_and_report_fees(), which calculates all fees for a given transaction and returns afees_by_assethash map and an optionalfee_payment_mapfor processing fee payments. This allows dual functionality for fees to be checked based on a state that only implementsStateReadand notStateWrite.pay_fees(), which intakes a state which implementsStateWritealong with afee_payment_maphash map. This function pays all fees and records all fee events to the state.actionIndexattribute totx.feesevents so that outside observers can see which action actually correlates to the fee event and where it is in the block.Testing
Related Issues
closes #1369
closes #1145
closes #1382