Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(katana-pool): rename error for clarity #2528

Merged
merged 2 commits into from
Oct 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion crates/katana/pool/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,9 @@ use katana_primitives::transaction::{ExecutableTxWithHash, TxHash};
use ordering::{FiFo, PoolOrd};
use pool::Pool;
use tx::{PendingTx, PoolTransaction};
use validation::error::InvalidTransactionError;
use validation::stateful::TxValidator;
use validation::{InvalidTransactionError, Validator};
use validation::Validator;

/// Katana default transacstion pool type.
pub type TxPool = Pool<ExecutableTxWithHash, TxValidator, FiFo<ExecutableTxWithHash>>;
Expand Down
3 changes: 2 additions & 1 deletion crates/katana/pool/src/pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ use tracing::{error, info, warn};

use crate::ordering::PoolOrd;
use crate::tx::{PendingTx, PoolTransaction, TxId};
use crate::validation::{InvalidTransactionError, ValidationOutcome, Validator};
use crate::validation::error::InvalidTransactionError;
use crate::validation::{ValidationOutcome, Validator};
use crate::{PoolError, PoolResult, TransactionPool};

#[derive(Debug)]
Expand Down
66 changes: 66 additions & 0 deletions crates/katana/pool/src/validation/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
use katana_primitives::class::ClassHash;
use katana_primitives::contract::{ContractAddress, Nonce};
use katana_primitives::Felt;

// TODO: figure out how to combine this with ExecutionError
#[derive(Debug, thiserror::Error)]
pub enum InvalidTransactionError {
/// Error when the account's balance is insufficient to cover the specified transaction fee.
#[error("Max fee ({max_fee}) exceeds balance ({balance}).")]
InsufficientFunds {
/// The specified transaction fee.
max_fee: u128,
/// The account's balance of the fee token.
balance: Felt,
},

/// Error when the specified transaction fee is insufficient to cover the minimum fee required
/// to start the invocation (including the account's validation logic).
///
/// It is a static check that is performed before the transaction is invoked to ensure the
/// transaction can cover the intrinsics cost ie data availability, etc.
///
/// This is different from an error due to transaction runs out of gas during execution ie.
/// the specified max fee is lower than the amount needed to finish the transaction execution
/// (either validation or execution).
#[error("Intrinsic transaction fee is too low")]
IntrinsicFeeTooLow {
/// The minimum required for the transaction to be executed.
min: u128,
/// The specified transaction fee.
max_fee: u128,
},
Comment on lines +26 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider enhancing the error message for clarity.

Including the min and max_fee values in the IntrinsicFeeTooLow error message can provide better context and align with the error message format used in InsufficientFunds. This will help users understand exactly why their transaction fee is considered too low.

You might update the error annotation as follows:

-#[error("Intrinsic transaction fee is too low")]
+#[error("Intrinsic transaction fee is too low. Minimum required: {min}; provided: {max_fee}.")]
📝 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.

Suggested change
#[error("Intrinsic transaction fee is too low")]
IntrinsicFeeTooLow {
/// The minimum required for the transaction to be executed.
min: u128,
/// The specified transaction fee.
max_fee: u128,
},
#[error("Intrinsic transaction fee is too low. Minimum required: {min}; provided: {max_fee}.")]
IntrinsicFeeTooLow {
/// The minimum required for the transaction to be executed.
min: u128,
/// The specified transaction fee.
max_fee: u128,
},


/// Error when the account's validation logic fails (ie __validate__ function).
#[error("{error}")]
ValidationFailure {
/// The address of the contract that failed validation.
address: ContractAddress,
/// The class hash of the account contract.
class_hash: ClassHash,
/// The error message returned by Blockifier.
// TODO: this should be a more specific error type.
error: String,
},

/// Error when the transaction's sender is not an account contract.
#[error("Sender is not an account")]
NonAccount {
/// The address of the contract that is not an account.
address: ContractAddress,
},

/// Error when the transaction is using a nonexpected nonce.
#[error(
"Invalid transaction nonce of contract at address {address}. Account nonce: \
{current_nonce:#x}; got: {tx_nonce:#x}."
)]
InvalidNonce {
/// The address of the contract that has the invalid nonce.
address: ContractAddress,
/// The current nonce of the sender's account.
current_nonce: Nonce,
/// The nonce that the tx is using.
tx_nonce: Nonce,
},
}
56 changes: 3 additions & 53 deletions crates/katana/pool/src/validation/mod.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
pub mod error;
pub mod stateful;

use katana_primitives::class::ClassHash;
use katana_primitives::contract::{ContractAddress, Nonce};
use error::InvalidTransactionError;
use katana_primitives::contract::Nonce;
use katana_primitives::transaction::TxHash;
use katana_primitives::Felt;

use crate::tx::PoolTransaction;

Expand All @@ -16,56 +16,6 @@ pub struct Error {
pub error: Box<dyn std::error::Error>,
}

// TODO: figure out how to combine this with ExecutionError
#[derive(Debug, thiserror::Error)]
pub enum InvalidTransactionError {
/// Error when the account's balance is insufficient to cover the specified transaction fee.
#[error("Max fee ({max_fee}) exceeds balance ({balance}).")]
InsufficientFunds {
/// The specified transaction fee.
max_fee: u128,
/// The account's balance of the fee token.
balance: Felt,
},

/// Error when the specified transaction fee is insufficient to cover the minimum fee required.
#[error("The specified tx max fee is insufficient")]
InsufficientMaxFee { min_fee: u128, max_fee: u128 },

/// Error when the account's validation logic fails (ie __validate__ function).
#[error("{error}")]
ValidationFailure {
/// The address of the contract that failed validation.
address: ContractAddress,
/// The class hash of the account contract.
class_hash: ClassHash,
/// The error message returned by Blockifier.
// TODO: this should be a more specific error type.
error: String,
},

/// Error when the transaction's sender is not an account contract.
#[error("sender is not an account")]
NonAccount {
/// The address of the contract that is not an account.
address: ContractAddress,
},

/// Error when the transaction is using a nonexpected nonce.
#[error(
"Invalid transaction nonce of contract at address {address}. Account nonce: \
{current_nonce:#x}; got: {tx_nonce:#x}."
)]
InvalidNonce {
/// The address of the contract that has the invalid nonce.
address: ContractAddress,
/// The current nonce of the sender's account.
current_nonce: Nonce,
/// The nonce that the tx is using.
tx_nonce: Nonce,
},
}

pub type ValidationResult<T> = Result<ValidationOutcome<T>, Error>;

/// A trait for validating transactions before they are added to the transaction pool.
Expand Down
2 changes: 1 addition & 1 deletion crates/katana/pool/src/validation/stateful.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ fn map_invalid_tx_err(
TransactionFeeError::MaxFeeTooLow { min_fee, max_fee } => {
let max_fee = max_fee.0;
let min_fee = min_fee.0;
Ok(InvalidTransactionError::InsufficientMaxFee { max_fee, min_fee })
Ok(InvalidTransactionError::IntrinsicFeeTooLow { max_fee, min: min_fee })
}

_ => Err(Box::new(err)),
Expand Down
4 changes: 2 additions & 2 deletions crates/katana/rpc/rpc-types/src/error/starknet.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use jsonrpsee::core::Error;
use jsonrpsee::types::error::CallError;
use jsonrpsee::types::ErrorObject;
use katana_pool::validation::InvalidTransactionError;
use katana_pool::validation::error::InvalidTransactionError;
use katana_pool::PoolError;
use katana_primitives::event::ContinuationTokenError;
use katana_provider::error::ProviderError;
Expand Down Expand Up @@ -182,7 +182,7 @@ impl From<Box<InvalidTransactionError>> for StarknetApiError {
fn from(error: Box<InvalidTransactionError>) -> Self {
match error.as_ref() {
InvalidTransactionError::InsufficientFunds { .. } => Self::InsufficientAccountBalance,
InvalidTransactionError::InsufficientMaxFee { .. } => Self::InsufficientMaxFee,
InvalidTransactionError::IntrinsicFeeTooLow { .. } => Self::InsufficientMaxFee,
InvalidTransactionError::NonAccount { .. } => Self::NonAccount,
InvalidTransactionError::InvalidNonce { .. } => {
Self::InvalidTransactionNonce { reason: error.to_string() }
Expand Down
Loading