Add support for claim limit for distributor #253
Conversation
| public async closeClaim(data: ICloseClaimData, extParams: IInteractSolanaExt): Promise<ITransactionResult> { | ||
| const ixs = await this.prepareCloseClaimInstructions(data, extParams); | ||
| const { tx, hash, context } = await prepareTransaction(this.connection, ixs, extParams.invoker.publicKey); | ||
| const signature = await wrappedSignAndExecuteTransaction( |
There was a problem hiding this comment.
NIT: I think it makes sense to get rid of these long wrappedSignAndExecuteTransactions calls and provide a private method to make it cleaner like so https://github.com/streamflow-finance/js-sdk/blob/master/packages/launchpad/solana/client.ts#L427
There was a problem hiding this comment.
Don't see how this would make it cleaner - I would just be adding more code by adding this already existing util function as a private method in this class, is it a better approach - idk
| const distributorPublicKey = new PublicKey(data.id); | ||
| const distributor = await MerkleDistributor.fetch(this.connection, distributorPublicKey); | ||
|
|
||
| const claimantPublicKey = new PublicKey(data.claimant); |
There was a problem hiding this comment.
NIT: We now have a neat pk method that can receive either publickey or string and alsways returns PublicKey https://github.com/streamflow-finance/js-sdk/blob/master/packages/common/solana/public-key.ts#L3
| program: this.programId, | ||
| }; | ||
|
|
||
| const closeClaimArgs: CloseClaimArgs = { |
There was a problem hiding this comment.
All these parameters are optional so may be it would also make sense to make them optional here (admin should be able to close claim without them, also a potential user might close their claim after calling newClaim - while it's out of the scope for the task we're doing it for, I think would be neat to have it supported from the start).
| claimsClosable: data.claimsClosable, | ||
| claimsClosableByAdmin: data.claimsClosableByAdmin, | ||
| claimsClosableByClaimant: data.claimsClosableByClaimant, | ||
| claimsLimit: new BN(data.claimsLimit || 0), |
There was a problem hiding this comment.
claimsLimit is u16, so no need to convert it to BN actually, we use BN for u64 as its value is larger what ecma number supports.
| @@ -42,6 +46,10 @@ export interface ClaimStatusJSON { | |||
| closed: boolean; | |||
| /** Distributor account for which ClaimStatus was created, useful to filter by in get_program_accounts */ | |||
| distributor: string; | |||
| /** Number of time amount has been claimed */ | |||
| claimsCount: string; | |||
There was a problem hiding this comment.
It will be returned as number I believe as it's u16.
| @@ -181,6 +203,8 @@ export class ClaimStatus { | |||
| lastAmountPerUnlock: new BN(obj.lastAmountPerUnlock), | |||
| closed: obj.closed, | |||
| distributor: new PublicKey(obj.distributor), | |||
| claimsCount: new BN(obj.claimsCount), | |||
There was a problem hiding this comment.
no need to convert to BN imo.
| /** Whether claims are closable by the claimant or not */ | ||
| claimsClosableByClaimant: boolean; | ||
| /** Limit number of claims */ | ||
| claimsLimit: string; |
There was a problem hiding this comment.
the same thing with number
| canUpdateDuration: obj.canUpdateDuration, | ||
| totalAmountUnlocked: new BN(obj.totalAmountUnlocked), | ||
| totalAmountLocked: new BN(obj.totalAmountLocked), | ||
| lastDurationUpdateTs: new BN(obj.lastDurationUpdateTs), | ||
| totalClaimablePreUpdate: new BN(obj.totalClaimablePreUpdate), | ||
| clawedBackTs: new BN(obj.clawedBackTs), | ||
| claimsClosableByClaimant: obj.claimsClosableByClaimant, | ||
| claimsLimit: new BN(obj.claimsLimit), |
There was a problem hiding this comment.
extra bn conversion not needed imo
| @@ -19,18 +24,33 @@ export interface CloseClaimAccounts { | |||
| program: PublicKey; | |||
| } | |||
|
|
|||
| export function closeClaim(accounts: CloseClaimAccounts, programId: PublicKey = PROGRAM_ID) { | |||
| export const layout = borsh.struct([ | |||
There was a problem hiding this comment.
All these values are optional - it's really important to mark it in the borsh struct as optional field has an additional byte at the start of the value which signals whether the value is presented or not.
| claimsClosable: boolean; | ||
| claimsClosableByAdmin: boolean; | ||
| claimsClosableByClaimant: boolean; | ||
| claimsLimit: BN; |
There was a problem hiding this comment.
It can be represented as number.
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "@streamflow/common", | |||
| "version": "7.3.3", | |||
| "version": "8.0.0", | |||
There was a problem hiding this comment.
why major? these changes are backward compatible if I understand correctly
There was a problem hiding this comment.
well we did change some exposed interfaces (claimsClosable renamed to claimsClosableByAdmin in MerkleDistributor and ICreateDistributorData for example) so it is breaking... however maybe this does not warrant such a change, I can keep the exposed interface the same for SDK to keep it backwards compatible and have this be a minor change now that I reconsider it
There was a problem hiding this comment.
@Yolley tagging you since you also asked me about this
Blog by Tom, the creator of SemVer - TL;DR ANY breaking change (and this is one) should merit a major version bump - we are not going to run out of numbers any time soon
There was a problem hiding this comment.
@tatomir-streamflow I agree with Roman as these methods are also not used at all by any client, so I think that having another major version bump for them is too much.
packages/common/solana/public-key.ts
Outdated
| @@ -1,5 +1,10 @@ | |||
| import { PublicKey } from "@solana/web3.js"; | |||
|
|
|||
| /** | |||
| * Converts a string or PublicKey to a PublicKey object. | |||
| * @param key - The input key as a string or PublicKey. | |||
There was a problem hiding this comment.
the param name is address. 🌚
Exposes the
claimsLimitargument for distributor creation as well as thecloseClaimmethod.Protocol PR