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

Feature: Update onlyOwnersGuard to use O(1) lookup, fix an incorrect test #699

Merged
merged 1 commit into from
Nov 12, 2023
Merged

Feature: Update onlyOwnersGuard to use O(1) lookup, fix an incorrect test #699

merged 1 commit into from
Nov 12, 2023

Conversation

mmv08
Copy link
Member

@mmv08 mmv08 commented Nov 11, 2023

This PR:

  • Update the example onlyOwnersGuard to use O(1) owner lookup instead of O(n)
  • The test for the guard's "happy" path was incorrect because it signed a transaction with user2, which wasn't an owner and the safe instance was still connected to the owner's signer. The transaction failed with GS026, Safe's built-in error for an incorrect owner. The guard actually didn't stop the transaction

@mmv08 mmv08 requested review from a team, rmeissner, nlordell and akshay-ap and removed request for a team November 11, 2023 19:43
@github-actions GitHub Actions
Copy link

Pull Request Test Coverage Report for Build 6836209786

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 94.567%
Totals Coverage Status
Change from base Build 6720462527: 0.3%
Covered Lines: 397
Relevant Lines: 405
💛 - Coveralls

@mmv08 mmv08 merged commit 8930059 into main Nov 12, 2023
18 checks passed
@mmv08 mmv08 deleted the feature/optimize-onlyowners-guard branch November 12, 2023 18:48
@github-actions github-actions bot locked and limited conversation to collaborators Nov 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nlordell nlordell nlordell approved these changes

@rmeissner rmeissner Awaiting requested review from rmeissner rmeissner was automatically assigned from safe-global/safe-protocol

@akshay-ap akshay-ap Awaiting requested review from akshay-ap akshay-ap was automatically assigned from safe-global/safe-protocol

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mmv08 @nlordell