-
Notifications
You must be signed in to change notification settings - Fork 94
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
Withdraw ERC1155 and EVM based chains support added for NFT PoC #1704
Conversation
Oh, it is from previous nft list / history struct versions, when there were different nft lists grouped by chain. |
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.
Thanks you for the fixes! Next iteration
mm2src/coins/eth.rs
Outdated
} | ||
|
||
#[cfg(feature = "enable-nft-integration")] | ||
fn get_valid_eth_withdraw_addresses( |
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.
get_valid_eth_withdraw_addresses
is used only for NFT so the name should probably be more indicative of this. I actually think there should be a common implementation for both withdraw NFT functions that gets used in both withdraw_erc721
, withdraw_erc1155
since there is a lot of common code between them, this will make this function not needed and the code below will be included in only the common withdraw NFT impl.
Please consider this refactor in the next PRs.
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.
changed to get_valid_nft_add_to_withdraw
.
Yeah, there is a need for refactoring, noted it
…aw-erc1155 # Conflicts: # mm2src/coins/eth.rs
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.
Great Work 🔥
Only a non-blocker below and this #1704 (comment)
@ozkanonur Hi, its ready for next review iteration. |
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.
LGTM, only a few 'not so important' suggestions
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.
Re-approve
related to #900
withdraw_erc1155
methodAvalanche
,Fantom
,Polygon
chains was added. Use uppercase for chain names.If there is no amount param for
withdraw_nft
, amount = 1 by default.withdraw_nft req for ERC1155 token without amount param
withdraw_nft res for ERC1155 token without amount param
send_raw_transaction req
send_raw_transaction res
polygonscan result
Another
withdraw_nft
request example with max param.withdraw_nft req for ERC1155 with max:true
withdraw_nft res for ERC1155 with max:true
send_raw_transaction req
send_raw_transaction res
polygonscan result