feat(lazer) Add response messages for lazer transactions #2743
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -88,3 +88,26 @@ message LazerTransaction { | |||
| GovernanceInstruction governance_instruction = 2; | |||
| } | |||
| } | |||
|
|
|||
| // Response to SignedLazerTransactionResponse from the relayer | |||
| message SignedLazerTransactionResponse { | |||
There was a problem hiding this comment.
💬 We will need to expand this a lot but I haven't figured out the best way to do this yet. The difficult part is handling all of the sub-type of what SignedLazerTransaction can contain (atm it's just an arbitrary binary payload which in effect is always LazerTransaction).
Somehow we should represent PublisherUpdateResponse which could be accept (full or partial) and reject, both possibly having some more contextual details (like what we have for the envelope contexts used internally)
We'll need the same sort of thing for GovernanceInstructionResponse.
We could define the top-level accept message using something like this (and the same for the rejections):
message SignedLazerTransactionAccepted {
oneof details {
PublisherUpdateResponseAccepted publisher_update_accepted = 1;
GovernanceInstructionResponseAccepted governance_instruction_accepted = 2;
}
}and include further details in those specific messages. We could also include a bytes details field in the message that contains type-specific response (in the same way we do with the payload) but I'm ideologically opposed to that idea.
I think SignedLazerTransactionResponse and LazerTransactionResponse are related but ultimately separate concepts and we need to deal with that.
@pyth-network/price-feeds-team i'd like to hear your ideas, comments, concerns. Let's make sure we're on the same page now because changing this later will be a big pain.
There was a problem hiding this comment.
In the API endpoints we just want to return JSON, so I'm not sure if there is any reason to define this type in protobuf. It can be just another type definition in our API. As for the structure, we could use a enum with payload for governance/publisher variants, but we can also just dump all possible response details as optional fields on the top object because the sender already knows what type of transaction it sends.
There was a problem hiding this comment.
Why is this payload so generic anyway? Couldn't we constrain it at all?
There was a problem hiding this comment.
It has to be bytes because we need to decode it as bytes to verify the signature.
| // an unstable ID (sensitive to any proto changes) are should only be used to correlate | ||
| // immediate responses sent from the relayer | ||
| pub fn id(&self) -> anyhow::Result<[u8; 32]> { | ||
| let mut hasher = Sha256::new(); |
There was a problem hiding this comment.
❔ Is this actually a good idea? I like the simplicity but this creates some problems (e.g. two separate transactions sent at different times could have the same id which is only OK if all the transactions are idempotent). I think we should consider attaching a UUID on the client side - it's a tiny amount of data and IMHO a more resilient way to identify messages.
There was a problem hiding this comment.
For governance we have sequence numbers, and for publishers we have publisher timestamps that have to be increasing. Both can be interpreted as a sort of nonce. Therefore, payloads will always be different unless it's literally the same transaction (in which case the second occurrence will be ignored).
There was a problem hiding this comment.
It has to be bytes because we need to decode it as bytes to verify the signature.
| // immediate responses sent from the relayer | ||
| pub fn id(&self) -> anyhow::Result<[u8; 32]> { | ||
| let mut hasher = Sha256::new(); | ||
| self.write_to_writer(&mut hasher)?; |
There was a problem hiding this comment.
This will hash both the signature and payload. We need it to be only payload.
| self.write_to_writer(&mut hasher)?; | ||
| hasher | ||
| .finalize() | ||
| .try_into() |
There was a problem hiding this comment.
Hashing is infallible. handler.finalize().into() into the array type will work.
| .context("failed to calculate the sha256 as transaction id") | ||
| } | ||
|
|
||
| pub fn calculate_id_from_bytes(payload: &Vec<u8>) -> anyhow::Result<[u8; 32]> { |
There was a problem hiding this comment.
| pub fn calculate_id_from_bytes(payload: &Vec<u8>) -> anyhow::Result<[u8; 32]> { | |
| pub fn id_from_payload(payload: &[u8]) -> [u8; 32] { |
| // an unstable ID (sensitive to any proto changes) are should only be used to correlate | ||
| // immediate responses sent from the relayer | ||
| pub fn id(&self) -> anyhow::Result<[u8; 32]> { | ||
| let mut hasher = Sha256::new(); |
There was a problem hiding this comment.
For governance we have sequence numbers, and for publishers we have publisher timestamps that have to be increasing. Both can be interpreted as a sort of nonce. Therefore, payloads will always be different unless it's literally the same transaction (in which case the second occurrence will be ignored).
| @@ -88,3 +88,26 @@ message LazerTransaction { | |||
| GovernanceInstruction governance_instruction = 2; | |||
| } | |||
| } | |||
|
|
|||
| // Response to SignedLazerTransactionResponse from the relayer | |||
| message SignedLazerTransactionResponse { | |||
There was a problem hiding this comment.
In the API endpoints we just want to return JSON, so I'm not sure if there is any reason to define this type in protobuf. It can be just another type definition in our API. As for the structure, we could use a enum with payload for governance/publisher variants, but we can also just dump all possible response details as optional fields on the top object because the sender already knows what type of transaction it sends.
Summary
Add new message types to represent responses from the relayer to lazer transactions. Initially only include a very simple structure that we will expand upon later. Also add a synthetic transaction_id derived as sha256 hash of the message payload. Note that this is not a stable ID across proto versions and should only be used by the clients to identify responses to transactions over the same connection they were sent to.
Rationale
The relayers receive two types of transactions: governance instructions, and publisher updates. Both deserve having an acknowledgment or otherwise a response explain what the error is to allow clients to fix it.
How has this been tested?