-
Notifications
You must be signed in to change notification settings - Fork 70
feat: finalize *tokenless* HubPool CCTP messages #2270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
DepositForBurn
messagesSigned-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
…ng status Signed-off-by: Ihor Farion <[email protected]>
…ying on cctpMessageIndex Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
…or pending attestations ... Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
* @returns A Set of unique transaction hashes. | ||
*/ | ||
async function getRelevantCCTPTxHashes( | ||
srcProvider: RetryProvider, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need RetryProvider
here? I'd have expected Provider
should be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what the differences are. Just used it because it was used in other places in the file
const isCctpV2 = isCctpV2L2ChainId(l2ChainId); | ||
senderAddresses: string[], | ||
sourceEventSearchConfig: EventSearchConfig, | ||
includeTokenlessHubPoolMessages: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt about setting a default on this? Are there scenarios where we might want to only look at tokens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming might be better here probably. This flag:
- enables scanning for any CCTP events in the HubPool-related txs.
- in practice, this means querying MessageRelayed, TokenRelayed events on HubPool address
- if HubPool address is not defined for a sourceChain, this funnction will crash. So I use
includeTokenlessHubPoolMessages = false
for L2 -> L1 CCTP for example
const messageRelayedEvents = await paginatedEventQuery(hubPool, messageRelayedFilter, sourceEventSearchConfig); | ||
messageRelayedEvents.forEach((e) => txHashesFromHubPool.push(e.transactionHash)); | ||
|
||
const tokensRelayedFilter = hubPool.filters.TokensRelayed(); | ||
const tokensRelayedEvents = await paginatedEventQuery(hubPool, tokensRelayedFilter, sourceEventSearchConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably makes sense to do these in parallel via Promise.all().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Interestingly, I tried to just create a correct combined topic filter but failed. Can tell if I did it wrong or paginatedEventQuery
doesn't support that
let lastMessageSentEventIdx = -1; | ||
receipt.logs.forEach((log, i) => { | ||
if (_isMessageSentEvent(log)) { | ||
if (lastMessageSentEventIdx == -1) { | ||
lastMessageSentEventIdx = i; | ||
} else { | ||
_addCommonMessageEventIfRelevant(receipt.logs[lastMessageSentEventIdx]); | ||
lastMessageSentEventIdx = i; | ||
} | ||
} else { | ||
const depositForBurnVersion = _getDepositForBurnVersion(log); | ||
if (depositForBurnVersion == -1) { | ||
// Skip non-`DepositForBurn` events | ||
return; | ||
} | ||
if (lastMessageSentEventIdx == -1) { | ||
throw new Error( | ||
"DepositForBurn event found without corresponding MessageSent event. " + | ||
"Each DepositForBurn event must have a preceding MessageSent event in the same transaction. " + | ||
`Transaction: ${receipt.transactionHash}, DepositForBurn log index: ${i}` | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you comment on what this loop is doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Here, we go one by one over a receipts that we "suspect might have CCTP messages for finalization".
It's important to understand what we're looking for in the logs. It's either of 2 things:
MessageSent
+DepositForBurn
== a single USDC transfer from one chain to another (1)- solo
MessageSent
with noDepositForBurn
-> a single crosschain message (2)
Then for each receipt we traverse all logs in order, and keep the latest encountered MessageSent
index in lastMessageSentEventIdx
. Then if we encounter next MessageSent
-> the one stored in lastMessageSentEventIdx
was (2). If we encounter DepositForBurn
, then we bundle stored lastMessageSentEventIdx
with it to receive (1)
} | ||
|
||
function _getPendingV2AttestationStatus(attestation: string): CCTPMessageStatus { | ||
return attestation === "pending_confirmation" ? "pending" : "ready"; | ||
function _getPendingV2AttestationStatus(attestation: CCTPV2APIAttestation): CCTPMessageStatus { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks identical to _getPendingAttestationStatus()
- should we make attestation a union of CCTPV2APIAttestation | CCTPAPIGetAttestatuibResponse
and drop the 2nd implementation? Could alternatively do something like Pick<CCTPV2APIAttestation, "attestation" | "status">
.
Current implementation of CCTP finalizer is only tracking
DepositForBurn
events, and thus is not considering other types of CCTP messages. CCTP can send tokenless messages viaMessageTransmitter
contract, which emitsMessageSent(bytes message)
event.We use
HubPool
eventsTokensRelayed
andMessageRelayed
to find relevant tx hashes to search for suchMessageTransmitter
MessageSent
events in addition to allDepositForBurn
events.