refactor(sequencer): store fees for actions in app state #1017

noot · 2024-04-26T20:56:59Z

Summary

store fees for actions in app state, fees are of the form base_fee + byte_cost_multiplier * byte_size.

for txs that don't write to DA, they have an implicit byte_size of zero, so they only have a base fee.

for txs that write to DA, they have a byte_cost_multiplier which is multiplied by the number of bytes written to DA for fee calculation.

Background

this is so eventually we can change fees via a tx.

Changes

store fees for actions in app state

Testing

unit tests

Related Issues

part of #1003

SuperFluffy

Requesting changes due to the use of std::mem::size_of. The rest looks fine to me, but I would like have saturating arithmetic instead of bare + and *. Also, unit tests for the following would be great:

all fee calculations
the byte length of Deposit
writing/reading from rocksdb

Orthogonal to this PR I have another concern:

We have no mechanism that ensure that the keys in hardcoded in the various state_ext modules/StateExt immplementations don't clash (scenario: somebody wants to store a new object in the DB 6 months from now). I think it might make sense to list all of them in a central locations in a first step (astria_sequencer::database_keys maybe?). In a second step I want to think about how we can ensure at compile time that they don't clash.

SuperFluffy · 2024-04-29T09:14:42Z

crates/astria-sequencer/src/transaction/mod.rs

@@ -95,11 +104,12 @@ pub(crate) async fn check_balance_for_total_fees<S: StateReadExt + 'static>(
                    .or_insert(act.amount);
                fees_by_asset
                    .entry(act.fee_asset_id)
-                    .and_modify(|amt| *amt += TRANSFER_FEE)
-                    .or_insert(TRANSFER_FEE);
+                    .and_modify(|amt| *amt += transfer_fee)


Suggested change

.and_modify(|amt| *amt += transfer_fee)

.and_modify(|amt| *amt = amt.saturating_add(transfer_fee))

SuperFluffy · 2024-04-29T09:14:57Z

crates/astria-sequencer/src/transaction/mod.rs

@@ -113,14 +123,14 @@ pub(crate) async fn check_balance_for_total_fees<S: StateReadExt + 'static>(
                    .or_insert(act.amount());
                fees_by_asset
                    .entry(*act.fee_asset_id())
-                    .and_modify(|amt| *amt += ICS20_WITHDRAWAL_FEE)
-                    .or_insert(ICS20_WITHDRAWAL_FEE);
+                    .and_modify(|amt| *amt += ics20_withdrawal_fee)


Suggested change

.and_modify(|amt| *amt += ics20_withdrawal_fee)

.and_modify(|amt| *amt = amt.saturating_add(ics20_withdrawal_fee))

SuperFluffy · 2024-04-29T09:15:26Z

crates/astria-sequencer/src/transaction/mod.rs

            }
            Action::InitBridgeAccount(act) => {
                fees_by_asset
                    .entry(act.fee_asset_id)
-                    .and_modify(|amt| *amt += INIT_BRIDGE_ACCOUNT_FEE)
-                    .or_insert(INIT_BRIDGE_ACCOUNT_FEE);
+                    .and_modify(|amt| *amt += init_bridge_account_fee)


Suggested change

.and_modify(|amt| *amt += init_bridge_account_fee)

.and_modify(|amt| *amt = amt.saturating_add(init_bridge_account_fee))

SuperFluffy · 2024-04-29T09:15:36Z

crates/astria-sequencer/src/transaction/mod.rs

@@ -129,8 +139,8 @@ pub(crate) async fn check_balance_for_total_fees<S: StateReadExt + 'static>(
                    .or_insert(act.amount);
                fees_by_asset
                    .entry(act.fee_asset_id)
-                    .and_modify(|amt| *amt += TRANSFER_FEE)
-                    .or_insert(TRANSFER_FEE);
+                    .and_modify(|amt| *amt += transfer_fee)


Suggested change

.and_modify(|amt| *amt += transfer_fee)

.and_modify(|amt| *amt = amt.saturating_add(transfer_fee))

SuperFluffy · 2024-04-29T09:17:15Z

crates/astria-sequencer/src/sequence/state_ext.rs

Add simple unit tests to this file in line with the other unit tests that were added over the last few weeks?

SuperFluffy · 2024-04-29T09:20:06Z

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

+            .get_bridge_lock_byte_cost_multiplier()
+            .await
+            .context("failed to get byte cost multiplier")?;
+        let fee = byte_cost_multiplier * DEPOSIT_BYTE_LEN + transfer_fee;


Saturating arithmetic:

let fee = byte_cost_multiplier.saturating_mul(DEPOSIT_BYTE_LEN).saturating_add(transfer_fee);

Also, please add a unit tests for this calculation.

SuperFluffy · 2024-04-29T09:31:48Z

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

@@ -26,6 +32,8 @@ use crate::{
    transaction::action_handler::ActionHandler,
 };

+pub(crate) const DEPOSIT_BYTE_LEN: u128 = std::mem::size_of::<Deposit>() as u128;


Don't use the Rust in-memory representation of types if you want deterministic results. There are no guarantees on ordering of fields or alignment paddding. You can get different results depending on Rust compiler or even architecture.

You can use #[repr(C)] to get a defined layout (and hence deterministic size), but even that is tricky if somebody changes the representation of one of Deposit's fields.

For Deposit in partciular I notice that it contains destination_chain_address: String, the memory size of which is is 24 bytes on a 64bit host (64 bit pointer, 64 bit capacity and length), with the actual memory used being unbounded.

good point on the destination_chain_address, i'll change this to calculate the byte size at runtime (similar to sequence actions)

SuperFluffy · 2024-04-29T09:32:33Z

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

+            .get_bridge_lock_byte_cost_multiplier()
+            .await
+            .context("failed to get byte cost multiplier")?;
+        let fee = byte_cost_multiplier * DEPOSIT_BYTE_LEN;


Suggested change

let fee = byte_cost_multiplier * DEPOSIT_BYTE_LEN;

let fee = byte_cost_multiplier.saturating_mul(DEPOSIT_BYTE_LEN);

And unit test please.

SuperFluffy · 2024-04-29T09:33:11Z

crates/astria-sequencer/src/bridge/bridge_lock_action.rs

+            .decrease_balance(from, self.fee_asset_id, fee)
+            .await
+            .context("failed to deduct fee from account balance")?;
+
        let rollup_id = state
            .get_bridge_account_rollup_id(&self.to)
            .await?


Just noticed the bare question mark (outside of the PR, but can you context please?)

…r-fees

SuperFluffy

Looking good. Just once place I found that should get error context.

SuperFluffy · 2024-04-30T09:11:22Z

crates/astria-sequencer/src/app.rs

        assert_eq!(
            app.state
                .get_account_balance(alice_address, native_asset)
                .await
                .unwrap(),
-            10u128.pow(19) - (value + TRANSFER_FEE),
+            10u128.pow(19) - (value + transfer_fee),


We should really move this tests module into src/app/tests.rs. I wanted to comment several times now how I want saturating arithmetic. :-)

yeah i was planning to move it next, it's getting way too long lol

SuperFluffy · 2024-04-30T09:12:19Z

crates/astria-sequencer/src/bridge/init_bridge_account_action.rs

@@ -41,6 +38,8 @@ impl ActionHandler for InitBridgeAccountAction {
            "invalid fee asset",
        );

+        let fee = state.get_init_bridge_account_base_fee().await?;


…r-fees

Reverts #1032 #1017 was incorrectly labelled as non breaking.

noot added 6 commits April 24, 2024 16:05

remove SequencerBlock::try_from_cometbft

02582ac

fix relayer tests

850b8f1

clippy

fa18eaa

store transfer fee in db

23d3902

store fees for sequence action and bridge

e084f6a

merge

c432b7c

noot requested a review from a team as a code owner April 26, 2024 20:56

noot requested a review from SuperFluffy April 26, 2024 20:57

github-actions bot added the sequencer pertaining to the astria-sequencer crate label Apr 26, 2024

noot temporarily deployed to BUF April 26, 2024 20:57 — with GitHub Actions Inactive

SuperFluffy requested changes Apr 29, 2024

View reviewed changes

noot added 4 commits April 29, 2024 12:15

Merge branch 'main' of github.com:astriaorg/astria into noot/sequence…

1a9a5c7

…r-fees

fmt

aaee4a4

use saturating arithmetic, change deposit size calc to runtime

98e25d6

add unit tests for bridge lock fee calc

59415e9

noot requested review from SuperFluffy and Lilyjjo April 29, 2024 19:13

noot added 2 commits April 29, 2024 15:15

cleanup

4d1ebff

fix test

1e47bcb

SuperFluffy approved these changes Apr 30, 2024

View reviewed changes

context

14fff86

noot enabled auto-merge April 30, 2024 17:32

Merge branch 'main' of github.com:astriaorg/astria into noot/sequence…

d100f91

…r-fees

noot added this pull request to the merge queue Apr 30, 2024

Merged via the queue into main with commit 049ec61 Apr 30, 2024
36 checks passed

noot deleted the noot/sequencer-fees branch April 30, 2024 17:43

joroshiba mentioned this pull request May 2, 2024

Revert "release(sequencer): sequencer 0.11.1" #1035

Merged

joroshiba added a commit that referenced this pull request May 2, 2024

Revert "release(sequencer): sequencer 0.11.1" (#1035)

f076bf1

Reverts #1032 #1017 was incorrectly labelled as non breaking.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(sequencer): store fees for actions in app state #1017

refactor(sequencer): store fees for actions in app state #1017

noot commented Apr 26, 2024

SuperFluffy left a comment •

edited

Loading

SuperFluffy Apr 29, 2024

SuperFluffy Apr 29, 2024

SuperFluffy Apr 29, 2024

SuperFluffy Apr 29, 2024

SuperFluffy Apr 29, 2024

SuperFluffy Apr 29, 2024

SuperFluffy Apr 29, 2024

noot Apr 29, 2024

SuperFluffy Apr 29, 2024

SuperFluffy Apr 29, 2024

SuperFluffy left a comment

SuperFluffy Apr 30, 2024

noot Apr 30, 2024

SuperFluffy Apr 30, 2024

	.and_modify(\|amt\| *amt += transfer_fee)
	.and_modify(\|amt\| *amt = amt.saturating_add(transfer_fee))

	.and_modify(\|amt\| *amt += ics20_withdrawal_fee)
	.and_modify(\|amt\| *amt = amt.saturating_add(ics20_withdrawal_fee))

	.and_modify(\|amt\| *amt += init_bridge_account_fee)
	.and_modify(\|amt\| *amt = amt.saturating_add(init_bridge_account_fee))

	let fee = byte_cost_multiplier * DEPOSIT_BYTE_LEN;
	let fee = byte_cost_multiplier.saturating_mul(DEPOSIT_BYTE_LEN);

refactor(sequencer): store fees for actions in app state #1017

refactor(sequencer): store fees for actions in app state #1017

Conversation

noot commented Apr 26, 2024

Summary

Background

Changes

Testing

Related Issues

SuperFluffy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperFluffy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SuperFluffy left a comment •

edited

Loading