-
Notifications
You must be signed in to change notification settings - Fork 37
feat: distributor protocol level fees #334
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
|
Alpha versions of packages are published 📦 PR: #334 |
| "packages": [ | ||
| "packages/*" | ||
| ], | ||
| "version": "11.0.0", |
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.
lfg 🔥
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 number is going up faster than the scale in my room during a week at an all inclusive in Turkey
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.
🌚
| /** | ||
| * Build a partner oracle program without a wallet, just to fetch accounts. | ||
| */ | ||
| export function buildPartnerOracle(connection: Connection): Program<PartnerOracle> { |
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.
are we not gonna expose anything else for fee fetching for use in the App? e.g. like we do for vesting
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.
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.
And in the common package I just expose the program since it's shared across protocols.
| "packages": [ | ||
| "packages/*" | ||
| ], | ||
| "version": "11.0.0", |
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 number is going up faster than the scale in my room during a week at an all inclusive in Turkey
| const feeConfig = await this.getFeeConfig(); | ||
| const nowTs = new BN(Math.trunc(Date.now() / 1000)); | ||
| return ( | ||
| feeConfig.partners.filter((partner) => partner.pubkey.equals(pubkey) && partner.expiryTs.gt(nowTs))[0] ?? 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.
should it throw if 1+ partners are found? just as a sanity check so we don't forget to clean some stale stuff
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.
It should never happen as the protocol does not allow it when writing fees, but I'll add a throw to mark it as unexpected behavior then.
packages/distributor/solana/types.ts
Outdated
| export type MerkleDistributor = MerkleDistributorAccountTypes["merkleDistributor"]; | ||
|
|
||
| type IdlInstruction<IDL extends Idl, Name extends IDL["instructions"][number]["name"]> = Extract< | ||
| IDL["instructions"][number], | ||
| { name: Name } | ||
| >; | ||
|
|
||
| type AccountsOfMethod<M extends keyof Program<MerkleDistributorIDL>["methods"]> = Parameters< | ||
| ReturnType<Program<MerkleDistributorIDL>["methods"][M]>["accounts"] | ||
| >[0]; | ||
| type ArgsOfMethod<M extends keyof Program<MerkleDistributorIDL>["methods"]> = Parameters< | ||
| Program<MerkleDistributorIDL>["methods"][M] | ||
| >; |
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.
a minor, but would be cool to have it globally like in common/anchor for instance since we're increasing anchor inference usage everywhere. Might be useful 🤷♂️ but not forcing it.
Just don't want it to be MIA later if a need appear again
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.
Makes sense, tho I put it to common/solana/types for now - as solana package is not exactly a web3js wrapper, it just contains common solana utilities.
| // const [feeOracle] = PublicKey.findProgramAddressSync( | ||
| // [Buffer.from(b"airdrop_config")], | ||
| // new PublicKey(PARTNER_ORACLE_PROGRAM_ID[cluster]), | ||
| // ); |
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.
outdated?
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.
Just keeping how the address was derived as it's a PDA>
| [ICluster.Testnet]: "pardoTarcc6HKsPcbXkVycxsJsoN9QEzrdHgVdHAGY3", | ||
| [ICluster.Local]: "pardoTarcc6HKsPcbXkVycxsJsoN9QEzrdHgVdHAGY3", |
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.
Curious do we ever use these two? in all other places as well
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.
Nop, never, prob makes sense to remove the unused clusters are at some point lol, may be in v23
| const { distributor, mint, clawbackReceiver, tokenProgram, tokenVault, admin } = accounts; | ||
| const { mint, clawbackReceiver, tokenProgram, admin } = accounts; | ||
| const distributorKey = getDistributorPda(this.merkleDistributorProgram.programId, pk(accounts.mint), data.version); | ||
| const tokenVaultKey = getAssociatedTokenAddressSync(pk(mint), pk(admin!), true, pk(tokenProgram)); |
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.
Is it anchor type inference that is broken and requires to enforce non nullish type or the NewDistributorAccounts helper?
Is it even allowed to call the getNewDistributorInstruction function w/o an admin account?
| const tokenVaultKey = getAssociatedTokenAddressSync(pk(mint), pk(admin!), true, pk(tokenProgram)); | |
| const tokenVaultKey = getAssociatedTokenAddressSync(pk(mint), pk(admin), true, pk(tokenProgram)); |
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.
the line below has it too
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.
So, admin is a signer which can be inferred from a transaction by anchor ts - that's why it's an optional account. In our case we always set it tho as we pass signer to all instruction related methods.
Makes sense to wrap it in Required then I guess since admin is the only optional account in this structure.
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 make types as close to the real life as possible - it is one of ways to make sdks "easier to use for outsiders" (and for us actually too)
…helpers to common, throw on found partners > 1

getFeesandgetDefaultFeesmethods to fetch fees beforehand:BREAKING CHANGE: sdk no longer exports distributor related classes, they are only exported as types, i.e.:
BREAKING CHANGE: when the protocol is deployed,
newDistributorwill work only from this SDK version, since it has 2 new accounts added.