feat(sequencer): implement transaction fee query #1196
Conversation
| @@ -37,6 +38,7 @@ impl AbciErrorCode { | |||
| 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(), | |||
There was a problem hiding this comment.
Maybe "the request body" or "request payload"?
| enum TransactionFeeResponseErrorKind { | ||
| #[error("`fee` field is unset")] | ||
| UnsetFee, | ||
| #[error("failed to parse asset denom")] |
There was a problem hiding this comment.
Maybe add the field, i.e. failed to parse ok asset listed in the ``.assets`` field
| #[error("`fee` field is unset")] | ||
| UnsetFee, | ||
| #[error("failed to parse asset denom")] | ||
| Asset(#[from] asset::ParseDenomError), |
There was a problem hiding this comment.
Better just #[source] because I think we don't use the from conversion ever
| state: &S, | ||
| ) -> anyhow::Result<()> { | ||
| ) -> anyhow::Result<HashMap<asset::IbcPrefixed, u128>> { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
yes - can make an issue
| } | ||
| } | ||
|
|
||
| for (asset, total_fee) in cost_by_asset { |
There was a problem hiding this comment.
Could fire these up concurrently too
|
|
||
| // Response to a transaction fee ABCI query. | ||
| message TransactionFeeResponse { | ||
| uint64 height = 2; |
There was a problem hiding this comment.
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?
| @@ -164,16 +205,10 @@ pub(crate) async fn check_balance_for_total_fees<S: StateReadExt + 'static>( | |||
| } | |||
|
|
|||
| fn transfer_update_fees( | |||
There was a problem hiding this comment.
Why is this body updated? Probably missing something, but from a cursory glance this looks orthogonal to the new functionality?
There was a problem hiding this comment.
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
| @@ -197,16 +232,10 @@ async fn sequence_update_fees<S: StateReadExt>( | |||
| } | |||
|
|
|||
| fn ics20_withdrawal_updates_fees( | |||
| asset: &asset::Denom, | |||
There was a problem hiding this comment.
Same here - is this functionality no longer necessary or wrong?
| state: &S, | ||
| bridge_address: Address, | ||
| amount: u128, | ||
| fn bridge_unlock_update_fees( |
There was a problem hiding this comment.
Same here - why all that change?
| @@ -57,17 +57,15 @@ pub(crate) async fn check_balance_mempool<S: StateReadExt + 'static>( | |||
| state: &S, | |||
| ) -> anyhow::Result<()> { | |||
| 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?; | |||
| from: Address, | ||
| state: &S, | ||
| ) -> anyhow::Result<()> { | ||
| let mut cost_by_asset = get_fees_for_transaction(tx, state).await?; |
| @@ -198,7 +199,7 @@ impl ActionHandler for UnsignedTransaction { | |||
| ensure!(curr_nonce == self.nonce(), InvalidNonce(self.nonce())); | |||
|
|
|||
| // 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
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
Testing
unit tests
Related Issues
closes #1071