-
Notifications
You must be signed in to change notification settings - Fork 65
Add an instruction plan for creating a mint #109
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
Conversation
clients/js/src/createMint.ts
Outdated
|
|
||
| // RPC `getMinimumBalanceForRentExemption` for 82 bytes | ||
| // Hardcoded to avoid requiring an RPC request each time | ||
| const minimumBalanceForMint = 1461600; |
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.
To flag this, I think it's wasteful/annoying to require you to pass Rpc<GetMinimumBalanceForRentExemption> API and re-calculate this every time, given this value doesn't change (without a server change).
Can change to do that though!
I added an optional minimumBalanceForMintOverride override in case you want to use this with a Solana network with different logic, or to account for a hypothetical change in future before the library is updated, but intended most usage to just not have to think about it.
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.
Yeah I think that's a great idea. IIRC we even discussed the possibility of adding a sync helper like that to Kit. Not sure where we landed on that though.
I also like the override parameter. What do you think of calling it mintBalance or mintLamports instead? I'm not a fan of "minimum balance" here because the created account will have this exact balance after the transaction. Also "override" might be redundant since it's optional.
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 like mintLamports, and good to drop the override!
Edit: Went with mintAccountLamports
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.
On the Kit sync helper, my issue is anza-xyz/kit#777
I don't think we really discussed it but I still think this would be worth having. Feels like the risk of this changing frequently is very low.
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.
Yeah I agree. I have a feeling that Steven wanted to first figure out a way to offer slot-specific or feature-specific builds of Kit so that, if this was to change in the future, we could gracefully offer two version of the library with different values based on the current state of the cluster.
lorisleiva
left a comment
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.
Looks great, thanks for that! I've made a few small comments.
clients/js/src/createMint.ts
Outdated
|
|
||
| // RPC `getMinimumBalanceForRentExemption` for 82 bytes | ||
| // Hardcoded to avoid requiring an RPC request each time | ||
| const minimumBalanceForMint = 1461600; |
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.
Yeah I think that's a great idea. IIRC we even discussed the possibility of adding a sync helper like that to Kit. Not sure where we landed on that though.
I also like the override parameter. What do you think of calling it mintBalance or mintLamports instead? I'm not a fan of "minimum balance" here because the created account will have this exact balance after the transaction. Also "override" might be redundant since it's optional.
| export const createDefaultSolanaClient = (): Client => { | ||
| const rpc = createSolanaRpc('http://127.0.0.1:8899'); | ||
| const rpcSubscriptions = createSolanaRpcSubscriptions('ws://127.0.0.1:8900'); | ||
| return { rpc, rpcSubscriptions }; | ||
| }; |
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.
Also happy to refactor this slightly in the tests so we can bake a sendAndConfirm function in the Client by accepting a feePayer.
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'd be good with that too, just wasn't sure if any tests are eg using multiple fee payers, or if we might want that kind of flexibility later. Either way I don't think the duplication is a concern in test helpers ATM.
599539e to
004b844
Compare
004b844 to
f52ce77
Compare
This PR adds a new instruction plan to create a mint. This combines the
createAccountinstruction from the system program, with theinitializeMint2instruction from the token program.I've structured it to take input in a similar way to generated instructions.
I've also added test helpers for a transaction plan/executor, using the same behaviour as the existing test helpers.
Example: