-
Notifications
You must be signed in to change notification settings - Fork 41
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
Reworks the balance checks and adds them for erc721 tokens #335
Conversation
schmanu
commented
Jan 7, 2022
- Instead of using the web3provider to check individual token balances we fetch all balances at the start of the app
- Adds balance and duplicate ID check for ERC721 transfers
- Also adds a Warning if there are more than 400 lines of transfers (and doesn't parse the file then)
- Instead of using the web3provider to check individual token balances we fetch all balances at the start of the app - Adds balance and duplicate ID check for ERC721 transfers
* The link to the sample file was moved into the Help-Modal * Instead there is a menu to generate transfers with the option to drain the safe closes #171
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.
Looking really good. Does this cover all the suggestions?
Wondering where will the drain safe button be?
collectibleBalance?.forEach((collectible) => { | ||
drainCSV += `\nnft,${collectible.address},${drainAddress},,${collectible.id}`; | ||
}); |
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.
Wow! Even NFTs! Sexy! This is probably better than the existing drain safe app.
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.
If we drain we leave no stain!
src/utils.ts
Outdated
if (typeof tokenSummary === "undefined") { | ||
tokenSummary = { | ||
tokenAddress: currentValue.tokenAddress, | ||
amount: 0, // We track the amount to detect duplicate ids |
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.
Maybe it would be better to use 'count' here. Amount seems "fungible"
* 100% test coverage of asset and collectible balance check
* the csvParser covers the max-line check * test for the max-line check
* a boolean flag for duplicate transfers instead of transferAmount * test for duplicate transfer detection
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.
Looks good to me!