-
Notifications
You must be signed in to change notification settings - Fork 182
Add support for compressed NFTs #480
Add support for compressed NFTs #480
Conversation
🦋 Changeset detectedLatest commit: 1364d8d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
/cc @lorisleiva i will continue to add support for some of the other functions while you're taking a peek at this, but let me know if you have any big changes you'd want to see here |
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 both of you for your great work on this!
I've added a few questions/comments.
Additionally, all existing tests are passing except for the one that ensures the JS SDK can be imported as an ESM module. It seems that the @solana/spl-account-compression
library is CJS only which can be imported to ESM modules but differently. See error below.
file:///home/runner/work/js/js/packages/js/dist/esm/plugins/nftModule/operations/createCompressedNft.mjs:3
import { SPL_ACCOUNT_COMPRESSION_PROGRAM_ID, SPL_NOOP_PROGRAM_ID } from '@solana/spl-account-compression';
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
SyntaxError: Named export 'SPL_ACCOUNT_COMPRESSION_PROGRAM_ID' not found. The requested module '@solana/spl-account-compression' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:
import pkg from '@solana/spl-account-compression';
const { SPL_ACCOUNT_COMPRESSION_PROGRAM_ID, SPL_NOOP_PROGRAM_ID } = pkg;
The issue is, if we do change the way we import it as suggested, we break CJS modules. I've had this issue with the Bundlr library in the past and ended up having to create a helper function that removed the double default
attribute from the package (See code below).
js/packages/js/src/plugins/bundlrStorage/BundlrStorageDriver.ts
Lines 37 to 56 in b3c9a45
/** | |
* This method is necessary to import the Bundlr package on both ESM and CJS modules. | |
* Without this, we get a different structure on each module: | |
* - CJS: { default: [Getter], WebBundlr: [Getter] } | |
* - ESM: { default: { default: [Getter], WebBundlr: [Getter] } } | |
* This method fixes this by ensure there is not double default in the imported package. | |
*/ | |
// eslint-disable-next-line @typescript-eslint/naming-convention | |
function _removeDoubleDefault(pkg: any) { | |
if ( | |
pkg && | |
typeof pkg === 'object' && | |
'default' in pkg && | |
'default' in pkg.default | |
) { | |
return pkg.default; | |
} | |
return pkg; | |
} |
Would either of you mind taking a look at that?
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.
I see a lot of TODOs in this file. Is this operation ready enough to be merged as-is?
Co-authored-by: Loris Leiva <loris.leiva@gmail.com>
I’ve tried fixing the issue regarding the import of The issue is that
Right now, the JS SDK correctly imports Additionally, even if that was right, the ESM module exported by
So the possible solutions are:
|
Co-authored-by: Loris Leiva <loris.leiva@gmail.com>
compression: input.compression, | ||
|
||
// TODO(jon): Read API doesn't return this info | ||
collectionDetails: null, |
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.
this is a restriction from the Read API standpoint, we should definitely get grouping info from the API
there's a lot of gaps to cover, so i figured it'd be worth getting a spot-check on the structure of this support before going too much further. this PR includes:
metaplex.nfts().create({ tree: <insert-tree-address>})
to mint a compressed NFT into an initialized Merkle tree and into a collection.metaplex.nfts().findByAssetId
open questions that would be valuable to answer:
known things that are missing that i want to add, provided there's good feedback on the approach so far:
transfer
NFTsfindByCreator
,findByOwner
this package was tested with a script that looks roughly like:
here's a sample transaction that this code ran
works too