feat: send digital asset #5988
feat: send digital asset #5988
Conversation
bhavidhingra
force-pushed
the
coin-3820-send-nft
branch
from
c4c95df to
c3836ce
Compare
| @@ -2729,6 +2729,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin { | |||
| return transferBuilder.build(); | |||
| } | |||
| } | |||
| return ''; | |||
There was a problem hiding this comment.
shouldn't we throw an error here ?
Ideally we should have done it when we introduced this change
There was a problem hiding this comment.
Yes, I was going to do this today, thanks.
| @@ -646,6 +646,7 @@ export interface SendManyOptions extends PrebuildAndSignTransactionOptions { | |||
| feeLimit?: string; | |||
| data?: string; | |||
| tokenName?: string; | |||
| tokenData?: TokenTransferRecipientParams; | |||
There was a problem hiding this comment.
what is the difference between data and tokenData ?
isn't it confusing for the user ?
There was a problem hiding this comment.
data field is being populated internally here.
tokenData field of type TokenTransferRecipientParams is less confusing for the users than the data field of type string. The data field is being used specifically for eth-like coins.
| @@ -2405,7 +2407,7 @@ export class Wallet implements IWallet { | |||
| if (!this.baseCoin.isValidAddress(recipientAddress)) { | |||
| throw new Error(`Invalid recipient address ${recipientAddress}`); | |||
| } | |||
| const baseAddress = this.coinSpecific()?.baseAddress; | |||
| const baseAddress = this.coinSpecific()?.baseAddress || this.coinSpecific()?.rootAddress; | |||
There was a problem hiding this comment.
can we do this.baseCoin.getBaseAddress ?
abstract the coinSpecific things
| case 'Digital Asset': { | ||
| this.validateNftEntries(sendNftOptions.entries, nftBalance, sendNftOptions); | ||
|
|
||
| const recipients = _.map(sendNftOptions.entries, (entry) => { |
There was a problem hiding this comment.
| const recipients = _.map(sendNftOptions.entries, (entry) => { | |
| const recipients = sendNftOptions.entries.map( (entry) => { |
There was a problem hiding this comment.
also what does entries mean here ?
There was a problem hiding this comment.
entries: https://github.com/BitGo/BitGoJS/blob/master/modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts#L457
it denotes a list of NFT IDs and their quantity for ERC1155 type NFTs.
I'm not using entries for Aptos NFTs now.
| }); | ||
| return this.sendMany({ | ||
| ...sendOptions, | ||
| recipients, |
There was a problem hiding this comment.
can the recipients be > 1 ?
There was a problem hiding this comment.
no, right now there will be just 1 recipient, using a single recipient now.
bhavidhingra
force-pushed
the
coin-3820-send-nft
branch
3 times, most recently
from
07f355b to
4acdae2
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for sending digital assets by extending the existing NFT transfer functionality. Key changes include:
- Extending the NFT transfer options (iBaseCoin and wallet interfaces) to include a new "Digital Asset" type.
- Adding a new case in the wallet implementation to handle digital asset transfers.
- Updating tokens and error handling logic to accommodate the new digital asset type.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| modules/sdk-core/src/bitgo/wallet/wallet.ts | Adds digital asset transfer flow and updates base address fallback logic. |
| modules/sdk-core/src/bitgo/wallet/iWallet.ts | Updates SendManyOptions to include an optional tokenData field. |
| modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts | Adds DIGITAL_ASSET to the TokenType enum and updates token transfer parameters. |
| modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts | Expands NFT transfer types to include "Digital Asset". |
| modules/abstract-eth/src/abstractEthLikeNewCoins.ts | Introduces a default case that throws an error for unsupported NFT types. |
Comments suppressed due to low confidence (2)
modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts:452
- Consider using the DIGITAL_ASSET enum value (TokenType.DIGITAL_ASSET) for consistency instead of the hardcoded string literal 'Digital Asset'.
type: 'ERC721' | 'Digital Asset';
modules/sdk-core/src/bitgo/wallet/wallet.ts:2477
- [nitpick] If digital asset transfers always use a quantity of 1, consider adding a clarifying comment or making this behavior configurable to avoid future confusion.
tokenQuantity: '1',
bhavidhingra
force-pushed
the
coin-3820-send-nft
branch
from
4acdae2 to
c40b9b8
Compare
bhavidhingra
force-pushed
the
coin-3820-send-nft
branch
4 times, most recently
from
cb86fbe to
4c7614b
Compare
TICKET: COIN-3820
bhavidhingra
force-pushed
the
coin-3820-send-nft
branch
from
4c7614b to
e4f9135
Compare
TICKET: COIN-3819
This pull request introduces support for a new NFT type, "Digital Asset," across the SDK. The changes include updates to type definitions, error handling, and transaction logic to accommodate this new token type.
Support for "Digital Asset" NFT type:
modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts: UpdatedNFTTransferOptionsto include the new type,'Digital Asset', alongside existing NFT types.modules/sdk-core/src/bitgo/utils/tss/baseTypes.ts: AddedDIGITAL_ASSETto theTokenTypeenum to represent the new token type.Error handling and transaction logic:
modules/abstract-eth/src/abstractEthLikeNewCoins.ts: Added a default case to handle unsupported NFT types, throwing an error if an unrecognized type is encountered.modules/sdk-core/src/bitgo/wallet/wallet.ts: Enhanced theWalletclass to handle "Digital Asset" NFTs, including validation for token existence and spendable balance, and added logic to process transactions with the new type.Additional enhancements:
modules/sdk-core/src/bitgo/wallet/iWallet.ts: Introduced a new property,tokenData, inSendManyOptionsto support token transfer details for recipients.modules/sdk-core/src/bitgo/wallet/wallet.ts: Updated thebaseAddressfallback logic to includerootAddressifbaseAddressis unavailable, ensuring compatibility with different wallet configurations.