-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(cast): erc20 safe preflight #12499
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
base: master
Are you sure you want to change the base?
feat(cast): erc20 safe preflight #12499
Conversation
|
Also, I would like to update Foundry book, according to a new flag |
grandizzy
left a comment
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.
thank you, left a comment re new arg, don't think we need it?
|
Hmmm, CI / deny fails. But looks like it isn't my fault |
grandizzy
left a comment
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.
thank you, left comment, pls check
onbjerg
left a comment
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.
i know you can pipe in yes, but let's also add a --yes/-y as this is pretty common for clis
Hi there, @onbjerg , I actually agree with the However, other maintainers had different perspectives:
Their reasoning was avoiding adding new arguments to keep the CLI simpler and easier to maintain and following Unix philosophy with I'm happy to implement either approach, but before making changes again, I think it would be helpful if the maintainers could align on the preferred solution: Option 1: @grandizzy @0xrusowsky @onbjerg - could you please discuss and let me know which direction you'd like me to take? I want to make sure we're all aligned before implementing the final version. Other maintainers are also invited to the conversation @mattsse , @zerosnacks , @DaniPopes |
Motivation
Closes #12402
Enhance UX and prevent accidental transfers with incorrect amounts. Users may misunderstand the token's decimal places and accidentally send far more (or less) tokens than intended.
Also, approve() requires identical feature, so I decided to add the same.
At the other hand, transferFrom() - doesn't need that, because this method is rarely called by users implicitly, more often him calls other smartcontracts (like swapRouter).
Solution
Added interactive confirmation prompts to cast erc20 transfer and cast erc20 approve commands that:
Fetch token metadata (symbol and decimals) from the ERC20 contract
Display human-readable amounts in the confirmation prompt:
Example: Confirm transfer of 100 USDC to address 0x666...666 instead of raw 100000000
Support non-interactive usage via stdin piping (e.g.,
yes | cast erc20 transfer ...)Handle edge cases gracefully:
-Falls back to raw amount if decimals/symbol can't be fetched
-Shows warning if token metadata is unavailable
Demonstration
Let's create a test token first:
Then we can test behavior:
In case y (yes):
In case n (no):
With skip promt that would be:
In case where contract address aren't correct/contract doesn't support ERC20 interface for decimals/symbol, we'll get a warning:
The same behavior has been added to approve():
PR Checklist