Logic for checking Substrate proofs from within runtime module. #3783
Conversation
| @@ -0,0 +1,25 @@ | |||
| // Copyright 2017-2019 Parity Technologies (UK) Ltd. | |||
There was a problem hiding this comment.
| // Copyright 2017-2019 Parity Technologies (UK) Ltd. | |
| // Copyright 2019 Parity Technologies (UK) Ltd. |
| @@ -0,0 +1,103 @@ | |||
| // Copyright 2017-2019 Parity Technologies (UK) Ltd. | |||
There was a problem hiding this comment.
| // Copyright 2017-2019 Parity Technologies (UK) Ltd. | |
| // Copyright 2019 Parity Technologies (UK) Ltd. |
| @@ -242,7 +246,7 @@ mod tests { | |||
| MockBridge::tracked_bridges(1), | |||
| Some(BridgeInfo { | |||
| last_finalized_block_number: 42, | |||
| last_finalized_block_hash: test_header.hash(), // FIXME: This is broken | |||
There was a problem hiding this comment.
I don't know the details but is this now unbroken?
There was a problem hiding this comment.
Yes, this is now fixed in this PR. Was a compilation issue.
There was a problem hiding this comment.
testing::Header doesn't have a hash() method available, which is why it was broken. But yeah, it's fixed now
There was a problem hiding this comment.
My comment was due to the fact that only looking at the local scope of the code, I only identified that test_header.hash(); now has a variable and no logic has changed and I was like hmm.. how does that solve the problem? 😬
| /// Constructs a new storage proof checker. | ||
| /// | ||
| /// This returns an error if the given proof is invalid with respect to the given root. | ||
| pub fn new(root: H::Out, proof: Vec<Vec<u8>>) -> Result<Self, Error> { |
There was a problem hiding this comment.
This API does look a bit strange to me that you build a new instance of checked and via the constructor you also give it the task that it has to do (i.e. proof to check). If it woks for you then it's okay but I was expecting new to actually return a new instance of something that can check and a .check to actually check.
There was a problem hiding this comment.
Though looking at the test I see it more clear of how you intend to use it. Just a thought. It seems like this struct is StateProofCheckerAndReader?
There was a problem hiding this comment.
Yeah... I'm also not thrilled about this API, but it's kind of a direct consequence of the proof format. See #3782.
StateProofCheckerAndReader is somewhat more accurate of a description, but in substrate-state-machine, it's just called check_read_proof usually so ¯_(ツ)_/¯.
See PR title.