-
Notifications
You must be signed in to change notification settings - Fork 817
fix: ensure transactions succeed when balance check is disabled #2286
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
Conversation
CodSpeed Performance ReportMerging #2286 will not alter performanceComparing Summary
|
crates/handler/src/validation.rs
Outdated
balance: Box::new(account.balance), | ||
}); | ||
if balance_check > account.balance { | ||
if context.cfg().is_balance_check_disabled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous code did remove balance check, but it would in deduct_caller reduce it to zero, is there a need for the balance to remain unchanged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation causes an OutOfFunds
when deducting the balance, in later execution calls.
I can try to find all missed checks there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm it is about value that is sent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets introduce a logic inside deduc_caller that would leave at least value
amount of balance if cfg is enabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 885b273
a72d2d1
to
885b273
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I discovered that code for the "disable balance check" feature was removed in the previous refactors, here.
This PR fixes the functionality.
[edit] I moved this to a draft as I need to validate a failing test on the Hardhat side