feat: transaction builder #1356
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Ignored Deployments
|
| * | ||
| * @param priceUpdateDataArray the output of the `@pythnetwork/price-service-client`'s `PriceServiceConnection.getLatestVaas`. This is an array of verifiable price updates. | ||
| */ | ||
| async withPostPriceUpdates(priceUpdateDataArray: string[]) { |
There was a problem hiding this comment.
Should we enforce that
withPostPriceUpdates, withPriceConsumerInstructions and withCloseInstructions need to be called sequentially and each one only once?
There was a problem hiding this comment.
I think it would be good to expose them. If you enforce it via different types (or markers from below) the auto complete will also guide people to use it correctly. If you have an intermediate step you can have Builder & pricesMap: Record<String, String> exposed and people can just call addInstruction or addInstructions without lambda function.
Look at here or here to see how you can impose order using some type markers. Feel free to ignore it if you think it gets complex.
There was a problem hiding this comment.
some of constraints here aren't strictly necessary right? Like in theory I could call withPriceConsumerInstructions multiple times adding different sets of instructions and the code will work (?)
My inclination would be to avoid adding constraints on usage that aren't necessary for correctness -- you're just making the builder less flexible. see also the comment below about withCloseInstructions which I think helps resolve the one correctness constraint that exists.
| * ... | ||
| * ``` | ||
| */ | ||
| async withPostPartiallyVerifiedPriceUpdates(priceUpdateDataArray: string[]) { |
There was a problem hiding this comment.
very nit: normally withX implied that you return it and i chain them together.
B.withX().withY().build(). Maybe addX be a name that doesn't imply it.
There was a problem hiding this comment.
yeah i agree with this. generally with builders there's always a question of "does this method mutate the internal state, or does it return a new builder". The with naming suggests that it returns a new builder
| * ... | ||
| * ``` | ||
| */ | ||
| async withPostPartiallyVerifiedPriceUpdates(priceUpdateDataArray: string[]) { |
There was a problem hiding this comment.
yeah i agree with this. generally with builders there's always a question of "does this method mutate the internal state, or does it return a new builder". The with naming suggests that it returns a new builder
| * | ||
| * @param priceUpdateDataArray the output of the `@pythnetwork/price-service-client`'s `PriceServiceConnection.getLatestVaas`. This is an array of verifiable price updates. | ||
| */ | ||
| async withPostPriceUpdates(priceUpdateDataArray: string[]) { |
There was a problem hiding this comment.
some of constraints here aren't strictly necessary right? Like in theory I could call withPriceConsumerInstructions multiple times adding different sets of instructions and the code will work (?)
My inclination would be to avoid adding constraints on usage that aren't necessary for correctness -- you're just making the builder less flexible. see also the comment below about withCloseInstructions which I think helps resolve the one correctness constraint that exists.
target_chains/solana/sdk/js/pyth_solana_receiver/src/PythSolanaReceiver.ts
Show resolved
Hide resolved
| this.priceFeedIdToPriceUpdateAccount[priceFeedId]; | ||
| if (!priceUpdateAccount) { | ||
| throw new Error( | ||
| `A price update account for the price feed ID ${priceFeedId} is being consumed before it was posted. Make sure to call addPostPriceUpdates or addPriceConsumerInstructions before calling addPriceConsumerInstructions.` |
There was a problem hiding this comment.
This comment is ambiguous
This PR addresses a comment in #1330. It tries to make the sdk more ergonomic by providing a transaction builder.
You can see how to use the transaction builder in the README.