-
Notifications
You must be signed in to change notification settings - Fork 274
feat(pyth-lazer-agent) Allow conditionally disabling success responses #2896
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…etwork/pyth-crosschain into feat/pyth-lazer/agent/jrpc-notifications-1
…eat/pyth-lazer/agent/jrpc-notifications-1
…eat/pyth-lazer/agent/jrpc-notifications-1
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.
Refactor looks good, nice.
### From cargo | ||
``` | ||
# Download the cargo package | ||
cargo install pyth-lazer-agent | ||
|
||
# Add .cargo/bin to PATH | ||
export PATH="$PATH:~/.cargo/bin" | ||
|
||
# Run the agent | ||
pyth-lazer-agent --help | ||
``` | ||
|
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.
Would also show the actual run usage which is just pyth-lazer-agent --config /path/to/config.toml
.
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.
imho pyth-lazer-agent --help
already explains how to use it in a lot more detail than we need to here
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.
Nice! I know many publishers were hesitant to have acks sent over the legacy WS connection as they've all noticed slowdowns so this doesn't totally surprise me.
@@ -1,4 +1,4 @@ | |||
relayer_urls = ["wss://relayer-0.pyth-lazer.dourolabs.app/v1/transaction", "wss://relayer-0.pyth-lazer.dourolabs.app/v1/transaction"] | |||
relayer_urls = ["wss://relayer.pyth-lazer-staging.dourolabs.app/v1/transaction", "wss://relayer-1.pyth-lazer-staging.dourolabs.app/v1/transaction"] |
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.
It's probably worth breaking these out as consts so that we can clearly understand they're "STAGING_RELAYER_1_WS_URL" or something.
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.
in config? does toml have support for some consts i'm missing?
{ | ||
Ok(_) => { | ||
if let Some(request_id) = request_id { | ||
send_json( |
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.
Do we respond like this on every update or just some? I think acks should be on some timer rather than driven by events from the publisher. Or maybe that can be configured by the publisher. Like they can set it to ack each update when testing, change it to interval when its running well, and disable them when fully running it. What do you think?
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 id
is specified we respond to each message as it's processed. We could batch the responses but I'm not sure if there's a big usecase here - most publishers don't seem to care about acks at all and just want to get the errors out. It would also be a little weird trying to fit that within json-rpc specs although not impossible. For now I'd like to keep it as is until somebody asks for it.
…eat/pyth-lazer/agent/jrpc-notifications-1
Summary
Add support for JSON-RPC notifications. Now if the
id
parameter is omitted the agent will not send acknowledgments for successful updates. Errors will still be sent back to the client, now without any id, but will include the full update to help the client correlate them with the original request.Rationale
Publishers see issues due to websocket contention largely caused by the acks being sent back to the publisher and have requested this change. PF-334
How has this been tested?