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

feat: quartz-common and merging quartz-cli business logic from cycles-protocol #123

Merged
merged 18 commits into from
Jul 30, 2024

Conversation

dangush
Copy link
Contributor

@dangush dangush commented Jul 26, 2024

Merges the quartz cli rust scripts, changes to quartz packages, and changes to mtcs app (so basically the entirety of cycles-protocol into cycles-quartz.

This merge precedes the removal of all quartz packages from the cycles-protocol repo.

@dangush dangush changed the title Dangush/rust scripts feat: Quartz CLI business logic Jul 26, 2024
@dangush dangush requested review from hu55a1n1 and thanethomson July 26, 2024 19:35
@dangush dangush force-pushed the dangush/rust-scripts branch from 986f27f to 4c61d69 Compare July 26, 2024 20:02
@dangush dangush force-pushed the dangush/rust-scripts branch from 4c61d69 to afc41f3 Compare July 26, 2024 20:06
@dangush dangush marked this pull request as ready for review July 26, 2024 22:00
@dangush dangush requested a review from ebuchman July 27, 2024 02:16
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

I haven't been able to run any of this code yet, so I can't verify that it works, but for time's sake I'd suggest we merge this and make any necessary fixes in follow-up PRs.

There's still a lot of work here to get to the point that we have the cycles-quartz repo in a good state and we have our quartz CLI working as per #61.

I'd still like to hear a good argument against the idea of having a single crates directory in which every crate that we make available as a library is kept. If there isn't one, let's get that done in a follow-up PR ASAP.

We also need to get rid of apps/mtcs from this repo ASAP.

Cargo.toml Outdated Show resolved Hide resolved
core/quartz-common/src/quartz_macro.rs Outdated Show resolved Hide resolved
utils/tm-prover/Cargo.toml Show resolved Hide resolved
utils/cycles-sync/Cargo.toml Show resolved Hide resolved
Copy link
Member

@hu55a1n1 hu55a1n1 left a comment

Choose a reason for hiding this comment

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

Awesome work @dangush! Did a quick review and didn't test but added some minor comments. Also the comments from #87 are still applicable because AFAICT this PR includes those changes too. Agree with Thane we should merge this quick, so I'm fine with merging this without addressing my comments but would be nice to address them in future PRs. 🙏

Cargo.toml Show resolved Hide resolved
apps/mtcs/contracts/cw-tee-mtcs/src/msg.rs Show resolved Hide resolved
apps/mtcs/contracts/cw-tee-mtcs/Cargo.toml Show resolved Hide resolved
Comment on lines +47 to +60
impl std::cmp::Ord for LiquiditySource {
fn cmp(&self, other: &Self) -> Ordering {
self.address.cmp(&other.address)
}
}

impl PartialOrd for LiquiditySource {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.address.cmp(&other.address))
}
}

// PartialEq implemented in #[cw_serde]
impl Eq for LiquiditySource {}
Copy link
Member

Choose a reason for hiding this comment

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

Can we derive these instead?

@dangush dangush changed the title feat: Quartz CLI business logic feat: quartz-common and merging quartz-cli business logic from cycles-protocol Jul 30, 2024
@dangush dangush merged commit 63aa30d into main Jul 30, 2024
7 checks 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.

3 participants