Open kcsongor opened 1 year ago
Great point being made here! I. think this would be a very positive change.
This code was always meant as a temporary solution and it's nice to see that now there are collection IDs being used on Solana.
Given this only touches the Solana -> Other Chain path because Solana inbound transfers ignore the contract address (except this check we'll need to drop https://github.com/wormhole-foundation/wormhole/blob/be49832a0767d6514892b2a9419f4f7747973a2a/solana/modules/nft_bridge/program/src/api/complete_transfer.rs#L107), this seems like a low-friction and -risk change.
2 things we have to consider:
any update on this?
Solana NFTs require special handling currently. This is because at the time the Solana NFT bridge was first implemented, there was no notion of NFT collections on Solana, and each NFT would simply be its own individual token. Due to the gas costs of creating collections on Ethereum, the Solana NFT bridge simply puts all NFTs into a single dummy collection, so when transferred to other chains, they end up under the same collection. https://github.com/wormhole-foundation/wormhole/blob/be49832a0767d6514892b2a9419f4f7747973a2a/solana/modules/nft_bridge/program/src/api/transfer.rs#L191 This means that storing the collection metadata in the wrapped collection does not work due to the many-to-one mapping. Instead, the receiving contracts implement a cache (the "SPL cache") to store the name and symbol of these tokens separately in a mapping keyed by the solana token's address.
For example, the eth contract has this logic to recover the metadata when transferring out the NFT: https://github.com/wormhole-foundation/wormhole/blob/be49832a0767d6514892b2a9419f4f7747973a2a/ethereum/contracts/nft/NFTBridge.sol#L55-L61
However, most NFTs on Solana now do include a collection key, which means it is possible to preserve the
token_address
field and avoid having to trigger the special code path on the other chains. Then, Solana NFTs would behave identically to other NFTs, and the wrapped collection would map 1-to-1 to the original collection on Solana.Migration
To avoid any breaking changes, I propose the following migration plan:
tokenChain == 1
, it also checks thattokenAddress == DUMMY_SOLANA_ADDRES
before triggering the special code path. Doing so is fully backwards compatible, since all NFT transfer VAAs currently look like this.Performing these two steps in order avoids producing any invalid states. #2102 implements 1. already, so it will be compatible with 2. from day one.