feat: Support SequenceAware cursors for Deliveries #4889
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4889 +/- ##
=======================================
Coverage 74.60% 74.60%
=======================================
Files 103 103
Lines 1516 1516
Branches 195 195
=======================================
Hits 1131 1131
Misses 364 364
Partials 21 21
|
| recipient: bytes_to_address(message.recipient)?, | ||
| body: message.msg_body.unwrap_or(Vec::new()), | ||
| })) | ||
| let delivery = bytes_to_address(delivery.msg_id)?; |
There was a problem hiding this comment.
Is there a more direct way of converting bytes to an h256 when we always expect the byte count to be 32? Feels a little awkward / misleading to be using a function intended for addresses that would support other lengths
| #[instrument(skip(self))] | ||
| pub async fn retrieve_message_by_nonce( | ||
| pub async fn retrieve_delivered_message_by_sequence( |
There was a problem hiding this comment.
nit: wonder if it's worth having the name reflect that this returns the message ID (not the message itself)
| } else { | ||
| Ok(None) | ||
| } | ||
| } | ||
|
|
||
| /// Get the tx id associated with a dispatched message. | ||
| /// Get the delivered message associated with a sequence. |
There was a problem hiding this comment.
nit
| /// Get the delivered message associated with a sequence. | |
| /// Get the tx id of a delivered message associated with a sequence. |
| sequence: u32, | ||
| ) -> Result<Option<Delivery>> { | ||
| if let Some(delivery) = delivered_message::Entity::find() | ||
| .filter(delivered_message::Column::Domain.eq(destination_domain)) |
There was a problem hiding this comment.
is there any meaningful benefit to selecting just the column we care about? i assume it's pretty much negligible as there isn't much data in each row
|
|
||
| #[async_trait] | ||
| impl HyperlaneSequenceAwareIndexerStoreReader<Delivery> for HyperlaneDbStore { | ||
| /// Gets a delivered message by its nonce. |
There was a problem hiding this comment.
nit
| /// Gets a delivered message by its nonce. | |
| /// Gets a delivered message by its sequence. |
| } | ||
|
|
||
| /// Gets the block number at which the log occurred. | ||
| async fn retrieve_log_block_number_by_sequence(&self, sequence: u32) -> Result<Option<u64>> { |
There was a problem hiding this comment.
nit: any thoughts / intuition on the runtime cost of this requiring 3 round trips? wondering if it's worth a single query that joins
| @@ -685,18 +685,18 @@ impl SealevelMailboxIndexer { | |||
| &self.mailbox.rpc() | |||
| } | |||
|
|
|||
| async fn get_dispatched_message_with_nonce( | |||
| async fn get_dispatched_message_with_sequence( | |||
There was a problem hiding this comment.
I don't feel strongly but fwiw I'm comfortable with referring to sequences as nonces for dispatched messages as they're the same. Calling this out mostly because in some other places we keep the nonce language (e.g. retrieve_dispatched_message_by_nonce that's in this PR), and consistency is nice. But agree that get_delivered_message_with_sequence is a better name
| @@ -939,10 +942,23 @@ impl Indexer<H256> for SealevelMailboxIndexer { | |||
| #[async_trait] | |||
| impl SequenceAwareIndexer<H256> for SealevelMailboxIndexer { | |||
| async fn latest_sequence_count_and_tip(&self) -> ChainResult<(Option<u32>, u32)> { | |||
| // TODO: implement when sealevel scraper support is implemented | |||
| info!("Message delivery indexing not implemented"); | |||
| let inbox = self | |||
There was a problem hiding this comment.
might be a good idea to use / dedupe some of the existing logic we have for getting this here
hyperlane-monorepo/rust/main/chains/hyperlane-sealevel/src/mailbox.rs
Lines 454 to 457 in 36a4c26
get_inbox_account or something
Description
Support SequenceAware cursors for Deliveries
Related issues
Backward compatibility
Yes (need adding column
sequencetodelivered_messagetable before mergingTesting
E2E Ethereum and Sealevel tests