Conversation
ethanoroshiba
requested review from
a team,
SuperFluffy,
joroshiba and
noot
as code owners
## Summary Implemented the `Protobuf` trait for all TX `Action`s and variants. ## Background `Action` and variants are refined types from generated Protobuf code, but did not implement the `Protobuf` trait. This change is to standardize the types `Error` and `Raw` across `Action`, as well as the methods `to_raw()`, `into_raw()`, `try_from_raw()`, and `try_from_raw_ref()`. ## Changes - Implemented `Protobuf` trait, types, and necessary functions for `Action` and all of its variants. - Deleted redundant functions which took ownership, but kept those which prioritized cost over their `ref` counterparts. ## Testing Passing all tests. ## Related Issues closes #1307 --------- Co-authored-by: Fraser Hutchison <190532+Fraser999@users.noreply.github.com>
ethanoroshiba
force-pushed
the
ENG-680/correct_block_fees
branch
from
6ed81d1 to
1cd5609
Compare
There was a problem hiding this comment.
I'd like to take this PR into a different direction:
Instead of implementing fee-payments on a per-action basis, I would fee payments to happen at a higher level in <SignedTransaction as ActionHandler>::check_and_execute.
I am thinking of something like this:
- define a
trait ActionFeewith a trait methodActionFee::action_fee - implement this for every action and the
enum Actionscollection. - define a
calculate_feesmethod onSignedTransaction(note that this is a foreign type, so you might either need to make a free-standing function or create a trait just for that);calculate_feesjust runs over all actions and sums up their fees. - pay the fee right before
SignedTransaction::check_and_executestarts processing the individual transactions.
I know that fee calculations right now require access to the state. Ideally the fee calculations on an individual action can do without access to state.
EDIT: Having had a second look at how fees are calculated, we use a base fee (that's fine) but across potentially many different fee_assets (not fine).
IMO this makes my suggestion above untenable, so that fees indeed must be paid on a per-action basis.
Note that the reason why this cannot be done prior to execution is the same as what required to fix #1318: fees cannot be paid before execution begins. Following scenario:
- action A adds a new fee asset
- action B adds funds to an account for that fee asset
- action C uses the fee asset for that account to pay for the transaction.
SuperFluffy
dismissed
their stale review
Suggestion made no sense. Wait to review until 1318 is merged (or wait for other reviewers)
| assert_eq!(total_block_fees, 1); | ||
| } | ||
|
|
||
| // TODO: Add test to ensure correct block fees for ICS20 withdrawal |
There was a problem hiding this comment.
will you add that in this PR?
There was a problem hiding this comment.
I attempted to do so but ran into a lot of difficulty setting up the IBC channel/ports, which is required for check_and_execute(). It looks like Janis ran into the same issue in #1345 because Penumbra doesn't make the StateWriteExt traits public. I may be missing something, though. If you have any suggestions it would be greatly appreciated! Also, thanks for the quick review and welcome back!
There was a problem hiding this comment.
Yeah, gotta make a PR against penumbra.
There was a problem hiding this comment.
If we are leaving a TODO in after merge, create a follow up issue and link in code please :) ie
// TODO(https://github.com/astriaorg/astria/issues/<issue num>): add test for correct block fees
Co-authored-by: noot <36753753+noot@users.noreply.github.com>
crates/astria-sequencer/src/bridge/bridge_sudo_change_action.rs
Outdated
Show resolved
Hide resolved
crates/astria-sequencer/src/bridge/init_bridge_account_action.rs
Outdated
Show resolved
Hide resolved
crates/astria-sequencer/src/bridge/init_bridge_account_action.rs
Outdated
Show resolved
Hide resolved
SuperFluffy
left a comment
SuperFluffy
left a comment
There was a problem hiding this comment.
Looks great. We should restructure the tests so we don't end up with a plethora of tests_* but rather create a module structure. But that can be done in a followup.
* main: refactor(core, proto)!: define app genesis state in proto (#1346) fix(sequencer): bump penumbra dep to fix ibc state access bug (#1389) feat(conductor)!: support disabled celestia auth (#1372) fix(sequencer)!: fix block fees (#1343) perf(sequencer): add benchmark for prepare_proposal (ENG-660) (#1337) fix(proto): fix import name mismatch (#1380) fix(ci): enable bridge withdrawer building with tag (#1374) feat(sequencer): rewrite memool to have per-account transaction storage and maintenance (#1323) refactor(core, sequencer)!: require that bridge unlock address always be set (#1339) fix(sequencer)!: take funds from bridge in ics20 withdrawals (#1344) fix(sequencer)!: fix TOCTOU issues by merging check and execution (#1332) fix: abci error code (#1280) refactor(core): shorten `try_from_block_info_and_data()` (#1371) fix(relayer): change `reqwest` for `isahc` in relayer blackbox tests (ENG-699) (#1366) fix(conductor): update for celestia-node v0.15.0 (#1367) Chore: Upgrade celestia-node to v0.14.1 (#1360) chore(charts): fix charts production templates (#1359) chore(core, proto): migrate byte slices from Vec to Bytes (#1319)
4 participants
Summary
Corrected block fee reporting so that proposer is paid the correct fees.
Background
Previously, only
TransferActionandSequenceActioncalledget_and_increase_block_fees(), the method for increasing the running fees for the block. Inend_block, this was used for calculating how much to increase the proposer's balance meaning that they were only paid the fees forTransferandSequenceactions.Changes
get_and_increase_block_fees()in all actions which charge fees.Testing
Added unit tests for each action, with the exception of
Ics20Withdrawal. This test should be implemented, but will require a more in-depth configuration.Breaking Changelist
Related Issues
closes #1333