refactor: make ChainInclusion trait fully generic and change input to the raw AnchorProof #570
Conversation
stbrody
requested review from
dav1do,
a team,
nathanielc and
AaronGoldman
as code owners
stbrody
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
| /// Get the CAIP2 chain ID supported by this RPC provider | ||
| fn chain_id(&self) -> &caip2::ChainId; | ||
|
|
||
| /// Get the block chain transaction if it exists with the block timestamp information | ||
| async fn chain_inclusion_proof(&self, input: &Self::InclusionInput) | ||
| -> Result<TimeProof, Error>; | ||
| async fn chain_inclusion_proof(&self, input: &AnchorProof) -> Result<TimeProofResults, Error>; |
There was a problem hiding this comment.
this is the core of the change in this PR. Now instead of having the ChainInclusion type be generic over an InclusionInput type that must be specified for each implementation of the trait (and built by each caller of the chain_inclusion_proof function), now we just simply take the raw AnchorProof object and let the implementations decide how to interpret that object.
This avoids the callers of this function needing to know whether the AnchorProof is for an ethereum anchor or an Adm anchor and having to build up the appropriate input type accordingly. Now the caller can just blindly pass the anchor proof down into this function and trust the underlying implementation to do the right thing.
The goal is that other than the initial setup to construct the ChainInclusionProviders from the given RPC urls at startup, none of the event validation code will need to be in any way aware of which chain its providers are for, or have any chain-specific logic. All of that gets pushed down to the two implementations of the ChainInclusion trait.
| /// Get the CAIP2 chain ID supported by this RPC provider | ||
| fn chain_id(&self) -> &caip2::ChainId; | ||
|
|
||
| /// Get the block chain transaction if it exists with the block timestamp information | ||
| async fn chain_inclusion_proof(&self, input: &Self::InclusionInput) | ||
| -> Result<TimeProof, Error>; | ||
| async fn chain_inclusion_proof(&self, input: &AnchorProof) -> Result<TimeProofResults, Error>; |
There was a problem hiding this comment.
Why do we use the AnchorProof struct when it has more information than is needed by the implementations? With this API its not clear if the chain_inclusion_proof method is checking if the root cid returned matches the root cid of the input proof structure.
This API is about only getting the proof result correct? In that case I think I prefer having the associated type InclusionInput that makes it explicit what information a given provider needs in order to get the proof result.
Thoughts? Maybe I have missed the motivation behind this change.
There was a problem hiding this comment.
Maybe we do type InclusionInput: From<AnchorProof> so that callers can still use the anchor proof but then its clear that the provider itself is not validating the proof but instead just retrieving the information the proof references?
There was a problem hiding this comment.
Per discussion on discord, there's unfortunately no way to do this without leaking details of the different ChainInclusion implementations for different chains into the calling code. Decided the lesser evil is taking the AnchorProof, even though its taking in more information than is needed, as it allows the EventValidation code to avoid any special logic that depends on what chain is being validated.
| /// A proof of time on the blockchain | ||
| pub struct TimeProof { | ||
| /// A proof of time derived from state on the blockchain | ||
| pub struct TimeProofResults { |
There was a problem hiding this comment.
We have lots of names for what seem like the same thing, can we consolidate on a consistent naming?
For example we have AnchorProof, TimeProofResults and chain_inclusion_proof. What is the difference between anchor, time, and chain inclusion? Do we really have three different types of proofs? Why is the function named chain_inclusion_proof when it returns a TimeProofResults?
Should the names instead be, AnchorProof, ChainInclusionProof and get_chain_inclusion_proof? Where anchor proof is a chain agnostic chain inclusion proof? I'd be fine with other names too, its just not clear how all these thing relate given their current names and the differences imply there are more entities than there really are.
There was a problem hiding this comment.
yep agreed. Took your suggested names, I think those are clearer.
stbrody
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
| @@ -226,7 +226,7 @@ impl EventValidator { | |||
| Ok(validated_events) | |||
| } | |||
|
|
|||
| /// Transforms the [`ChainInclusionError`] into a [`ValidationError`] with an appropriate message | |||
| /// Transforms the [`eth_rpc::Error`] into a [`ValidationError`] with an appropriate message | |||
There was a problem hiding this comment.
How so? Function takes an eth_rpc::Error and returns a ValidationError
| .map_err(|e| eth_rpc::Error::InvalidProof(e.to_string()))?, | ||
| }; | ||
| let proof = provider.chain_inclusion_proof(&input).await?; | ||
| let proof = provider.chain_inclusion_proof(event.proof()).await?; |
There was a problem hiding this comment.
I know that this is the way it is now. but can we rename proof here to make clear that this is the proof from the chain and that
proof.root_cid != event.proof().root()Is comparing the events attestation to the chain's attestation.
There was a problem hiding this comment.
good call, renamed proof to chain_proof, added a comment, and added the other root to the error message
stbrody
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
… the raw AnchorProof
stbrody
force-pushed
the
generic-chain-inclusion
branch
from
fb38835
to
474bfd3
Compare
stbrody
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
builds on #550 making things more fully generic and preparing for the addition of an "adm" implementation of the
ChainProvidertrait.