lp-gateway: Add extrinsics for initiating and disputing message recovery #1970
Conversation
| @@ -226,7 +226,7 @@ pub enum Message<BatchContent = BatchMessages> { | |||
| /// The hash of the message which shall be recovered | |||
| hash: [u8; 32], | |||
| /// The address of the router | |||
| address: Address, | |||
| router: [u8; 20], | |||
There was a problem hiding this comment.
Can anyone please confirm that changing these fields is OK?
There was a problem hiding this comment.
I don't think so. The deserialize part in Solidity would expect 32 bytes 🤔, unless this message has special treatment
cc @hieronx
There was a problem hiding this comment.
We must not change these fields because as @lemunozm mentioned, EVM expects 32 bytes: https://github.com/centrifuge/liquidity-pools/blob/6e8f1a29dff0d7cf5ff74285cfffadae8a8b303f/src/gateway/Gateway.sol#L259
There was a problem hiding this comment.
There are a few messages which use 32 byte array fields such as RecoverAssets even though for now, 20 bytes suffice. The reasoning behind this is to be well positioned longterm for potential 32 byte addresses. On Centrifuge Chain, we have the luxury of being able to apply migrations. On EVM, that is a much more difficult (and expensive) process AFAIK.
There was a problem hiding this comment.
Thanks for explaining the whys! I didn't know the exact reason.
There was a problem hiding this comment.
OK, I will revert the type, what about the field name then?
cdamian
force-pushed
the
feat/lp-v2-message-recovery
branch
2 times, most recently
from
d82316a to
2fc0470
Compare
cdamian
force-pushed
the
feat/lp-v2-message-recovery
branch
from
3849801 to
b3f2f07
Compare
| origin: OriginFor<T>, | ||
| domain: Domain, | ||
| message_hash: MessageHash, | ||
| recovery_router: [u8; 32], | ||
| messaging_router: T::RouterId, |
There was a problem hiding this comment.
Question. Should the domain field always be the domain belonging to the messaging_router passed?
If that is the case, we can extend RouterId: Into<Domain>, which is already implemented in common/routing.rs and save the user to write the domain parameter.
There was a problem hiding this comment.
If agree, we can merge this anyways and do it in a next PR
There was a problem hiding this comment.
Let's do it separately and not waste another CI run?
| origin: OriginFor<T>, | ||
| domain: Domain, | ||
| message_hash: MessageHash, | ||
| recovery_router: [u8; 32], | ||
| messaging_router: T::RouterId, |
There was a problem hiding this comment.
Same here, and probably in any place where we're passing both domain and router_id, because router_id already carries the domain information
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1970 +/- ##
==========================================
+ Coverage 48.86% 48.96% +0.10%
==========================================
Files 183 183
Lines 13095 13133 +38
==========================================
+ Hits 6399 6431 +32
- Misses 6696 6702 +6 ☔ View full report in Codecov by Sentry. |
| /// | ||
| /// Hash - hash of the message that should be recovered. | ||
| /// Router - the address of the recovery router. | ||
| fn initiate_recovery_message(hash: [u8; 32], router: [u8; 32]) -> Self; |
There was a problem hiding this comment.
Nit: Should use MessageHash type in signature
| fn initiate_recovery_message(hash: [u8; 32], router: [u8; 32]) -> Self; | |
| fn initiate_recovery_message(hash: MessageHash, router: [u8; 32]) -> Self; |
There was a problem hiding this comment.
I'll add it in - #1974
| /// | ||
| /// Hash - hash of the message that should be disputed. | ||
| /// Router - the address of the recovery router. | ||
| fn dispute_recovery_message(hash: [u8; 32], router: [u8; 32]) -> Self; |
There was a problem hiding this comment.
Nit: Should use MessageHash type in signature
| fn dispute_recovery_message(hash: [u8; 32], router: [u8; 32]) -> Self; | |
| fn dispute_recovery_message(hash: MessageHash, router: [u8; 32]) -> Self; |
Description
Added:
Changed:
LPEncodingtoLPMessage.Checklist: