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.

Sign up for GitHub

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

Jump to bottom

feat: add quorum #49

Merged
merged 2 commits into from
Jan 17, 2024
Merged

feat: add quorum #49

merged 2 commits into from
Jan 17, 2024

Conversation

guidanoli
Copy link
Collaborator

Closes #48

@guidanoli guidanoli self-assigned this Aug 28, 2023
@guidanoli guidanoli linked an issue Aug 28, 2023 that may be closed by this pull request
Design a contract that implements a Quorum consensus model #48
Closed
@guidanoli guidanoli mentioned this pull request Aug 28, 2023
Closed
Base automatically changed from feature/adapt-workflows to main August 28, 2023 12:18
@ZzzzHui ZzzzHui mentioned this pull request Aug 30, 2023
Closed
@guidanoli guidanoli force-pushed the feature/quorum branch 2 times, most recently from 528b1e8 to 8735702 Compare September 6, 2023 17:23
@guidanoli guidanoli force-pushed the feature/quorum branch 2 times, most recently from 8cce1d9 to a3b1038 Compare September 21, 2023 18:57
@guidanoli
Copy link
Collaborator Author

Benchmark

For future optimizations, I've run forge test with some extra options.

forge test --match-contract Quorum --gas-report --fuzz-seed 1

Here is the gas report.

Quorum
Deployment Cost Deployment Size
1763700 9019
Function Name min avg median max # calls
getHistory 280 280 280 280 1
submitClaim 48629 56786 49200 72529 3

The most important data points are the min and max gas costs for the submitClaim function.

This was referenced Sep 22, 2023
Merged
Merged
@ZzzzHui
Copy link
Contributor

ZzzzHui commented Oct 12, 2023

This branch could use a rebase

@changeset-bot
Copy link

changeset-bot bot commented Oct 25, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: 102fa78

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ZzzzHui
Copy link
Contributor

ZzzzHui commented Nov 6, 2023

should the commit "feat: revert if erc20 deposit failed" be in this PR? Maybe you rebased this PR on "next" branch but pushed to base "main"?

@guidanoli
Copy link
Collaborator Author

should the commit "feat: revert if erc20 deposit failed" be in this PR? Maybe you rebased this PR on "next" branch but pushed to base "main"?

Oops, sorry! I think I rebased this on top of next. Let me fix that real quick... Thanks!

@guidanoli guidanoli force-pushed the feature/quorum branch 2 times, most recently from 73bf922 to a8454c6 Compare November 7, 2023 15:16
@guidanoli guidanoli marked this pull request as ready for review November 7, 2023 16:25
@guidanoli guidanoli force-pushed the feature/quorum branch 2 times, most recently from f6f1dfd to 102fa78 Compare November 8, 2023 11:25
@guidanoli guidanoli force-pushed the feature/quorum branch 2 times, most recently from 83c3b2b to c9f1b91 Compare December 18, 2023 21:44
@guidanoli guidanoli requested a review from ZzzzHui December 18, 2023 21:45
ZzzzHui
ZzzzHui reviewed Dec 19, 2023
View reviewed changes
onchain/rollups/contracts/consensus/quorum/Quorum.sol Outdated Show resolved Hide resolved
onchain/rollups/contracts/consensus/quorum/Quorum.sol Show resolved Hide resolved
onchain/rollups/contracts/consensus/quorum/Quorum.sol Outdated Show resolved Hide resolved
onchain/rollups/contracts/consensus/quorum/Quorum.sol Outdated Show resolved Hide resolved
onchain/rollups/contracts/consensus/quorum/Quorum.sol Outdated Show resolved Hide resolved
onchain/rollups/contracts/consensus/AbstractConsensus.sol Outdated Show resolved Hide resolved
onchain/rollups/contracts/consensus/quorum/Quorum.sol Show resolved Hide resolved
onchain/rollups/test/foundry/consensus/quorum/Quorum.t.sol Outdated Show resolved Hide resolved
onchain/rollups/test/foundry/consensus/quorum/Quorum.t.sol Outdated Show resolved Hide resolved
onchain/rollups/test/foundry/consensus/quorum/Quorum.t.sol Outdated Show resolved Hide resolved
ZzzzHui
ZzzzHui previously approved these changes Dec 19, 2023
View reviewed changes
@guidanoli guidanoli changed the base branch from main to release/2.1.x December 19, 2023 14:08
@guidanoli guidanoli dismissed ZzzzHui’s stale review December 19, 2023 14:08

The base branch was changed.

ZzzzHui
ZzzzHui previously approved these changes Dec 19, 2023
View reviewed changes
@guidanoli @ZzzzHui
feat: add Quorum contract
2acc815 
- Special thanks to @ZzzzHui for helping with gas optimizations!
@ZzzzHui
Copy link
Contributor

ZzzzHui commented Jan 12, 2024

rebased on top of main branch.
commit fix: remove empty set check and fix: {first,last}InputIndex -> {first,last}Index are already in the main branch so those 2 commits are dropped.

@ZzzzHui ZzzzHui changed the base branch from release/2.1.x to main January 12, 2024 08:32
@guidanoli guidanoli requested a review from ZzzzHui January 16, 2024 14:02
@guidanoli guidanoli merged commit fc4a435 into main Jan 17, 2024
2 checks passed
@guidanoli guidanoli deleted the feature/quorum branch January 17, 2024 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZzzzHui ZzzzHui ZzzzHui approved these changes

@pedroargento pedroargento pedroargento approved these changes

Assignees

@guidanoli guidanoli

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Design a contract that implements a Quorum consensus model
3 participants
@guidanoli @ZzzzHui @pedroargento