Skip to content

Conversation

optke3
Copy link
Contributor

@optke3 optke3 commented Aug 4, 2023

Summary

  • Pyth Sui JS SDK
  • SuiPriceServiceConnection class
  • SuiRelay example for fetching update data and updating a price feed
  • helpers for updating price feed using accumulator message / batch price attestation, getting latest package ids given Wormhole or Pyth state, parsing VAA from accumulator msg, etc.
  • add updated examples to README

TODOs

  • publish the pyth-sui-js npm package so that it is available to users via npm install --save @pythnetwork/pyth-sui-js

@vercel
Copy link

vercel bot commented Aug 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
example-oracle-amm ⬜️ Ignored (Inspect) Visit Preview Aug 15, 2023 8:41am
xc-admin-frontend ⬜️ Ignored (Inspect) Visit Preview Aug 15, 2023 8:41am

@optke3 optke3 changed the title [Sui 21/x] - js sdk [Sui 21/x] - pyth-sui-js Aug 4, 2023
@optke3 optke3 changed the title [Sui 21/x] - pyth-sui-js [Sui 21/x] - pyth-sui-js SDK Aug 4, 2023
@optke3 optke3 requested a review from m30m August 9, 2023 19:00
@optke3 optke3 marked this pull request as ready for review August 9, 2023 19:00
@optke3 optke3 requested review from jayantk and ali-behjati August 9, 2023 19:00
Copy link
Collaborator

@ali-behjati ali-behjati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the most complex sdk that we have had so far! I left some inline comments.

Also on sui/scripts/pyth:

  • Can we remove Yarn in favor of our package?
  • It's good to make it a local package which relies local dependencies instead of their released version

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not exists when we have lerna. We should instead add this package to the global package.json file

import { SuiPriceServiceConnection } from "../index";

const argv = yargs(hideBin(process.argv))
.option("price-feed", {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ideally it's better to accept multiple price feeds

@@ -0,0 +1,2 @@
export { SuiPriceServiceConnection } from "./SuiPriceServiceConnection";
export { SuiPythClient } from "./client";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to export other useful types as well like this

* @param priceIds Array of hex-encoded price ids.
* @returns Array of price update data.
*/
async getPriceFeedsUpdateData(priceIds: HexString[]): Promise<number[][]> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

number[] is a weird type. I prefer passing arrays of buffers instead and do the conversion on the edges as Buffer is more known as the data blob type.

Comment on lines 285 to 296
const trailingPayloadSize = accumulatorMessage.slice(6, 7)[0];
const vaaSizeOffset =
7 + // header bytes (header(4) + major(1) + minor(1) + trailing payload size(1))
trailingPayloadSize + // trailing payload (variable number of bytes)
1; // proof_type (1 byte)
const vaaSizeBytes = accumulatorMessage.slice(
vaaSizeOffset,
vaaSizeOffset + 2
);
const vaaSize = vaaSizeBytes[1] + 16 * vaaSizeBytes[0];
const vaaOffset = vaaSizeOffset + 2;
return accumulatorMessage.slice(vaaOffset, vaaOffset + vaaSize);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use Buffer you can use buffer.readUint...

Comment on lines +155 to +158
if (updates.length > 1) {
throw new Error(
"SDK does not support sending multiple accumulator messages in a single transaction"
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for the sake of simplicity of the sdk and the fact that it's rare.

Comment on lines +127 to +129
throw new Error(
`Price feed ${feedId} not found, please create it first`
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we don't want to create this ourself each time. Isn't it better to add the object creation part to the tx if possible?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try but last time I checked it was not that easy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PriceInfoObjects are shared objects and should be specified by user as inputs to the transaction. Therefore, it's not possible to create and use them in the same transaction.

);
}
priceInfoObjects.push(priceInfoObjectId);
const coin = tx.splitCoins(tx.gas, [tx.pure(1)]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it assuming the update cost is always 1 per price feed? I think we should query it from on-chain.

Copy link
Collaborator

@ali-behjati ali-behjati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'm still a bit unclear on the createPrice part and the single sized update data but i don't have much context to judge.

Please add this package to the global package.json file before merging.
https://github.com/pyth-network/pyth-crosschain/blob/main/package.json

@m30m m30m merged commit 55129e5 into main Aug 15, 2023
@m30m m30m deleted the sui_js_sdk branch August 15, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants