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

fix(og): N-04 - Eliminate redundant code #4493

Merged
merged 3 commits into from Mar 22, 2023
Merged

Conversation

Reinis-FRP
Copy link
Contributor

Motivation

OZ identified the following issue:

Consider making the following changes to eliminate redundant code:
• In OptimisticGovernor.sol , msg.sender is used on line 263 to transfer tokens
from the proposer to the Optimistic Governor contract, but proposer has already been
defined as msg.sender on line 234, so proposer can be used here.
• In OptimisticGovernor.sol , line 310 computes the assertionId , but this value
was already computed on line 307 inside a require statement. Lines 310 and 307 can
be swapped, and proposalHashes[_proposalhash] inside the require
statement can be replaced with assertionId .

Summary

This PR addresses this issue by implementing suggested fixes above.

Testing

Check a box to describe how you tested these changes and list the steps for reviewers to test.

  • Ran end-to-end test, running the code as in production
  • New unit tests created
  • Existing tests adequate, no new tests required
  • All existing tests pass
  • Untested

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Copy link
Member

@mrice32 mrice32 left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
Signed-off-by: Reinis Martinsons <reinis@umaproject.org>
@Reinis-FRP Reinis-FRP merged commit 835a6ff into master Mar 22, 2023
1 check passed
@Reinis-FRP Reinis-FRP deleted the reinis-frp/n04-og-upgrade branch March 22, 2023 11:54
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