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: token paymaster decimal bug #459

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

Conversation

a00012025
Copy link

Summary

When I tried the latest TokenPaymaster in sample, I noticed that it only considers the case of token's decimal being 18, which will cause amount calculation error. This PR fixes this issue by querying the token's decimals when calculating the token amount to transfer to/from paymaster

@drortirosh
Copy link
Contributor

are you sure?
we do have tests that work with decimals=8.
If you do think there is a problem, please also try add a test to it as well.

@0xBigBoss
Copy link

Funny, I encountered the same thing when adding the TokenPaymaster to https://github.com/0xsend/sendapp. Really racked my head on this one. I believe for the most part all the calculations are correct except for the ones in UniswapHelper. I think it's missing the decimal scaling when converting. I had a similar approach in our version of UniswapHelper. You can see that here,
develop...0xBigBoss:account-abstraction:develop.

I didn't spend too much time looking at fixing the tests, but you can see now that the actualTokenCharge amounts are inline with what they should be.

actualTokenChargeEvents= 1106
actualTokenCharge= 1106
actualTokenChargeEvents= 1106
expectedTokenCharge= 1106842507747897
    1) should be able to sponsor the UserOp while charging correct amount of ERC-20 tokens

If I understand correctly, the decimal for the token in TokenPaymaster.test.ts is 6, which means this transaction would have used $0.001106 as opposed to $1,106,842,507.747897 😅

Screenshot 2024-04-01 at 21 05 44

I re-wrote most of the tests in solidity for those interested. https://github.com/0xsend/sendapp/blob/7fa242d2069dbfa71c475f73020b17f96670a970/packages/contracts/test/TokenPaymaster6.t.sol#L27

@a00012025
Copy link
Author

a00012025 commented Apr 2, 2024

@drortirosh Hi, I've updated the test to reflect my changes and make them pass. I also updated the assertion of charged token price to avoid floating point error. Please help check it, thanks.

@a00012025
Copy link
Author

@drortirosh Hi, can you help check this? Thanks

@0xFirekeeper
Copy link

+1, had to make same change for usdc with chainlink oracle

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.

4 participants