-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: Add option to check any ERC20 token balance #25
base: main
Are you sure you want to change the base?
Conversation
Hey @Pratham-19! I’m really impressed with your improvements to the CLI—they look fantastic. I have a few comments about your implementation that are critical to address before we can merge your PR. I’ve already tested it, and it works well! However, there are some aspects of the implementation that don’t align with the existing CLI structure and the plans outlined for upcoming features in our backlog. Specifically, you created a new program called
For reference, there is a very similar implementation using Thanks again for your hard work! You’re doing an amazing job. If you have any questions or need further clarification, feel free to reach out. |
Thanks for your comment @chrisarevalodev I got the requirements, I'll revamp my code and add inquirer and ora for the same |
Hey @chrisarevalodev , These are the updates
|
Hey @Pratham-19, I like how the updates are looking! 👀. However, I noticed some bugs that I'm going to describe below.
Please check on these comments and make the proper updates so we can approve it and merge it! You've done a great work. |
Hey @chrisarevalodev , I have added a function to check whether the contract is erc20, so it will only work for all the ERC20 contract |
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.
Tested and working. LGTM @ezequiel-rodriguez
src/constants/tokenAdress.ts
Outdated
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.
src/constants/erc20ABI.ts
Outdated
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.
Please remove this file and use erc20abi
constant that can be imported from viem.
src/utils/index.ts
Outdated
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.
Unnecesary blank spaces
src/commands/balance.ts
Outdated
getTokenInfo, | ||
isERC20Contract, | ||
resolveTokenAddress, | ||
} from "../utils/tokenHelper.js"; | ||
import ora from "ora"; | ||
import { console } from "inspector"; |
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.
Please remove the line with import:
import { console } from "inspector";
as it is not necessary. Just use default console.log
function
Implementing
rsk-cli balance --contract <address> --address (optional) -t