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

Fix overflow in bad token detection #212

Merged
merged 2 commits into from
May 20, 2022

Conversation

fedgiac
Copy link
Contributor

@fedgiac fedgiac commented May 19, 2022

We had some panics in prod because the NOT token uses maxuint256 for all user balances, which causes a panic when checking if the token is unsupported.

These changes fix the panic.

Test Plan

Updated ignored unit test:

$ cargo test -p shared mainnet_tokens -- --nocapture --ignored
[...]
token 0x0027449bf0887ca3e431d263ffdefb244d95b555 is Ok(Bad { reason: "token total supply does not fit a uint256" })
[...]

Also, compare the output of the test with its output on main to check that no other changes were introduced:

$ diff before after
104a105
> token 0x0027449bf0887ca3e431d263ffdefb244d95b555 is Ok(Bad { reason: "token total supply does not fit a uint256" })
107c108
< test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 250 filtered out; finished in 23.82s
---
> test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 250 filtered out; finished in 23.80s

@fedgiac fedgiac requested a review from a team as a code owner May 19, 2022 14:36
@codecov-commenter
Copy link

Codecov Report

Merging #212 (314f00c) into main (aa29117) will decrease coverage by 0.00%.
The diff coverage is 46.66%.

@@            Coverage Diff             @@
##             main     #212      +/-   ##
==========================================
- Coverage   64.41%   64.40%   -0.01%     
==========================================
  Files         189      189              
  Lines       38788    38800      +12     
==========================================
+ Hits        24987    24991       +4     
- Misses      13801    13809       +8     

Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

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

LGTM

@fedgiac fedgiac added the oncall Issue/PR for consideration during oncall rotation label May 19, 2022
@fedgiac fedgiac enabled auto-merge (squash) May 20, 2022 09:25
@fedgiac fedgiac merged commit 5b24a90 into main May 20, 2022
@fedgiac fedgiac deleted the fix-overflow-in-bad-token-detection branch May 20, 2022 09:29
@github-actions github-actions bot locked and limited conversation to collaborators May 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
oncall Issue/PR for consideration during oncall rotation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants