STREAM-1123: distributor client #131
Conversation
Yolley
commented
Feb 13, 2024
•
Yolley
commented
- add distributor package with create/claim/clawback methods for now
- separate check or create ATAs into an util
- remove docs from repo, move to CI
- add common package with common utilities
- add more exports for all modules
- move to pnpm as it works better with workspaces
packages/stream/solana/utils.ts
Outdated
| for (let i = 0; i < response.length; i++) { | ||
| if (!response[i]) { | ||
| ixs.push( | ||
| createAssociatedTokenAccountInstruction(invoker.publicKey!, atas[i], owners[i], mint) | ||
| ); | ||
| } | ||
| } | ||
| return ixs; |
There was a problem hiding this comment.
I dunno, is it an optimization or not, but as an option again 😄
| for (let i = 0; i < response.length; i++) { | |
| if (!response[i]) { | |
| ixs.push( | |
| createAssociatedTokenAccountInstruction(invoker.publicKey!, atas[i], owners[i], mint) | |
| ); | |
| } | |
| } | |
| return ixs; | |
| return response.reduce((result, responseItem) => { | |
| if (!responseItem) { | |
| result.push(createAssociatedTokenAccountInstruction(invoker.publicKey!, atas[i], owners[i], mint)); | |
| return result; | |
| } | |
| return result; | |
| }, []; |
There was a problem hiding this comment.
I dunno, is it an optimization or not, but as an option again 😄
It's harder to read. 🌚
| clusterUrl, | ||
| cluster = ICluster.Mainnet, |
There was a problem hiding this comment.
When I touched our js-sdk api for the first time I had an issue with this setup.
As clusterUrl and cluster are separated, it wasn't obvious for me that I have to pass them both when I switch to ``devnet` for instance. Probably we should combine them to an one parameter?
like
type ClusterConnectionInput = {
clusterUrl: string,
cluster: <some predefined union type>
}
There was a problem hiding this comment.
Well, the idea is that you can't really extra cluster from URL, so that's why you send them both. And I mean they are part of the constuctors, what's the problem? We can rename it to clusterType if it makes it more obvious. I am opposed to adding extra types, especially in costructors.
| const mintAccount = await getMint(this.connection, mint); | ||
| const distributorPublicKey = getDistributorPda(this.programId, mint, data.version); | ||
| const tokenVault = await ata(mint, distributorPublicKey); | ||
| const senderTokens = await ata(mint, sender.publicKey); |
There was a problem hiding this comment.
can we somehow make these requests in parallel?
All this RPC calls can be slow as we see always 😄
There was a problem hiding this comment.
can we somehow make these requests in parallel? All this RPC calls can be slow as we see always 😄
Seems like it requires refactoring through the whole sdk then as we do all these calls in our core client also.
packages/stream/solana/distributor/generated/accounts/MerkleDistributor.ts
Outdated
Show resolved
Hide resolved
packages/stream/solana/distributor/generated/instructions/newDistributor.ts
Outdated
Show resolved
Hide resolved
Yolley
force-pushed
the
oleg/feature/STREAM-1123_distributor_integration
branch
from
488a82f
to
b37a381
Compare
| @@ -25,12 +49,13 @@ | |||
| "jest": "29.3.1", | |||
| "prettier": "2.8.1", | |||
| "ts-jest": "29.0.3", | |||
| "typescript": "4.6.4" | |||
| "typescript": "^4.9.5" | |||
| }, | |||
| "dependencies": { | |||
There was a problem hiding this comment.
thinking about one package.json per repo. Will it be easier to manage cross-package dependencies sync in this case? 3rd parties like @solana/x, bn.js etc should be the same across all packages?
There was a problem hiding this comment.
As I understand, lerna promotes each package having their dependencies listed individually within its own package.json. By looking at other monorepoes like https://github.com/suiet/wallet-kit or https://github.com/aptos-labs/aptos-wallet-adapter/tree/main I see that they also for every package list its own dependencies, even if they are shared across multiple packages.
| @@ -1,6 +1,6 @@ | |||
| import { Wallet } from "ethers"; | |||
|
|
|||
| import { StreamflowAptos, StreamflowEVM, StreamflowSolana, StreamflowSui } from "../../"; | |||
| import { StreamflowAptos, StreamflowEVM, StreamflowSolana, StreamflowSui } from "../../index"; | |||
There was a problem hiding this comment.
🤔 auto-refactoring feature replaced it? btw shouldn't be required
There was a problem hiding this comment.
There was a problem with running tests locally when dist was built, so I decided just to import from index here.
| return this.client.signAndExecuteTransactionBlock({ | ||
| ...input, | ||
| // @ts-ignore |
There was a problem hiding this comment.
what error do you have here?
There was a problem hiding this comment.
There are several sui.js library versions provided from different libraries and that makes it error out here. Basically we need to get rid of manahipppo/aptos-wallet-adapter to get rid of older versions, but it's a huge refactoring, not part of this feature.
| signer: this.wallet, | ||
| }); | ||
| } | ||
| // @ts-ignore |
Yolley
force-pushed
the
oleg/feature/STREAM-1123_distributor_integration
branch
from
3a4612c
to
b3264d5
Compare
1 similar comment
