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

test: resolution tests #68

Draft
wants to merge 6 commits into
base: dev
Choose a base branch
from
Draft

test: resolution tests #68

wants to merge 6 commits into from

Conversation

xorsal
Copy link
Contributor

@xorsal xorsal commented Oct 15, 2024

🤖 Linear

Closes OPT-516

Copy link

linear bot commented Oct 15, 2024

Comment on lines +141 to +142
// review: should we allow an alternative to this gas consuming approach?
// we could add a configurable param to skip the forced token mass-transfer, and let users do it manually.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also found this recommendation: https://www.notion.so/defi-wonderland/Internal-Report-69aa58aab1024d5b81bf24631c86636d

I'd prefer to allow a way to skip the mass transfer, but we could leave it for later :)


// warp past the revealing window
vm.warp(block.timestamp + _revealingTimeWindow);
oracle.resolveDispute(mockRequest, mockResponse, mockDispute);
Copy link
Member

Choose a reason for hiding this comment

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

Add more actions in the middle. For example a user trying to claimVote in the middle and it should fail

Copy link
Member

Choose a reason for hiding this comment

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

Cast vote into something that doesn't exist or that is already resolved

Copy link
Member

@0xShaito 0xShaito left a comment

Choose a reason for hiding this comment

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

Almost there! A little bit more integration testing and we are golden 🙏

I found two bugs in ERC20Resolution:
- `claimVote` allows voters to withdraw their casted votes any number of
  any number of times. A voter could cast a single vote into a
resolution and call `claimVote` until all votes from a resolution are
withdrawn.
- when you try to resolve a not escalated dispute it reverts with
  `ERC20ResolutionModule_AlreadyResolved`. This one also happens in
PrivateERC20Resolution.
@xorsal
Copy link
Contributor Author

xorsal commented Oct 16, 2024

I found two bugs in ERC20Resolution:

  • claimVote allows voters to withdraw their casted votes any number of times. A voter could cast a single vote on a
    resolution and call claimVote repeatedly until all votes are withdrawn. Check here

  • when you try to resolve a not escalated dispute reverts with ERC20ResolutionModule_AlreadyResolved. This one also happens in PrivateERC20ResolutionModule. Check here

@ashitakah
Copy link
Contributor

Also needed to merge with dev, branch conflicts

@gas1cent gas1cent marked this pull request as draft November 5, 2024 17:07
@gas1cent
Copy link
Member

gas1cent commented Nov 5, 2024

We've discovered 2 bugs in the modules that are not going to be used in EBO. Converting this PR to a draft to return to it after finishing EBO.

Copy link

linear bot commented Nov 7, 2024

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.

4 participants