Skip to content

Conversation

Wodann
Copy link
Contributor

@Wodann Wodann commented Mar 21, 2025

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

Copy link

codspeed-hq bot commented Mar 21, 2025

CodSpeed Performance Report

Merging #2286 will not alter performance

Comparing Wodann:fix/balance-check (885b273) with main (556f8dd)

Summary

✅ 8 untouched benchmarks

@Wodann Wodann marked this pull request as draft March 22, 2025 18:59
balance: Box::new(account.balance),
});
if balance_check > account.balance {
if context.cfg().is_balance_check_disabled() {
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 885b273

@Wodann Wodann force-pushed the fix/balance-check branch from a72d2d1 to 885b273 Compare March 24, 2025 20:28
@Wodann Wodann marked this pull request as ready for review March 24, 2025 20:29
Copy link
Member

@rakita rakita left a comment

Choose a reason for hiding this comment

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

lgtm

@rakita rakita merged commit d0b1af1 into bluealloy:main Mar 25, 2025
29 checks passed
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.

2 participants