-
Notifications
You must be signed in to change notification settings - Fork 857
Conversation
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.
Nice! I started to review this PR, but later I saw it's still draft.
It requires some changes in the circuit as well, like for example inwitness_row.rs
adding
#[derive(Clone, Serialize, Deserialize, Debug)]
#[serde(transparent)]
struct HexVal {
#[serde(with = "hex::serde")]
hex: Vec<u8>,
}
and changing for Node
struct
pub values: Vec<Vec<u8>>,
to
pub values: Vec<HexVal>,
and then changing the assign function of MainRlpGadget
. Do you plan to do these changes in this PR?
60bb458
to
e115fd1
Compare
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.
Hi @miha-stopar,
Thanks for the early review! The PR is still a draft, but a quick look from you is helpful!
I think the MainRLPGadget can still take &[u8]
as inputs. The Deref trait on Hex resolved that issue.
d76d45a
to
938dbf9
Compare
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.
I didn't check json files but others looks good to me.
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.
LGTM!
de39a68
to
4e9502e
Compare
Description
Issue Link
#1658
Type of change
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Contents
How Has This Been Tested?
See if the new JSON format can be consumed by the MPT circuit and run the test successfully.