feat(lazer): add governance instructions protocol #2654
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Riateche
force-pushed
the
lazer-governance-instructions
branch
from
4d1b0d3 to
846718b
Compare
| SET_SHARD_GROUP = 105; | ||
| RESET_LAST_SEQUENCE_NO = 106; | ||
| ADD_PUBLISHER = 107; | ||
| ADD_FEED = 109; |
There was a problem hiding this comment.
These are just the same numbers as in GovernanceDirective. There is gap because there is no UpdatePublisher = 108 here. I prefer to treat these values as opaque anyway.
keyvankhademi
left a comment
keyvankhademi
left a comment
There was a problem hiding this comment.
lgtm, but let's wait for someone else to approve this
darunrs
left a comment
darunrs
left a comment
There was a problem hiding this comment.
LGTM too. Various questions.
|
|
||
| // A dynamically typed value similar to `google.protobuf.Value` | ||
| // but supporting more types. | ||
| message DynamicValue { |
There was a problem hiding this comment.
I wonder if it's worth adding an impl on this in this crate to create a rust map from the type?
There was a problem hiding this comment.
Added some conversions to/from serde_value::Value.
| // rejected (e.g. if instruction #3 has been successfully processed before instruction #2 was observed, | ||
| // #2 will always be rejected). | ||
| // Sequence numbers are assigned and tracked separately for each governance source. | ||
| optional uint32 governance_sequence_no = 4; |
There was a problem hiding this comment.
Is the source specified in this message? Or does Lazer determine it somehow, like from an access key? Seems we need to know the source and its previous seq. no. in Relayer to properly validate this.
There was a problem hiding this comment.
Added source field to GovernanceInstruction.
| optional ShardFilter shard_filter = 1; | ||
| // [required] | ||
| // Note: when adding a new variant here, update `Permissions` as well. | ||
| oneof action { |
There was a problem hiding this comment.
Is this mirroring existing governance structure? Was curious why remove was part of update, when add is separated.
There was a problem hiding this comment.
Not really. I don't know how much sense there is in it, but my reasoning was as follows: I don't want to merge e.g. AddPublisher with UpdatePublisher because semantics of the publisher_id field is different: in one case it's a new id, in another case it has to be an existing id. I can merge UpdatePublisher and RemovePublisher though, because semantics of the publisher_id field for them is the same.
From the perspective of different permission groups, it also kind of makes sense. If I have UpdatePublisher permission, even if I don't have permission to remove a publisher, I can edit it instead so it becomes unusable. So there is no reason for a separate RemovePublisher permission. On the other hand, AddPublisher permission is clearly less powerful than Update or Remove because with Add permission you can only add new publishers, not modify existing ones. In practice it's mostly irrelevant though.
| // strictly sequential (i.e. gaps are allowed). Each shard separately keeps track of the last executed | ||
| // governance instruction and will reject instructions with the same or smaller sequence no. | ||
| // Note that if instructions are received out of order, some of them may become permanently | ||
| // rejected (e.g. if instruction #3 has been successfully processed before instruction #2 was observed, |
There was a problem hiding this comment.
This is a good callout. The seq. no. prevents accidental replays and such. Is it possible for governance to receive confirmations before sending the next instruction or are we just at the mercy of network latencies?
There was a problem hiding this comment.
We could create an endpoint the returns latest governance seq no for a source. This would allow the sender to verify that the previous message has been processed.
| } | ||
| } | ||
|
|
||
| // Create a new shard. Shard name will be determined by the value of `GovernanceDirective.filter`. |
There was a problem hiding this comment.
I think my understanding of a shard is still vague. This seems like a good spot to fully define what it actually is. Could you put a definition here?
| optional uint32 publisher_id = 1; | ||
| // [required] | ||
| // Note: when adding a new variant here, update `Permissions` as well. | ||
| oneof action { |
There was a problem hiding this comment.
Should we also have a DeactivatePublisher too? We can theoretically do this by removing one/all public keys. But we would need to undo this afterward if it was temporary. Also, what about removing the publisher entirely?
There was a problem hiding this comment.
set_publisher_active can be used to deactivate it. remove_publisher action removes the publisher entirely.
|
Seems the build is failing though. Could you also verify that publish.sh works as you would expect? You can make sure your working directory has no changes and run it. And then inspect the files to see it all looks good. |
Riateche
force-pushed
the
lazer-governance-instructions
branch
from
e3c7168 to
5788fdc
Compare
Riateche
force-pushed
the
lazer-governance-instructions
branch
from
38fe032 to
db10a10
Compare
Riateche
force-pushed
the
lazer-governance-instructions
branch
from
db10a10 to
8bd51b5
Compare
Summary
Add proto declarations describing payload of a governance transaction.
Rationale
We want to manage Lazer through Pyth governance.
How has this been tested?