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

Add client side check of withdraw-ability #1546

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pLabarta
Copy link
Contributor

This pull request introduces changes to the frame/ethereum and frame/evm modules to improve transaction validation and add a new method for checking the maximum withdrawable amount on the client side.

@pLabarta pLabarta requested a review from sorpaas as a code owner December 10, 2024 18:48
@pLabarta pLabarta marked this pull request as draft December 10, 2024 18:48
@pLabarta pLabarta marked this pull request as ready for review December 10, 2024 21:09
@@ -199,6 +199,14 @@ impl<'config, E: From<TransactionValidationError>> CheckEvmTransaction<'config,
}
}

pub fn max_withdraw_amount(&self) -> Result<U256, E> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does with_balance_for in CheckEvmTransaction meet your needs?


use pallet_evm::OnChargeEVMTransaction;
let max_withdraw = check_transaction.max_withdraw_amount().map_err(|e| e.0)?;
<T as pallet_evm::Config>::OnChargeTransaction::can_withdraw(&origin, max_withdraw)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What additional validation does this offer compared to the with_balance_for mentioned above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm rechecking this change we have at Moonbeam's frontier fork to see if the issue was fixed with paritytech/polkadot-sdk#2292. Basically it meant to make a consistent check for the withdraw amount to prevent a corner case where the TX would be included in the pool even if it did not have enough balance to pay the fees.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants