Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Conversation

rrzhang139
Copy link
Contributor

parse_calldata and parse_code seem to have a few discrepancies between yaml and json version (e.g yaml does not mention asm as tags)

@rrzhang139 rrzhang139 changed the title deuduplicated a few functions: address, to_address, bytes, hash, u256, u64 deuduplicated a few functions Dec 15, 2022
@ChihChengLiang ChihChengLiang linked an issue Dec 16, 2022 that may be closed by this pull request
@aguzmant103
Copy link
Member

Hey @rrzhang139 just checking, it this ready for review? Feel free to reach out directly or autoassign by assigning it to privacy-scaling-explorations/zkevm-reviewers

@rrzhang139
Copy link
Contributor Author

@aguzmant103 I don't have the ability to assign reviewers, but this seems ready to review! Perhaps @ed255 could review this for me?

@ChihChengLiang ChihChengLiang self-requested a review December 26, 2022 18:32
@ChihChengLiang
Copy link
Collaborator

Hi @rrzhang139, I can help review this PR. Can you resolve the conflict meanwhile?

@rrzhang139
Copy link
Contributor Author

@ChihChengLiang I resolved this, but I am failing tests when running make test. I also cloned a fresh instance of the repo from main and it also failed the exact same tests, which is very strange. I pushed it, for now, to check if it passes CI, but I'm also debugging locally at the moment. I will keep you updated when I find the problem.

@ChihChengLiang
Copy link
Collaborator

@rrzhang139 What are the failing tests you see in make test? I did see some error logs from make test, but those should come from the negative test cases. The error logs look annoying but should be fine.

The CI reported some failing Clippy checks. You might need to run make clippy and fix some bugs.

Copy link
Collaborator

@ChihChengLiang ChihChengLiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! The code is less duplicated and looks nicer now!
We can merge once the CI issues are all fixed.
I added some low-hanging fruit refactor opportunities. They might be out of the scope of this PR so feel free to ignore them.

Comment on lines 16 to 24
if let Some(hex) = as_str.strip_prefix("0x") {
Ok(Address::from_slice(
&hex::decode(hex).context("parse_address")?,
))
} else {
Ok(Address::from_slice(
&hex::decode(as_str).context("parse_address")?,
))
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I see many functions that use strip_prefix have duplicated lines. We could deduplicate the expression further. It's fine if we don't do it now.

Suggested change
if let Some(hex) = as_str.strip_prefix("0x") {
Ok(Address::from_slice(
&hex::decode(hex).context("parse_address")?,
))
} else {
Ok(Address::from_slice(
&hex::decode(as_str).context("parse_address")?,
))
}
let hex = as_str.strip_prefix("0x").unwrap_or(as_str);
Ok(Address::from_slice(
&hex::decode(hex).context("parse_address")?,
))

Comment on lines 16 to 24
if let Some(hex) = as_str.strip_prefix("0x") {
Ok(Address::from_slice(
&hex::decode(hex).context("parse_address")?,
))
} else {
Ok(Address::from_slice(
&hex::decode(as_str).context("parse_address")?,
))
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I see many functions that use strip_prefix have duplicated lines. We could deduplicate the expression further. It's fine if we don't do it now.

Suggested change
if let Some(hex) = as_str.strip_prefix("0x") {
Ok(Address::from_slice(
&hex::decode(hex).context("parse_address")?,
))
} else {
Ok(Address::from_slice(
&hex::decode(as_str).context("parse_address")?,
))
}
let hex = as_str.strip_prefix("0x").unwrap_or(as_str);
Ok(Address::from_slice(
&hex::decode(hex).context("parse_address")?,
))

@rrzhang139 rrzhang139 force-pushed the reuse-type-deserializers-json-yaml-testool branch from ae841a0 to 47adadd Compare December 28, 2022 13:37
@ChihChengLiang
Copy link
Collaborator

@rrzhang139 one more clippy fix to go.

@rrzhang139 rrzhang139 force-pushed the reuse-type-deserializers-json-yaml-testool branch from 47adadd to 5acb742 Compare December 29, 2022 13:56
@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member labels Dec 29, 2022
@github-actions github-actions bot removed crate-zkevm-circuits Issues related to the zkevm-circuits workspace member crate-bus-mapping Issues related to the bus-mapping workspace member labels Dec 29, 2022
@ChihChengLiang ChihChengLiang merged commit 325a58f into privacy-scaling-explorations:main Dec 29, 2022
@ChihChengLiang
Copy link
Collaborator

Thank you @rrzhang139 for the contribution!

noel2004 pushed a commit to noel2004/zkevm-circuits that referenced this pull request Nov 29, 2023
* fix soundness bug in ecdsa circuit

* update parameters

* fix: ec_sub_unequal would have panicked for some edge cases (privacy-scaling-explorations#999)

* update ecdsa parameters

---------

Co-authored-by: Rohit Narurkar <rohit.narurkar@protonmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reuse type deserializers for json and yaml parsing in testool
5 participants