feat: implement ICS23 - existence proof verification part #200
Conversation
Farhad-Shabani
changed the title
| } | ||
| } | ||
|
|
||
| fn decode_raw(ref self: Proof, ref context: DecodeContext) { |
There was a problem hiding this comment.
This is a temporary implementation until we appropriately handle #265.
| @@ -1,219 +0,0 @@ | |||
| use protobuf::types::message::{ | |||
There was a problem hiding this comment.
Moved into the packages/ics23 library, and the enums have been revised to include only the operations (variants) that are already supported on Starknet or those we have an implementation for.
| @@ -0,0 +1,91 @@ | |||
| use protobuf::errors::ProtobufErrors; | |||
There was a problem hiding this comment.
I've gathered the Varint conversions that were added earlier, along with the new ones, under the varint module. Similarly, the related tests have been placed under protobuf/src/tests/varint.cairo for better organization.
| @@ -68,3 +66,26 @@ pub impl ArrayAsProtoMessage<T, +ProtoMessage<T>, +Drop<T>, +Default<T>> of Prot | |||
| } | |||
| } | |||
|
|
|||
| pub impl BytesAsProtoMessage of ProtoMessage<Array<u8>> { | |||
There was a problem hiding this comment.
More context on this: #267
Farhad-Shabani
force-pushed
the
farhad/ics23-cairo-impl
branch
2 times, most recently
from
c9f591a to
9a34698
Compare
Farhad-Shabani
force-pushed
the
farhad/ics23-cairo-impl
branch
from
9a34698 to
60d08e7
Compare
| } | ||
| } | ||
|
|
||
| impl ExistenceProofAsProtoMessage of ProtoMessage<ExistenceProof> { |
There was a problem hiding this comment.
Actually, I think we don't need to encode the Merkle proofs as protobuf. They are not part of the chain commitment, and thus can be freely re-encoded as Cairo when transmitted by the relayer.
There was a problem hiding this comment.
Good idea. But this requires further exploration into relayer code -- which is out of scope of this PR. Maybe we open an issue?
There was a problem hiding this comment.
I'm here basically implementing ICS-23 as a library. That's the next step. To decide how to integrate ICS-23 into both the Cairo and relayer codebases. We have the flexibility to use either encoded Protobuf data on-chain or the Cairo-encoded version.
By the way, I preferred to include the Protobuf implementation here as well, specifically since we were having bunch of artifacts ready to use. A main reason is that it allows me to reuse the existing cosmos/ics23 test data, keeping them as reference test cases. Otherwise, we’d have to generate new test data, which could raise questions about correctness.
| #[derive(Default, Debug, Copy, Drop, PartialEq, Serde)] | ||
| pub enum HashOp { | ||
| #[default] | ||
| NoOp, |
There was a problem hiding this comment.
IIRC, the NoOp hash op is insecure, and should never be used in production. We are probably better off only support the Cosmos-specific SHA256 hash and nothing else to reduce the attack surface.
There was a problem hiding this comment.
Good point. but let's tackle the discussion in a different issue? Because it's present in main as part of the old protobuf impl done by me.
There was a problem hiding this comment.
In production, only SHA256 is supported—anything else causes verification to fail. That said, I’d still advocate for keeping the NoOp variant. One really useful function of this variant that I found is in testing. It lets you skip hashing and check whether the verifier's behavior stays correct. (Defaults, No ops, zeros all should be rejected by verifier functions)
Additionally, I think making NoOp as the default improves the system security, since if no proof spec or hash op is specified, it defaults to NoOp, causing proof verification to fail. Basically this forces users / relayers to be explicit about the hash ops in their proof specification.
Closes: #266
Closes: #267
First part of #208
Description
This PR introduces a new
ics23library under thecairo-libs, which includes the implementation of ICS-23 existence proof verification, along with minimal happy-path tests. We here also fix some issues in ourprotobuflibrary to make it work with this PR. The non-existence proof verification requires further changes to ourprotobufAPIs and will be handled in a separate follow-up PR. Also, more rigorous tests will be added once the implementation is fully completed.