feat: add fee token to TransactionOption and TxnConfig
#2650
Conversation
glihm
changed the title
TransactionOption and TxnConfig
|
@kariy I didn't add the auto-check of the balance. We could add this in the PR if required, or may be a subsequent PR. |
WalkthroughOhayo, sensei! This pull request introduces several modifications across multiple files, primarily enhancing error handling and fee configuration management in transaction-related functionalities. Key changes include the transition from simple type conversions to fallible conversions using Changes
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (10)
crates/dojo/utils/src/lib.rs (1)
11-11: Ohayo sensei! The wildcard export simplifies but consider maintenance implications.While using
pub use tx::*reduces boilerplate, it may make the public API surface less explicit. Consider maintaining a list of intended public exports in documentation to help track API changes.crates/dojo/utils/src/tx/declarer.rs (2)
97-111: Clean implementation of fee-based declaration routing, sensei!The conditional logic for v2/v3 declarations based on fee configuration is well-structured. However, consider adding a doc comment explaining the difference between v2 and v3 declarations for future maintainers.
Add a comment like:
// V2 declarations use ETH for fees while V3 declarations use STRK. // This allows flexibility in fee token selection based on user preference.
97-111: Consider enhancing observability with debug logging.Adding debug logs before the version selection would help with monitoring and debugging.
Add logging before the match statement:
+ tracing::debug!( + fee_config = ?txn_config.fee_config, + "Selecting declaration version based on fee configuration" + ); let DeclareTransactionResult { transaction_hash, class_hash } = match txn_config.fee_configcrates/sozo/ops/src/account.rs (1)
274-277: Consider adding documentation for fee multiplier logic.While the implementation is correct, it would be helpful to:
- Document why Eth configs have a configurable multiplier
- Explain the reasoning behind the default 1.1 multiplier for other configs
crates/dojo/utils/src/tx/invoker.rs (1)
58-67: Ohayo, sensei!Consider refactoring to reduce code duplication in match arms.
The match arms in lines 58-67 are nearly identical, differing only in the method called (
execute_v3vs.execute_v1). To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, consider abstracting the differing method into a variable or closure.Here's a possible refactor:
let execute = match self.txn_config.fee_config { FeeConfig::Strk(config) => { trace!(?config, "Invoking with STRK."); A::execute_v3 } FeeConfig::Eth(config) => { trace!(?config, "Invoking with ETH."); A::execute_v1 } }; let tx = execute(&self.account, vec![call]).send_with_cfg(&self.txn_config).await?;This approach assigns the differing method to a variable, reducing repetition and enhancing readability.
bin/sozo/src/commands/options/transaction.rs (3)
83-91: Consider Refactoringfee_configInitialization to Reduce DuplicationOhayo, sensei! The initialization of
fee_configin both theto_txn_actionmethod (lines 83-91) and thetry_fromimplementation (lines 126-134) is nearly identical. Refactoring this logic into a dedicated method would enhance maintainability and reduce code duplication.Apply this diff to extract the
fee_configinitialization:+impl TransactionOptions { + fn get_fee_config(&self) -> FeeConfig { + match self.fee_token { + FeeToken::Strk => FeeConfig::Strk(StrkFeeConfig { + gas: self.gas, + gas_price: self.gas_price, + }), + FeeToken::Eth => FeeConfig::Eth(EthFeeConfig { + max_fee_raw: self.max_fee_raw, + fee_estimate_multiplier: self.fee_estimate_multiplier, + }), + } + } +}Then update the
to_txn_actionandtry_frommethods:- fee_config: match self.fee_token { - // existing match arms - }, + fee_config: self.get_fee_config(),
104-110: Enhance Error Messages for Improved ClarityOhayo, sensei! The error messages when incompatible fee options are used could be clearer to guide users effectively. Consider rephrasing them to specify which options are unsupported for each fee token.
Apply this diff to improve the error messages:
- bail!( - "Gas and gas price are not supported for ETH transactions. Use \ - `--fee-token strk` instead or use `--max-fee-raw` and \ - `--fee-estimate-multiplier`." - ); + bail!( + "Options `--gas` and `--gas-price` are not supported for ETH transactions. \ + Please remove them or use `--fee-token strk` instead." + );And for STRK:
- bail!( - "Max fee raw and fee estimate multiplier are not supported for STRK \ - transactions. Use `--fee-token eth` instead or use `--gas` and \ - `--gas-price`." - ); + bail!( + "Options `--max-fee-raw` and `--fee-estimate-multiplier` are not supported for STRK transactions. \ + Please remove them or use `--fee-token eth` instead." + );Also applies to: 113-119
148-150: Alignvalue_variantsOrder with Default FeeTokenOhayo, sensei! In the implementation of
ValueEnum, thevalue_variantsmethod listsEthbeforeStrk, butStrkis the default. To maintain consistency and avoid potential confusion, it's advisable to list the default variant first.Apply this diff to reorder the variants:
fn value_variants<'a>() -> &'a [Self] { - &[Self::Eth, Self::Strk] + &[Self::Strk, Self::Eth] }crates/dojo/utils/src/tx/mod.rs (2)
50-55: Document the defaultFeeConfigbehaviorOhayo, sensei! The
Defaultimplementation forFeeConfigsets it toFeeConfig::Strkby default. It would be beneficial to explicitly document this behavior to inform users that STRK is the default fee token, which could prevent potential confusion.
69-74: EnhanceTxnActionenum documentationOhayo, sensei! The
TxnAction::Sendvariant now includesfee_configandwalnut, but the documentation does not reflect these changes.Consider updating the documentation for
TxnActionto include descriptions of the new fields for better clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
bin/sozo/src/commands/execute.rs(1 hunks)bin/sozo/src/commands/migrate.rs(1 hunks)bin/sozo/src/commands/options/transaction.rs(2 hunks)crates/dojo/utils/src/lib.rs(1 hunks)crates/dojo/utils/src/tx/declarer.rs(2 hunks)crates/dojo/utils/src/tx/deployer.rs(2 hunks)crates/dojo/utils/src/tx/invoker.rs(2 hunks)crates/dojo/utils/src/tx/mod.rs(5 hunks)crates/sozo/ops/src/account.rs(3 hunks)crates/sozo/ops/src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/sozo/ops/src/lib.rs
🔇 Additional comments (14)
crates/dojo/utils/src/lib.rs (1)
11-11: Verify the exposed API surface, sensei!
Let's ensure we're not accidentally exposing internal implementation details with the wildcard export.
✅ Verification successful
Ohayo! The exposed API surface looks well-defined, sensei!
All publicly exposed items serve clear purposes for transaction handling:
- Core transaction structures (
TxnConfig,FeeConfig,StrkFeeConfig,EthFeeConfig) - Transaction actions and results (
TxnAction,TransactionResult) - Essential transaction handlers (
Declarer,Deployer,Invoker,TransactionWaiter) - Error handling (
TransactionError,TransactionWaitingError) - Utility functions (
parse_block_id,execution_status_from_receipt) - Extension trait (
TransactionExt)
The wildcard export is appropriate here as these are all legitimate public API components that users of the crate would need access to.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check all public items that will be exposed by the wildcard export
# Test: List all public items in the tx module
rg -t rust "^pub (struct|enum|fn|type|trait|const|static|use)" crates/dojo/utils/src/tx/
Length of output: 1103
bin/sozo/src/commands/execute.rs (1)
71-71: Ohayo sensei! Great improvement in error handling!
The change from into() to try_into()? enhances robustness by properly handling potential conversion failures, especially important with the new fee token configuration options.
Let's verify the error handling in upstream code:
✅ Verification successful
Ohayo! The error handling change is perfectly implemented, sensei!
The conversion from TransactionOptions to TxnConfig is properly implemented with comprehensive error handling for incompatible fee token configurations. The try_into() change propagates these validation errors up through ExecuteArgs::run(), which already returns a Result<()>, making it a seamless integration.
Key validations handled:
- ETH transactions cannot use gas/gas price options
- STRK transactions cannot use max fee raw/fee estimate multiplier options
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how errors from try_into() are handled in parent functions
# Expected: Find error handling patterns in functions that call execute.rs
# Search for functions that call ExecuteArgs::run
ast-grep --pattern 'ExecuteArgs::run($$$)'
# Search for error handling patterns around ExecuteArgs
rg -A 5 'ExecuteArgs.*\?'
Length of output: 72
Script:
#!/bin/bash
# Let's try a different approach to find error handling patterns
# Find where ExecuteArgs is used
rg "ExecuteArgs" -B 2 -A 5
# Find error propagation patterns in execute.rs
rg "\?\s*$" "bin/sozo/src/commands/execute.rs" -B 2 -A 2
# Look for the TxnConfig implementation
ast-grep --pattern 'impl TryFrom<$_> for TxnConfig'
# Check error type definitions related to TxnConfig
rg "type Error" -A 3 --type rust
Length of output: 14567
crates/dojo/utils/src/tx/declarer.rs (1)
18-20: Ohayo! Clean import organization, sensei!
The addition of FeeConfig to the grouped imports is well-organized and aligns with the PR's objective of supporting different fee token configurations.
crates/sozo/ops/src/account.rs (1)
259-260: Ohayo sensei! The fee configuration changes look good!
The new fee_config parameter properly encapsulates fee-related settings, making it more maintainable and extensible for supporting different fee token types.
crates/dojo/utils/src/tx/deployer.rs (2)
12-14: Ohayo sensei! The addition of FeeConfig and related types is spot on.
Including FeeConfig enhances the configurability of transactions, allowing for flexible fee handling.
63-70: Ohayo sensei! The match statement elegantly handles the fee configurations.
Switching between execute_v3 and execute_v1 based on the FeeConfig ensures that the correct transaction type is executed. This implementation is clear and maintainable.
crates/dojo/utils/src/tx/invoker.rs (1)
58-67: Ohayo, sensei!
Verify consistent error handling between execute_v3 and execute_v1.
Ensure that both execute_v3 and execute_v1 handle transactions and errors consistently. Differences in error propagation or side effects might lead to unexpected behavior.
You can run the following script to compare the implementations:
This script searches the codebase for the implementations of execute_v3 and execute_v1 to help you review their consistency.
✅ Verification successful
Let me gather more information about the ExecutionV1 and ExecutionV3 implementations.
Let me search for the implementation of the TransactionExt trait to understand how these executions are handled.
Let me try a different search pattern to find the error handling implementations.
Ohayo, sensei! Based on the code analysis, I can now provide a conclusive response.
Both execute_v1 and execute_v3 handle errors consistently
The implementations show that both execution versions:
- Return the same result type
InvokeTransactionResult - Use the same error type
AccountError<T::SignError> - Share identical error handling through the
TransactionExttrait implementation - Process transactions through the same
send_with_cfgmethod with consistent error propagation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare the implementations of `execute_v3` and `execute_v1`.
# Find and display the implementations of `execute_v3` and `execute_v1`.
fd -e rs -a . | xargs rg -A 5 -B 5 'fn (execute_v3|execute_v1)\b'
Length of output: 1432
Script:
#!/bin/bash
# Search for ExecutionV1 and ExecutionV3 struct definitions and their implementations
ast-grep --pattern 'struct ExecutionV1 { $$$ }'
ast-grep --pattern 'struct ExecutionV3 { $$$ }'
ast-grep --pattern 'impl$_ ExecutionV1$_ { $$$ }'
ast-grep --pattern 'impl$_ ExecutionV3$_ { $$$ }'
# Also search for any usage patterns
rg "ExecutionV[13]" -A 5 -B 5
Length of output: 28658
Script:
#!/bin/bash
# Search for TransactionExt trait implementation and its send_with_cfg method
ast-grep --pattern 'impl$_ TransactionExt$_ { $$$ }'
rg "impl.*TransactionExt.*send_with_cfg" -A 10 -B 10
Length of output: 109
Script:
#!/bin/bash
# Search for TransactionExt trait definition and implementations
rg "trait TransactionExt" -A 10 -B 2
rg "impl.*for ExecutionV[13]" -A 10 -B 2
# Search for error handling in execute_v1 and execute_v3
rg "fn (execute_v1|execute_v3)" -A 10 -B 2
Length of output: 3845
bin/sozo/src/commands/options/transaction.rs (2)
262-263: Unit Tests for Display Implementation Look Great
Ohayo, sensei! The unit tests verifying the Display implementation for FeeToken are thorough and ensure correct string representations.
169-259: Excellent Addition of Comprehensive Unit Tests
Ohayo, sensei! The added unit tests for TransactionOptions conversions and error handling robustly cover various scenarios, enhancing code reliability.
crates/dojo/utils/src/tx/mod.rs (5)
Line range hint 260-273: Validate FeeConfig variant matches transaction type
Ohayo, sensei! In AccountDeploymentV3, it's important to validate the FeeConfig variant to prevent misconfigured transactions.
Add a check to ensure FeeConfig::Strk is supplied for AccountDeploymentV3 transactions.
164-173:
Validate FeeConfig variant matches transaction type
Ohayo, sensei! Similar to the previous comment, in the send_with_cfg method for DeclarationV2, there's no check for mismatched FeeConfig variants. This could lead to unintended behavior if FeeConfig::Strk is provided.
Implement validation to ensure that FeeConfig::Eth is used with DeclarationV2 transactions.
191-204:
Validate FeeConfig variant matches transaction type
Ohayo, sensei! The send_with_cfg method for AccountDeploymentV1 should validate that the FeeConfig variant is FeeConfig::Eth. Without this check, there's a risk of misconfiguration and unexpected transaction outcomes.
Add a validation step to confirm the correct FeeConfig variant is used.
214-227:
Validate FeeConfig variant matches transaction type
Ohayo, sensei! In ExecutionV3, the method assumes that the FeeConfig variant is FeeConfig::Strk. If FeeConfig::Eth is supplied, the fee parameters won't be set correctly.
Ensure that the FeeConfig variant is validated to be FeeConfig::Strk for ExecutionV3 transactions, and handle mismatches appropriately.
237-250:
Validate FeeConfig variant matches transaction type
Ohayo, sensei! The same validation concern applies to DeclarationV3. Without checking the FeeConfig variant, using an incorrect fee configuration could cause issues.
Include validation to confirm that FeeConfig::Strk is used with DeclarationV3 transactions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2650 +/- ##
==========================================
+ Coverage 57.34% 57.85% +0.51%
==========================================
Files 399 399
Lines 49747 49806 +59
==========================================
+ Hits 28527 28816 +289
+ Misses 21220 20990 -230 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
crates/dojo/utils/src/tx/mod.rs (4)
26-54: Enhance documentation for fee configuration typesOhayo, sensei! The fee configuration structure looks clean, but let's improve the documentation to help users understand the differences between ETH and STRK configurations.
Add these doc comments to clarify the purpose and usage:
#[derive(Debug, Copy, Clone)] pub enum FeeConfig { - /// The STRK fee configuration. + /// The STRK fee configuration for v3 transactions. + /// Uses gas and gas price parameters specific to STRK token. Strk(StrkFeeConfig), - /// The ETH fee configuration. + /// The ETH fee configuration for v1 transactions. + /// Uses fee estimation multiplier and max fee parameters specific to ETH token. Eth(EthFeeConfig), }
58-66: Add documentation for fee_config fieldOhayo! The fee_config field integration looks good, but let's add documentation to explain its purpose and default behavior.
Add this doc comment to clarify the fee_config field:
pub struct TxnConfig { /// Whether to wait for the transaction to be accepted or reverted on L2. pub wait: bool, /// Whether to display the transaction receipt. pub receipt: bool, /// Whether to use the `walnut` fee estimation strategy. pub walnut: bool, - /// The fee configuration to use for the transaction. + /// The fee configuration to use for the transaction. + /// Defaults to STRK configuration for v3 transactions. + /// Can be set to ETH configuration for v1 transactions using --fee-token eth. pub fee_config: FeeConfig, }
Line range hint
137-198: Add validation for fee token configurationOhayo, sensei! The v1 transaction implementations should validate that they're not used with STRK fee configuration.
Add validation to prevent silent failures:
async fn send_with_cfg( mut self, txn_config: &TxnConfig, ) -> Result<Self::R, AccountError<T::SignError>> { - if let FeeConfig::Eth(c) = txn_config.fee_config { + match txn_config.fee_config { + FeeConfig::Eth(c) => { if let Some(fee_est_mul) = c.fee_estimate_multiplier { self = self.fee_estimate_multiplier(fee_est_mul); } if let Some(max_raw_f) = c.max_fee_raw { self = self.max_fee(max_raw_f); } - } + } + FeeConfig::Strk(_) => { + return Err(AccountError::Custom( + "STRK fee configuration is not supported for v1 transactions".into() + )); + } + } self.send().await }
Line range hint
137-272: Consider extracting common fee configuration patternsOhayo, sensei! The fee configuration handling is duplicated across v1 and v3 implementations. Consider extracting this into helper functions or macros.
Here's a suggested approach using a helper trait:
/// Helper trait for fee configuration trait FeeConfigExt<T> { fn apply_eth_config(self, config: &EthFeeConfig) -> Self; fn apply_strk_config(self, config: &StrkFeeConfig) -> Self; } impl<'a, T: ConnectedAccount + Sync> FeeConfigExt<T> for ExecutionV1<'a, T> { fn apply_eth_config(mut self, config: &EthFeeConfig) -> Self { if let Some(fee_est_mul) = config.fee_estimate_multiplier { self = self.fee_estimate_multiplier(fee_est_mul); } if let Some(max_raw_f) = config.max_fee_raw { self = self.max_fee(max_raw_f); } self } fn apply_strk_config(self, _: &StrkFeeConfig) -> Self { panic!("STRK fee configuration is not supported for v1 transactions"); } } // Similar implementations for other transaction typesThis would simplify the
send_with_cfgimplementations:async fn send_with_cfg( mut self, txn_config: &TxnConfig, ) -> Result<Self::R, AccountError<T::SignError>> { match txn_config.fee_config { FeeConfig::Eth(ref c) => self = self.apply_eth_config(c), FeeConfig::Strk(ref c) => self = self.apply_strk_config(c), } self.send().await }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
crates/dojo/utils/src/tx/mod.rs(5 hunks)crates/sozo/ops/src/account.rs(0 hunks)crates/sozo/ops/src/lib.rs(0 hunks)
💤 Files with no reviewable changes (2)
- crates/sozo/ops/src/account.rs
- crates/sozo/ops/src/lib.rs
| impl<T> TransactionExt<T> for ExecutionV3<'_, T> | ||
| where | ||
| T: ConnectedAccount + Sync, | ||
| { | ||
| type R = InvokeTransactionResult; | ||
| type U = AccountError<T::SignError>; | ||
|
|
||
| async fn send_with_cfg( | ||
| mut self, | ||
| txn_config: &TxnConfig, | ||
| ) -> Result<Self::R, AccountError<T::SignError>> { | ||
| if let FeeConfig::Strk(c) = txn_config.fee_config { | ||
| if let Some(g) = c.gas { | ||
| self = self.gas(g); | ||
| } | ||
|
|
||
| if let Some(gp) = c.gas_price { | ||
| self = self.gas_price(gp); | ||
| } | ||
| } | ||
|
|
||
| self.send().await | ||
| } | ||
| } | ||
|
|
||
| impl<T> TransactionExt<T> for DeclarationV3<'_, T> | ||
| where | ||
| T: ConnectedAccount + Sync, | ||
| { | ||
| type R = DeclareTransactionResult; | ||
| type U = AccountError<T::SignError>; | ||
|
|
||
| async fn send_with_cfg( | ||
| mut self, | ||
| txn_config: &TxnConfig, | ||
| ) -> Result<Self::R, AccountError<T::SignError>> { | ||
| if let FeeConfig::Strk(c) = txn_config.fee_config { | ||
| if let Some(g) = c.gas { | ||
| self = self.gas(g); | ||
| } | ||
|
|
||
| if let Some(gp) = c.gas_price { | ||
| self = self.gas_price(gp); | ||
| } | ||
| } | ||
|
|
||
| self.send().await | ||
| } | ||
| } | ||
|
|
||
| impl<T> TransactionExt<T> for AccountDeploymentV3<'_, T> | ||
| where | ||
| T: AccountFactory + Sync, | ||
| { | ||
| type R = DeployAccountTransactionResult; | ||
| type U = AccountFactoryError<T::SignError>; | ||
|
|
||
| async fn send_with_cfg( | ||
| mut self, | ||
| txn_config: &TxnConfig, | ||
| ) -> Result<Self::R, AccountFactoryError<<T>::SignError>> { | ||
| if let FeeConfig::Strk(c) = txn_config.fee_config { | ||
| if let Some(g) = c.gas { | ||
| self = self.gas(g); | ||
| } | ||
|
|
||
| if let Some(gp) = c.gas_price { | ||
| self = self.gas_price(gp); | ||
| } | ||
| } | ||
|
|
||
| self.send().await |
There was a problem hiding this comment.
Add validation for fee token configuration in v3 transactions
Ohayo! Similar to v1 transactions, v3 implementations should validate they're not used with ETH fee configuration.
Add validation to prevent silent failures:
async fn send_with_cfg(
mut self,
txn_config: &TxnConfig,
) -> Result<Self::R, AccountError<T::SignError>> {
- if let FeeConfig::Strk(c) = txn_config.fee_config {
+ match txn_config.fee_config {
+ FeeConfig::Strk(c) => {
if let Some(g) = c.gas {
self = self.gas(g);
}
if let Some(gp) = c.gas_price {
self = self.gas_price(gp);
}
- }
+ }
+ FeeConfig::Eth(_) => {
+ return Err(AccountError::Custom(
+ "ETH fee configuration is not supported for v3 transactions".into()
+ ));
+ }
+ }
self.send().await
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl<T> TransactionExt<T> for ExecutionV3<'_, T> | |
| where | |
| T: ConnectedAccount + Sync, | |
| { | |
| type R = InvokeTransactionResult; | |
| type U = AccountError<T::SignError>; | |
| async fn send_with_cfg( | |
| mut self, | |
| txn_config: &TxnConfig, | |
| ) -> Result<Self::R, AccountError<T::SignError>> { | |
| if let FeeConfig::Strk(c) = txn_config.fee_config { | |
| if let Some(g) = c.gas { | |
| self = self.gas(g); | |
| } | |
| if let Some(gp) = c.gas_price { | |
| self = self.gas_price(gp); | |
| } | |
| } | |
| self.send().await | |
| } | |
| } | |
| impl<T> TransactionExt<T> for DeclarationV3<'_, T> | |
| where | |
| T: ConnectedAccount + Sync, | |
| { | |
| type R = DeclareTransactionResult; | |
| type U = AccountError<T::SignError>; | |
| async fn send_with_cfg( | |
| mut self, | |
| txn_config: &TxnConfig, | |
| ) -> Result<Self::R, AccountError<T::SignError>> { | |
| if let FeeConfig::Strk(c) = txn_config.fee_config { | |
| if let Some(g) = c.gas { | |
| self = self.gas(g); | |
| } | |
| if let Some(gp) = c.gas_price { | |
| self = self.gas_price(gp); | |
| } | |
| } | |
| self.send().await | |
| } | |
| } | |
| impl<T> TransactionExt<T> for AccountDeploymentV3<'_, T> | |
| where | |
| T: AccountFactory + Sync, | |
| { | |
| type R = DeployAccountTransactionResult; | |
| type U = AccountFactoryError<T::SignError>; | |
| async fn send_with_cfg( | |
| mut self, | |
| txn_config: &TxnConfig, | |
| ) -> Result<Self::R, AccountFactoryError<<T>::SignError>> { | |
| if let FeeConfig::Strk(c) = txn_config.fee_config { | |
| if let Some(g) = c.gas { | |
| self = self.gas(g); | |
| } | |
| if let Some(gp) = c.gas_price { | |
| self = self.gas_price(gp); | |
| } | |
| } | |
| self.send().await | |
| impl<T> TransactionExt<T> for ExecutionV3<'_, T> | |
| where | |
| T: ConnectedAccount + Sync, | |
| { | |
| type R = InvokeTransactionResult; | |
| type U = AccountError<T::SignError>; | |
| async fn send_with_cfg( | |
| mut self, | |
| txn_config: &TxnConfig, | |
| ) -> Result<Self::R, AccountError<T::SignError>> { | |
| match txn_config.fee_config { | |
| FeeConfig::Strk(c) => { | |
| if let Some(g) = c.gas { | |
| self = self.gas(g); | |
| } | |
| if let Some(gp) = c.gas_price { | |
| self = self.gas_price(gp); | |
| } | |
| } | |
| FeeConfig::Eth(_) => { | |
| return Err(AccountError::Custom( | |
| "ETH fee configuration is not supported for v3 transactions".into() | |
| )); | |
| } | |
| } | |
| self.send().await | |
| } | |
| } | |
| impl<T> TransactionExt<T> for DeclarationV3<'_, T> | |
| where | |
| T: ConnectedAccount + Sync, | |
| { | |
| type R = DeclareTransactionResult; | |
| type U = AccountError<T::SignError>; | |
| async fn send_with_cfg( | |
| mut self, | |
| txn_config: &TxnConfig, | |
| ) -> Result<Self::R, AccountError<T::SignError>> { | |
| match txn_config.fee_config { | |
| FeeConfig::Strk(c) => { | |
| if let Some(g) = c.gas { | |
| self = self.gas(g); | |
| } | |
| if let Some(gp) = c.gas_price { | |
| self = self.gas_price(gp); | |
| } | |
| } | |
| FeeConfig::Eth(_) => { | |
| return Err(AccountError::Custom( | |
| "ETH fee configuration is not supported for v3 transactions".into() | |
| )); | |
| } | |
| } | |
| self.send().await | |
| } | |
| } | |
| impl<T> TransactionExt<T> for AccountDeploymentV3<'_, T> | |
| where | |
| T: AccountFactory + Sync, | |
| { | |
| type R = DeployAccountTransactionResult; | |
| type U = AccountFactoryError<T::SignError>; | |
| async fn send_with_cfg( | |
| mut self, | |
| txn_config: &TxnConfig, | |
| ) -> Result<Self::R, AccountFactoryError<<T>::SignError>> { | |
| match txn_config.fee_config { | |
| FeeConfig::Strk(c) => { | |
| if let Some(g) = c.gas { | |
| self = self.gas(g); | |
| } | |
| if let Some(gp) = c.gas_price { | |
| self = self.gas_price(gp); | |
| } | |
| } | |
| FeeConfig::Eth(_) => { | |
| return Err(AccountFactoryError::Custom( | |
| "ETH fee configuration is not supported for v3 transactions".into() | |
| )); | |
| } | |
| } | |
| self.send().await |
There was a problem hiding this comment.
that could be great, but we're already verifying this when the fee_config is being built. So this verification here would never be hit.
There was a problem hiding this comment.
@glihm, ohayo! Understood, sensei. Thank you for clarifying that fee token configurations are already verified when the fee_config is being built, so the additional validation in send_with_cfg wouldn't be necessary. I'll keep that in mind for future reviews.
✏️ Learnings added
Learnt from: glihm
PR: dojoengine/dojo#2650
File: crates/dojo/utils/src/tx/mod.rs:201-272
Timestamp: 2024-11-07T14:43:23.403Z
Learning: In `crates/dojo/utils/src/tx/mod.rs`, fee token configurations are verified when the `fee_config` is built, making additional validations in the `send_with_cfg` methods redundant.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
crates/dojo/utils/src/tx/error.rs (1)
20-21: Consider documenting the validation error variant.Adding documentation for the new
TransactionValidationvariant would help clarify its intended use cases, especially in the context of fee token configuration for different transaction types.Consider adding documentation:
#[error("{0}")] + /// Represents errors that occur during transaction validation, + /// such as fee token configuration validation for v1/v3 transactions. TransactionValidation(String),crates/dojo/utils/src/tx/invoker.rs (2)
58-67: Consider enhancing error handling with specific messages, sensei!While the implementation correctly handles both STRK and ETH fee configurations, we could improve error handling by adding specific error messages for each case.
Consider this enhancement:
let tx = match self.txn_config.fee_config { FeeConfig::Strk(config) => { trace!(?config, "Invoking with STRK."); - self.account.execute_v3(vec![call]).send_with_cfg(&self.txn_config).await? + self.account.execute_v3(vec![call]).send_with_cfg(&self.txn_config).await + .map_err(|e| TransactionError::ExecutionError(format!("STRK v3 execution failed: {}", e)))? } FeeConfig::Eth(config) => { trace!(?config, "Invoking with ETH."); - self.account.execute_v1(vec![call]).send_with_cfg(&self.txn_config).await? + self.account.execute_v1(vec![call]).send_with_cfg(&self.txn_config).await + .map_err(|e| TransactionError::ExecutionError(format!("ETH v1 execution failed: {}", e)))? } };
91-100: Consider optimizing vector cloning, sensei!The implementation works correctly, but
self.calls.clone()could be inefficient for large vectors. Consider passing a reference and collecting into a vector only when needed.Here's a potential optimization:
let tx = match self.txn_config.fee_config { FeeConfig::Strk(config) => { trace!(?config, "Invoking with STRK."); - self.account.execute_v3(self.calls.clone()).send_with_cfg(&self.txn_config).await? + let calls: Vec<_> = self.calls.iter().map(|c| c.as_ref()).collect(); + self.account.execute_v3(calls).send_with_cfg(&self.txn_config).await? } FeeConfig::Eth(config) => { trace!(?config, "Invoking with ETH."); - self.account.execute_v1(self.calls.clone()).send_with_cfg(&self.txn_config).await? + let calls: Vec<_> = self.calls.iter().map(|c| c.as_ref()).collect(); + self.account.execute_v1(calls).send_with_cfg(&self.txn_config).await? } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
crates/dojo/utils/src/tx/error.rs(2 hunks)crates/dojo/utils/src/tx/invoker.rs(3 hunks)
🔇 Additional comments (1)
crates/dojo/utils/src/tx/invoker.rs (1)
8-8: Ohayo! The import looks good, sensei!
The FeeConfig import is properly organized with other transaction-related imports.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
crates/dojo/utils/src/tx/deployer.rs (1)
63-72: Clean implementation of fee configuration handling, sensei!The match statement elegantly handles both STRK and ETH fee configurations with proper tracing. However, consider adding explicit error handling for potential invalid fee configurations that might be introduced in the future.
Consider adding a catch-all pattern to handle future fee configurations:
let InvokeTransactionResult { transaction_hash } = match self.txn_config.fee_config { FeeConfig::Strk(_) => { trace!("Deploying with STRK."); self.account.execute_v3(vec![call]).send_with_cfg(&self.txn_config).await? } FeeConfig::Eth(_) => { trace!("Deploying with ETH."); self.account.execute_v1(vec![call]).send_with_cfg(&self.txn_config).await? } + _ => { + return Err(TransactionError::Custom("Unsupported fee configuration".into())); + } };bin/sozo/src/commands/options/transaction.rs (3)
13-14: Ohayo sensei! Consider enhancing the help text for fee token.The help text could be more descriptive about the available options and their implications.
- #[arg(help = "Fee token to use.")] + #[arg(help = "Fee token to use for transaction fees (ETH or STRK).")]
106-109: Enhance error message clarity for ETH transactions.The error message could be more specific about the available options for ETH transactions.
- "Gas and gas price are not supported for ETH transactions. Use `--fee \ - strk` instead." + "Gas and gas price options are only available with STRK. For ETH transactions, use \ + --max-fee-raw or --fee-estimate-multiplier instead."
168-264: Consider adding more edge case tests.While the test coverage is good, consider adding tests for:
- Default values when no fee options are provided
- Behavior with zero values for gas and gas price
- Boundary values for fee estimate multiplier
examples/simple/manifest_sepolia.json (2)
1494-1501: Contract configurations follow consistent patterns!Each contract is well-defined with unique selectors and appropriate system lists. Notable observations:
- Base contracts (c2, c3) have minimal systems (only upgrade)
- Feature-rich contracts (c1) include additional systems (system_1 through system_4)
Consider documenting the rationale for system distribution across contracts to aid future maintenance.
Also applies to: 1674-1677, 1850-1853, 2104-2111
2117-2125: Model and event configurations are well-structured!The configurations maintain consistency with:
- Unique selectors for each model/event
- Clear tagging system (M, E, EH)
- Consistent class_hash patterns
Consider adding comments to describe the purpose of each model and event for better documentation.
Also applies to: 2131-2139
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
bin/sozo/src/commands/options/transaction.rs(2 hunks)crates/dojo/utils/src/tx/deployer.rs(2 hunks)examples/simple/manifest_sepolia.json(16 hunks)
🔇 Additional comments (6)
crates/dojo/utils/src/tx/deployer.rs (2)
12-14: Ohayo! Clean and well-organized imports, sensei!
The new imports align perfectly with the PR's objective of adding fee token configuration support.
61-72: Consider adding gas parameter validation before execution.
Before executing the transaction, it would be beneficial to validate gas parameters to prevent potential failures, especially since different fee tokens might have different gas requirements.
Let's check if there are any existing gas validation patterns in the codebase:
bin/sozo/src/commands/options/transaction.rs (2)
139-166: LGTM! Well-implemented enum with good CLI integration.
The FeeToken enum implementation is robust with:
- Case-insensitive input support through aliases
- Clear string representation
- Proper CLI integration
Line range hint 1-264: Ohayo sensei! Overall implementation looks solid! 🎉
The implementation effectively:
- Introduces fee token configuration with proper validation
- Maintains clear separation between ETH and STRK options
- Provides good error handling and test coverage
- Follows CLI best practices
The code aligns well with the PR objectives of allowing users to switch between v1 and v3 transaction types.
examples/simple/manifest_sepolia.json (2)
3-5: Ohayo! World configuration looks good, sensei!
The updated world configuration and comprehensive entrypoints list provide a clear structure for the contract's capabilities. The systematic organization of entrypoints (core operations → entity management → permissions) enhances maintainability.
Also applies to: 7-29
Line range hint 109-119: New type definitions enhance batch operation support!
The addition of span structs for arrays and ModelIndex improves the contract's capability to handle batch operations efficiently. The bool enum implementation follows Cairo's best practices.
Also applies to: 210-233
Add simple configuration to switch between
v1andv3transactions.We might add the
gas_price_rawif very required by users.Currently the default behavior is
STRK(hence v3), but can be switched using--fee-token eth.Summary by CodeRabbit
New Features
FeeConfigoptions for different fee structures.TransactionValidationerror variant for more granular error handling during transaction processing.manifest_sepolia.jsonfile, enhancing the structure and functionality of the world and contracts sections.Bug Fixes
Documentation
Chores
accountmodule and adjusted its visibility for improved access control.