Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PS-217: Implement the circuit for SC2SC verification #110

Merged
merged 20 commits into from
Jul 4, 2023

Conversation

la10736
Copy link
Contributor

@la10736 la10736 commented Jan 30, 2023

This circuit follow the specifications in ZenIP-42205 and tests use cctp_primitives::commitment_tree::CommitmentTree to build a valid states tree.

BTW I did a little refactor to reuse some utilities in tests from other circuits. I've introduced some structs to simplify the tests for this circuit but I avoided to refactor the other circuits tests and use the newer utilities ... That can be done later in another PR if we prefer.

Copy link
Collaborator

@DanieleDiBenedetto DanieleDiBenedetto left a comment

Choose a reason for hiding this comment

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

LGTM ! The requested changes are mainly very minor: refactoring, stile, typos...

demo-circuit/src/common/data_structures.rs Show resolved Hide resolved
demo-circuit/src/sc2sc/mod.rs Outdated Show resolved Hide resolved
demo-circuit/src/sc2sc/mod.rs Outdated Show resolved Hide resolved
demo-circuit/src/sc2sc/mod.rs Outdated Show resolved Hide resolved
demo-circuit/src/sc2sc/mod.rs Outdated Show resolved Hide resolved
demo-circuit/src/test_utils.rs Outdated Show resolved Hide resolved
demo-circuit/src/test_utils.rs Outdated Show resolved Hide resolved
demo-circuit/src/sc2sc/tests.rs Show resolved Hide resolved
demo-circuit/src/sc2sc/tests.rs Outdated Show resolved Hide resolved
demo-circuit/src/sc2sc/tests.rs Outdated Show resolved Hide resolved
}

impl CommitmentHelper {
pub(crate) fn add_random_forward_transert_to_sc(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mmm don't see it changed

});
}

pub(crate) fn add_random_backward_transert_to_sc(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't see it changed

@la10736
Copy link
Contributor Author

la10736 commented Jan 31, 2023

Should be done now... THX 👀

@la10736 la10736 marked this pull request as draft February 6, 2023 08:43
@DanieleDiBenedetto DanieleDiBenedetto changed the base branch from development to audit_fixes_2023 March 7, 2023 11:10
@DanieleDiBenedetto DanieleDiBenedetto changed the base branch from audit_fixes_2023 to development March 7, 2023 11:11
@DanieleDiBenedetto DanieleDiBenedetto marked this pull request as ready for review March 21, 2023 09:14
la10736 and others added 17 commits July 4, 2023 12:01
to reuse some utilities in tests from other
circuits.
ScCommitmentPath and simplified the
tests that now fail all with the same
message
it by hand and removed the
 _man in the middle_ enforce_message_path() .
implemented check_membership()
also for ScCommitmentCertPathGadget: a more
idionatic way to define it.
RandomWithdrawalCertificateDataBuilder to
build WithdrawalCertificateData for a generic
Rng
 - transfert -> transfer
 - widthdrawal -> withdrawal
check the minimum number of custom
fields. Add also test and removed the
hardcoded 3: replaced it with MSG_MT_HEIGHT
everywhere
Implement Sc2Sc jni interface
Add ScCommitmentCertPath.updateScCommitmentPath() 
to handle the case where sdk can just rebuilt the sidechain 
tree but not the complete commitment tree.
- Fixed TYPOS
- Renamed variable to use the same notation
used in design documents
- Don't point to outdated doc
@DanieleDiBenedetto DanieleDiBenedetto merged commit 8104111 into development Jul 4, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants