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

feat(sdk-core, sdk-coin-eth): add function to transfer nfts #4150

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

mohammadalfaiyazbitgo
Copy link
Contributor

Ticket: WP-1094

TICKET: WP-1094

@mohammadalfaiyazbitgo
Copy link
Contributor Author

* Build the data to transfer an ERC-721 or ERC-1155 token to another address
* @param params
*/
buildNftTransferData(params: NFTTransferOptions & { fromAddress: string }): string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that this is only implemented for abstractEthLikeNewCoins and not abstractEthLike. That's intentional right? I'm not super familiar on the ETH set up, just wanna make sure that we're only expecting it to be for these new coins

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so, both eth & polygon extend from abstractEthLikeNewCoins which are the only chains we need to implement nfts for atm.

recipients: [
{
address: sendNftOptions.tokenContractAddress,
amount: '0',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a case where we'd have any fees and it doesnt make sense for amount to be 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think so, the documentation https://developers.bitgo.com/guides/transact/nfts says to always set the amount to 0.

- Add a new sendNftMethod to make transferring
ERC-721/1155 tokens easier.
- Expose function to construct call data for
an ERC-721/1155 transfers

TICKET: WP-1094
@mohammadalfaiyazbitgo mohammadalfaiyazbitgo marked this pull request as ready for review December 13, 2023 18:07
Comment on lines +2146 to +2156
const data = this.baseCoin.buildNftTransferData({ ...sendNftOptions, fromAddress: baseAddress });
return this.sendMany({
...sendOptions,
recipients: [
{
address: sendNftOptions.tokenContractAddress,
amount: '0',
data: data,
},
],
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems duplicated, a nit for another pr

@mohammadalfaiyazbitgo mohammadalfaiyazbitgo merged commit 2302dab into master Dec 13, 2023
7 checks passed
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