Skip to content

Conversation

@darunrs
Copy link
Contributor

@darunrs darunrs commented Apr 27, 2025

Summary

  • Removes the publisher ID field from the publisher update sent by publishers. Publishers don't need know what their ID is, and its better to not ask them to self report it.
  • Removes update index from feed update context. It's unnecessary, and I will make assertions to make sure the ordering and length of the array match the publisher updates vec exactly.
  • Moved transaction envelope definition to publisher SDK to skip one decode step. Also added comments, adopting Pavel's comment styling in the Pyth Lazer repo.
  • Switched the parser to use protoc and then added the argument to generate rust docs for the generated types, as per Pavel's PR.

Rationale

Reduces some burden on publishers by not requiring them to know their own IDs. The update index field was also confusing. It's better to write thoroughly tested code and make some assumptions instead. The added rust docs should also help with understanding the generated types.

How has this been tested?

  • Current tests cover my changes
  • Added new tests
  • Manually tested the code

@vercel
Copy link

vercel bot commented Apr 27, 2025

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

Name Status Preview Comments Updated (UTC)
api-reference ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 14, 2025 0:06am
component-library ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 14, 2025 0:06am
entropy-debugger ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 14, 2025 0:06am
entropy-explorer ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 14, 2025 0:06am
insights ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 14, 2025 0:06am
proposals ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 14, 2025 0:06am
staking ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 14, 2025 0:06am

Copy link
Contributor

@merolish merolish left a comment

Choose a reason for hiding this comment

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

So we determine the publisher id from auth?

@darunrs
Copy link
Contributor Author

darunrs commented Apr 28, 2025

So we determine the publisher id from auth?

Yep, Lazer will determine their ID based on their public key/access key. Also sorry I will do some more things with this PR actually. Will ping when I'm done.

@darunrs darunrs requested a review from a team as a code owner May 9, 2025 16:23
Copy link
Contributor

@Riateche Riateche left a comment

Choose a reason for hiding this comment

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

Looks good! See one comment below.

// Submitted over Message Queue to be read by rest of Pyth Lazer service.
message PayloadContext {
// [required] ID of publisher based on the access token used to connect
optional uint32 publisher_id = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

The next option we will likely add to PayloadContext.context is a governance update context. That one doesn't have a publisher_id, so I think it makes more sense to move this publisher_id field to PublisherUpdateContext message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't imagine that we would have a permissionless endpoint on Relayer. If permissioned, we need some way for governance to authn/z with Relayer, at which point we could read their ID. We may not necessarily want to. Maybe the ID takes another form.

Just curious what you had in mind. For now, I don't mind moving it into publisher context. I had a comment on your PR adding governance on how we determine the governance source.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed in Slack. If we add publisher's public key to the payload, I guess we won't need publisher id in the context at all.

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.

4 participants