-
Notifications
You must be signed in to change notification settings - Fork 267
feat(evm/sdk/js): add pyth filler #2832
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
da11879
to
5e37390
Compare
5e37390
to
b443c30
Compare
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.
Pull Request Overview
This PR revamps the deprecated EVM SDK to automatically detect and bundle Pyth price update calls into EVM transactions.
- Added two tracer utilities (
trace-call-many
anddebug-trace-call
) to extract Pyth price feed IDs from RPC traces. - Implemented
fillPythUpdate
infiller.ts
to iteratively trace, fetch data from Hermes, compute fees, and produce bundled calls. - Updated exports (
index.ts
) and documentation (README.md
), removed old examples, and bumped package to2.0.0-alpha
.
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/tracer/trace-call-many.ts | New batch tracer using trace_callMany to pull Pyth feed IDs |
src/tracer/debug-trace-call.ts | Recursive tracer using debug_traceCall to extract nested feed IDs |
src/filler.ts | Core fillPythUpdate implementation orchestrating trace + Hermes |
src/multicall3-bundler.ts | Bundler combining Pyth update and original call via Multicall3 |
src/index.ts | Simplified exports exposing only new utilities |
README.md | Updated quickstart and examples for fillPythUpdate utility |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (3)
target_chains/ethereum/sdk/js/README.md:25
- Documentation says
fillPythUpdate
usestrace_callMany
by default, but in codedefaultConfig.method
is set todebug_traceCall
. Align the docs with the actual default or update the default behavior in code.
This function uses the `trace_callMany` method by default but can be used with `debug_traceCall` and a bundler as well. See the example below for more information.
target_chains/ethereum/sdk/js/README.md:49
- In the example code, a comma is missing after the
method: "trace_callMany"
property, which makes the snippet invalid JavaScript.
method: "trace_callMany"
target_chains/ethereum/sdk/js/src/filler.ts:135
- [nitpick] Consider adding unit tests for
fillPythUpdate
(and the tracer helpers) to verify that price feed extraction and iterative update bundling behave correctly under different scenarios.
export async function fillPythUpdate<
} | ||
} | ||
if (trace.calls === undefined) { | ||
return new Set(); |
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.
When trace.calls
is undefined, this returns a new empty set and discards any feeds collected in result
. It should return the existing result
set instead.
return new Set(); | |
return result; |
Copilot uses AI. Check for mistakes.
bundler: Bundler; | ||
/** Maximum number of iterations to find all required price feeds. Default is 5. | ||
* Each iteration traces the current transaction to find new Pyth price feed calls. */ | ||
maxIter: number; |
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.
Why do we need to iterate multiple times to find price feeds?
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.
function increment() public returns (int256) {
// Fetch the latest price from Pyth
PythStructs.Price memory ethPrice = pyth.getPriceNoOlderThan(0xff61491a931112ddf1bd8147cd1b641375f79f5825126d665480874634fd0ace, 30);
PythStructs.Price memory btcPrice = pyth.getPriceNoOlderThan(0xe62df6c8b4a85fe1a67db44dc12de5db330f7ac66b72dc658afedf0f4a415b43, 30);
}
Consider a call to the function above is given to the method. The problem is that with the first trace call, the execution reverts at the first price fetch request and we only identify ETH price feed id. We need to iterate again, by bundling the update and call to pass that line to get the BTC price feed id.
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.
Looks good, mostly minor nits / suggestions, also the improvements to fillPythUpdate
we'd discussed on slack
import { createPublicClient, http } from "viem"; | ||
import { optimismSepolia } from "viem/chains"; | ||
|
||
async function example() { |
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.
Minor thing but since top-level await exists, you don't need the code here to live in a function, and it kind of distracted me a bit when I was reading through the docs here -- when I saw this my immediate thought was "what is the example function returning and where do I use it" -- I'd just put the body at the top level to minimize the distraction.
data: "0xd09de08a", // Your transaction calldata | ||
from: "0x78357316239040e19fC823372cC179ca75e64b81", | ||
}, | ||
"0x0708325268df9f66270f1401206434524814508b", // Pyth contract address |
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.
Can we export this as a const? I know that contract manager isn't in a great state but even just manually copying it over seems to be a better experience for now
from: "0x78357316239040e19fC823372cC179ca75e64b81", | ||
}, | ||
"0x0708325268df9f66270f1401206434524814508b", // Pyth contract address | ||
"https://hermes.pyth.network", // Hermes endpoint |
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.
Can we make this a const too? Or better yet, can we make this function just automatically derive these two fields from the client
, but allow overriding them in the options?
"typescript": "^4.6.3", | ||
"web3": "^1.8.2", | ||
"yargs": "^17.4.1" | ||
"ts-node": "catalog:", |
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 this dependency used anywhere?
/** | ||
* Union type for tracing configuration options | ||
*/ | ||
export type Config = DebugTraceCallConfig | TraceCallManyConfig; |
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.
You can make this type a lot more concise:
export type Config = { maxIter: number } & (
| { method: "trace_callMany" }
| { method: "debug_traceCall", bundler: Bundler }
);
call: CallRequest, | ||
pythContractAddress: Address, | ||
hermesEndpoint: string, | ||
config?: Config, |
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.
minor nit but a better way to write the default is:
export async function fillPythUpdate(
// ... other args
config: Config = {
method: "debug_traceCall",
bundler: multicall3Bundler,
maxIter: 5
}
)
}; | ||
export * from "./filler"; | ||
export * from "./multicall3-bundler"; | ||
export * from "./pyth-abi"; |
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.
As we'd discussed, barrel exports aren't great, I would consider just putting these into the same file -- I know it feels nice to have smaller separated files, but it does add fs read cost at load time for apps that use it. It's not a huge thing, but it adds up across the whole js package ecosystem, so the more packages that stop using barrel exports, the better.
pythContractAddress: Address, | ||
ignoreParsingErrors = false, | ||
): Set<`0x${string}`> { | ||
const result = new Set<`0x${string}`>(); |
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.
You can use Address
from viem instead of 0x${string}
} catch { | ||
if (!ignoreParsingErrors) { | ||
throw new Error( | ||
`Failed to decode calldata: ${trace.input}. Make sure correct Pyth contract address is used.`, | ||
); | ||
} | ||
} |
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.
} catch { | |
if (!ignoreParsingErrors) { | |
throw new Error( | |
`Failed to decode calldata: ${trace.input}. Make sure correct Pyth contract address is used.`, | |
); | |
} | |
} | |
} catch (error: unknown) { | |
if (!ignoreParsingErrors) { | |
const thrownError = new Error( | |
`Failed to decode calldata: ${trace.input}. Make sure correct Pyth contract address is used.`, | |
); | |
thrownError.cause = error; | |
throw thrownError; | |
} | |
} |
this way you don't lose the context of the underlying error
Summary
This change revamps the deprecated EVM SDK to add a utilty for EVM to fill a call with the Pyth data it needs automatically. Under the hood it performs trace calls to detect Pyth contract usage and extracts the IDs from there, then fetches the update data from Hermes and creates a pyth update call.
This change is the alpha version and is subject to change. In the subsequent PRs more improvements will come. Also I'm going to improve the packaging (like dual export, file structure) later.
Please review the change commit-by-commit (the first commit has just removals, and the second one has actual changes).
Rationale
This change makes integrations to Pyth in the pull model easier.
How has this been tested?