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

Add airdrop support via Gumdrop #73

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add airdrop support via Gumdrop #73

wants to merge 10 commits into from

Conversation

ngundotra
Copy link
Collaborator

Closes #71

@ngundotra ngundotra requested review from jarry-xiao, austbot and lwus May 24, 2022 15:50
}

// Claim a compressed NFT
pub fn claim_bubblegum<'info>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

It does seem like there's some repeated code in all these claim_xxx type methods and maybe it could be refactored to do all the similar stuff in a helper function and then call issue_mint_nft, issue_compressed_nft, etc.

But that is fine if that is out of scope. I understand this is not a gumdrop specific PR but just adding using similar established paradigms.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unless @austbot or @jarry-xiao say otherwise, I'll assume this structure will be reviewed more in depth upon merging back into mpl

/// Creates a new [MerkleDistributor].
/// After creating this [MerkleDistributor], the account should be seeded with tokens via
/// delegates
pub fn new_distributor_compressed(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there be a comment that says something like "tree is created here to make sure more NFTs cannot be added after the airdrop"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. I prefer being explicit about the pros and cons of using the tree authorities in this way. It made sense to me to limit append permissions to instructions issued via gumdrop, but I would like to open a discussion.

@@ -0,0 +1,1815 @@
//! Program for distributing tokens efficiently via uploading a Merkle root.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overall I'm mostly looking at the new functions added to support bubblegum, not doing detailed review of gumdrop program. I also assume if anyone wants to be very detail oriented about the new instruction handler APIs, those details can be discussed when this is back ported to the metaplex program library.

creator: PublicKey,
share: number,
};
type Metadata = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ultra nit: Should this be TokenMetadata?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. I'll rename

creators: Creator[],
};

const METADATA = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ultra nit: Should this be TOKEN_METADATA or something slightly more than just metadata?

@@ -0,0 +1,77 @@
import { keccak_256 } from 'js-sha3';

export class MerkleTree {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I really like this class and how it seems to define how the test is built. If the test were able to be written without this object, I think the test wouldn't have been as clean.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All credit to @lwus

Copy link
Collaborator

@samwise2 samwise2 left a comment

Choose a reason for hiding this comment

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

Looks great! Added some nits about the test

}


function getMerkleRollAccountSize(maxDepth: number, maxBufferSize: number): number {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can probably just import this from ./merkle-roll-serde like the bubblegum test

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Fixed

const txId = await gumdrop.provider.send(tx, [merkleRollKeypair, payer], {
skipPreflight: true
});
console.log(`${txId} sent`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
console.log(`${txId} sent`);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

return new Promise(resolve => setTimeout(resolve, ms));
}

async function succeedOrThrow(txId: string, connection: Connection) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great. Maybe we can move it to ./utils.ts to be reused?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

import { MerkleTree } from './gumdropTree';
import { BinaryWriter } from 'borsh'

async function delay(ms: number) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

move to ./utils.ts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@ngundotra
Copy link
Collaborator Author

Waiting to merge this in until #77 is addressed

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.

[Gumdrop] needed for batch mint
3 participants