feat(sequencer): implement transaction fee query by noot · Pull Request #1196 · astriaorg/astria

noot · 2024-06-19T16:37:11Z

Summary

implement transaction fee ABCI query method. calculates the fee for the transaction based on the current state.

Background

useful for UX and fee estimation.

Changes

implement transaction fee ABCI query method which calculates the fee for the transaction based on the current state
update sequencer-client respectively

Testing

unit tests

Related Issues

closes #1071

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

SuperFluffy

I left comments looking to understand why various code was updated (updating fees etc), even the PR just aims to read (not write)?

SuperFluffy · 2024-07-02T10:00:33Z

crates/astria-core/src/protocol/abci.rs

            8 => "the requested value was not found".into(),
            9 => "the transaction expired in the app's mempool".into(),
            10 => "the transaction failed to execute in prepare_proposal()".into(),
+            11 => "the request was malformed".into(),


Maybe "the request body" or "request payload"?

SuperFluffy · 2024-07-02T10:02:49Z

crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs

+enum TransactionFeeResponseErrorKind {
+    #[error("`fee` field is unset")]
+    UnsetFee,
+    #[error("failed to parse asset denom")]


Maybe add the field, i.e. failed to parse ok asset listed in the ``.assets`` field

SuperFluffy · 2024-07-02T10:04:07Z

crates/astria-core/src/protocol/transaction/v1alpha1/mod.rs

+    #[error("`fee` field is unset")]
+    UnsetFee,
+    #[error("failed to parse asset denom")]
+    Asset(#[from] asset::ParseDenomError),


Better just #[source] because I think we don't use the from conversion ever

SuperFluffy · 2024-07-02T10:05:47Z

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

-    from: Address,
    state: &S,
-) -> anyhow::Result<()> {
+) -> anyhow::Result<HashMap<asset::IbcPrefixed, u128>> {


Fine for doing this in a follow-up: cant we request all the data bits in this method concurrently instead of one after the other?

yes - can make an issue

SuperFluffy · 2024-07-02T10:07:11Z

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

+        }
+    }
+
+    for (asset, total_fee) in cost_by_asset {


Could fire these up concurrently too

SuperFluffy · 2024-07-02T10:07:49Z

proto/protocolapis/astria/protocol/transactions/v1alpha1/types.proto

+
+// Response to a transaction fee ABCI query.
+message TransactionFeeResponse {
+  uint64 height = 2;


Index from 1?

we should probably change all of the response types if we want to index from 1, maybe we can do that when we move them to their own package?

SuperFluffy · 2024-07-02T10:09:21Z

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

@@ -164,16 +205,10 @@ pub(crate) async fn check_balance_for_total_fees<S: StateReadExt + 'static>(
 }

 fn transfer_update_fees(


Why is this body updated? Probably missing something, but from a cursory glance this looks orthogonal to the new functionality?

the function checking "fees" before was actually checking the total value used by the transaction, so it was summing fees and value transfers. however, the fees endpoint wants to return only fees used by the tx, so i had to separate these

SuperFluffy · 2024-07-02T10:10:29Z

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

 }

 fn ics20_withdrawal_updates_fees(
-    asset: &asset::Denom,


Same here - is this functionality no longer necessary or wrong?

same as above

SuperFluffy · 2024-07-02T10:10:54Z

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

-    state: &S,
-    bridge_address: Address,
-    amount: u128,
+fn bridge_unlock_update_fees(


Same here - why all that change?

same as above

SuperFluffy

approve for API.

few comments on missing error context

SuperFluffy · 2024-07-04T19:30:45Z

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

    let signer_address = crate::address::base_prefixed(tx.verification_key().address_bytes());
-    check_balance_for_total_fees(tx.unsigned_transaction(), signer_address, state).await?;
+    check_balance_for_total_fees_and_transfers(tx.unsigned_transaction(), signer_address, state)
+        .await?;


SuperFluffy · 2024-07-04T19:30:55Z

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

+    from: Address,
+    state: &S,
+) -> anyhow::Result<()> {
+    let mut cost_by_asset = get_fees_for_transaction(tx, state).await?;


SuperFluffy · 2024-07-04T19:31:44Z

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


        // Should have enough balance to cover all actions.
-        check_balance_for_total_fees(self, from, state).await?;
+        check_balance_for_total_fees_and_transfers(self, from, state).await?;


* main: (27 commits) refactor(sequencer): fix prepare proposal metrics (#1211) refactor(bridge-withdrawer): move generated contract bindings to crate (#1237) fix(sequencer) fix wrong metric and remove unused metric (#1240) feat(sequencer): implement transaction fee query (#1196) chore(cli)!: remove unmaintained rollup subcommand (#1235) release(sequencer): 0.14.1 patch release (#1233) feat(sequencer-utils): generate example genesis state (#1224) feat(sequencer): implement abci query for bridge account info (#1189) feat(charts): bridge-withdrawer, smoke test, various chart improvements (#1141) chore(charts): update for new geth update (#1226) chore(chart)!: dusk-8 chart version updates (#1223) release(conductor): fix conductor release version (#1222) release: dusk-8 versions (#1219) fix(core): revert `From` ed25519_consensus types for crypto mod (#1220) Refactor(chart, sequencer): restructure sequencer chart, adjust configs (#1193) refactor(withdrawer): read from block subscription stream and get events on each block (#1207) feat(core): implement with verification key for address builder and crypto improvements (#1218) feat(proto, sequencer)!: use full IBC ICS20 denoms instead of IDs (#1209) chore(chart): update evm chart for new prefix field (#1214) chore: bump penumbra deps (#1216) ...

## Summary implement transaction fee ABCI query method. calculates the fee for the transaction based on the current state. ## Background useful for UX and fee estimation. ## Changes - implement transaction fee ABCI query method which calculates the fee for the transaction based on the current state - update sequencer-client respectively ## Testing unit tests ## Related Issues closes #1071

noot added 2 commits June 18, 2024 17:29

implement transaction fee query

15d1a00

add get_transaction_fee to client

bf4dacb

noot requested review from a team as code owners June 19, 2024 16:37

noot requested a review from SuperFluffy June 19, 2024 16:37

github-actions bot added proto pertaining to the Astria Protobuf spec sequencer pertaining to the astria-sequencer crate labels Jun 19, 2024

noot temporarily deployed to BUF June 19, 2024 16:37 — with GitHub Actions Inactive

clippy

3d34fa0

noot temporarily deployed to BUF June 19, 2024 16:46 — with GitHub Actions Inactive

Fraser999 reviewed Jun 21, 2024

View reviewed changes

merge w main

1794379

noot temporarily deployed to BUF June 26, 2024 20:37 — with GitHub Actions Inactive

noot added 2 commits June 26, 2024 16:39

address some comments

3de23cc

address comments

6cf46f2

noot temporarily deployed to BUF June 26, 2024 21:05 — with GitHub Actions Inactive

noot requested a review from Fraser999 June 27, 2024 16:48

merge w main

84c609c

noot temporarily deployed to BUF July 1, 2024 21:21 — with GitHub Actions Inactive

Fraser999 approved these changes Jul 2, 2024

View reviewed changes

SuperFluffy reviewed Jul 2, 2024

View reviewed changes

address comments and merge w main

56d8308

noot temporarily deployed to BUF July 4, 2024 16:42 — with GitHub Actions Inactive

fmt

b0c45d0

noot temporarily deployed to BUF July 4, 2024 16:48 — with GitHub Actions Inactive

noot requested a review from SuperFluffy July 4, 2024 16:53

SuperFluffy approved these changes Jul 4, 2024

View reviewed changes

context

5ab1478

noot enabled auto-merge July 5, 2024 17:10

noot temporarily deployed to BUF July 5, 2024 17:10 — with GitHub Actions Inactive

noot mentioned this pull request Jul 5, 2024

sequencer: concurrency retrieve balances for each asset in check_balance_for_total_fees_and_transfers #1239

Closed

fix unit test

84d05db

noot temporarily deployed to BUF July 5, 2024 18:06 — with GitHub Actions Inactive

noot added this pull request to the merge queue Jul 5, 2024

Merged via the queue into main with commit fc4e76b Jul 5, 2024

noot deleted the noot/estimate-fee branch July 5, 2024 18:23

SuperFluffy mentioned this pull request Oct 25, 2024

Add unit tests for queries missing tests #1741

Closed

		@@ -164,16 +205,10 @@ pub(crate) async fn check_balance_for_total_fees<S: StateReadExt + 'static>(
		}

		fn transfer_update_fees(

Conversation

noot commented Jun 19, 2024

Summary

Background

Changes

Testing

Related Issues

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SuperFluffy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperFluffy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants