feat: add finalizer support for messages from HubPool to UniversalSpoke
#2203
Conversation
…roken. Init commit Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
…lticall2Call | AugmentedTransaction) to enable returning individual txns from finalizer adapters 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]>
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]>
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]>
grasphoper
requested review from
mrice32,
nicholaspai,
pxrl,
james-a-morris,
dohaki and
bmzig
as code owners
src/finalizer/utils/helios.ts
Outdated
| proofId, | ||
| errorMessage: proofState.error_message, | ||
| }); | ||
| try { |
There was a problem hiding this comment.
Why re-try here? Should we just log an error and then wait for the next finalizer run to re-request?
There was a problem hiding this comment.
Here, ZK API tried to generate a proof but failed. We re-request it here and log an error. In practice, we should never have proofs for which generation errors. The next finalizer run will not re-request it unless we explicitly do so here
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
…hBlockNumber ones Signed-off-by: Ihor Farion <[email protected]>
| // todo: (by not making them to wait for finality longer than needed) if our blockNumber that we need a proved slot for is older than this head. | ||
| const currentHead = headBn.toNumber(); | ||
| const currentHeader = await sp1HeliosContract.headers(headBn); | ||
| if (!currentHeader || currentHeader === ethers.constants.HashZero) { |
There was a problem hiding this comment.
i'm not sure calling a contract can ever return undefined-- it should either error or return you a type that you expect--in this case a BN
There was a problem hiding this comment.
Not sure how to check this. IMO this is a nice check that prevents us from sending a request with dst_chain_contract_from_header: undefined down below
There was a problem hiding this comment.
ok i think its unneccessary but your call
src/finalizer/utils/helios.ts
Outdated
| }); | ||
| } else { | ||
| // todo? Might want to log an error here | ||
| logger.warn({ |
There was a problem hiding this comment.
Would this ever happen? Don't we have a guarantee about the API's response shape, and if state == "success" then update_calldata must exist?
I'm wary any time I see a 4x nested if {} block that we can simplify somehow
There was a problem hiding this comment.
It can happen if API returns an incorrectly formatted response. The thinking is, "it must exist" yeah. But networking stuff is weird
There was a problem hiding this comment.
i think we should refactor and check the response shape of proofState when we first get it and throw if its ill formatted. And then work off the assumption that its correct on all following lines
I think we can reduce this extra if-else statement which makes the code less readable
src/finalizer/utils/helios.ts
Outdated
| * Retrieves an ethers.Contract instance for the SP1Helios contract on the specified chain. | ||
| * @throws {Error} If the SP1Helios contract address or ABI is not found for the given chainId in CONTRACT_ADDRESSES. | ||
| */ | ||
| function getSp1HeliosContract(chainId: number, signerOrProvider: Signer | Provider): ethers.Contract { |
There was a problem hiding this comment.
nit: should we move getSp1HeliosContract and getHubPoolStoreContract into UniversalUtils?
There was a problem hiding this comment.
Well. It's nuanced. These fns are opinionated in the sense that they throw if the contract is not there. This throwing opinion is really of helios.ts, not of utils package. But I agree that we should probably always throw on missing addr / abi so we can move?
There was a problem hiding this comment.
Also, I don't feel like these fns are super-useful to reuse. I don't mind either way
There was a problem hiding this comment.
i don't think the opinionated throwing is a bad thing to have in a utility function named as such. We should move it to a utility folder if only because this file is long and maintaining these utility functions willl be more difficult over time.
Signed-off-by: Ihor Farion <[email protected]>
Signed-off-by: Ihor Farion <[email protected]>
|
@nicholaspai I addressed most of your comments -- resolved the addressed ones. What else should I 100% address before we can merge? P.S. the failing test is just flakiness I think |
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]>
| chainId: l2ChainId, | ||
| method: "executeMessage", | ||
| args: executeArgs, | ||
| unpermissioned: true, |
There was a problem hiding this comment.
This should be false right? Only the EOA with the updater role can call this right?
This PR adds an "adapter" for a finalizer to be able to finalize messages from
HubPoolto "Universal chains" using ZK proofs:helios.ts. This finalization only works in L1 -> L2 direction.Like for other adapters, there's a main function:
heliosL1toL2Finalizer, which does roughly the following:Step 1: identify messages that require finalizing
HubPoolStorethat were recently stored (up to lookback)SP1Heliosthat signify recently verified (via ZK proofs) Source Chain storage slotsUniversal_SpokePoolthat signify execution of messages that were sent.Using this information, we can identify which messages have been executed (3.) -- skip those. For other messages, if we received an event on (2.) but not (3.), the message only needs execution, and not proof verification -- so we do only that.
For all the other messages we do the full flow: ZK proof generation +
Sp1Helios.update+Universal_SpokePool.executeMessageStep 2: request ZK proofs for the messages that have yet to be verified
request_proof()proofState.statusrequest_proof()Step 3: generate transactions that need to be executed.
For all messages that only require
executeMessage-> generate a transaction.For all messages that require both proof and
executeMessage, only if there's proof data available already, generate 2 transactions.