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

Add CompoundGovernorVotesUpgradeable #27

Merged
merged 7 commits into from
Oct 14, 2024
Merged

Add CompoundGovernorVotesUpgradeable #27

merged 7 commits into from
Oct 14, 2024

Conversation

garyghayrat
Copy link
Member

@garyghayrat garyghayrat commented Oct 9, 2024 •
edited
Loading

closes #13

@coveralls
Copy link

coveralls commented Oct 9, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 11331411098

Details

  • 0 of 10 (0.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-2.1%) to 89.947%
Changes Missing Coverage Covered Lines Changed/Added Lines %
contracts/CompoundGovernor.sol 0 1 0.0%
contracts/extensions/GovernorVotesCompUpgradeable.sol 0 9 0.0%
Totals Coverage Status
Change from base Build 11295366207: -2.1%
Covered Lines: 318
Relevant Lines: 355
💛 - Coveralls

@garyghayrat garyghayrat marked this pull request as ready for review October 9, 2024 20:06
jferas
jferas requested changes Oct 9, 2024
View reviewed changes
Copy link
Collaborator

@jferas jferas left a comment

Choose a reason for hiding this comment

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

Doesn't the CompoundGovernor.sol contract need to import from and inherit from this new contract that knows how to call the COMP token's getPriorVotes function?

contracts/extensions/CompoundGovernorVotesUpgradeable.sol Outdated Show resolved Hide resolved
contracts/test/CompoundGovernorVotesUpgradeable.t.sol Outdated Show resolved Hide resolved
contracts/test/CompoundGovernorVotesUpgradeable.t.sol Outdated Show resolved Hide resolved
contracts/test/CompoundGovernorVotesUpgradeable.t.sol Outdated Show resolved Hide resolved
contracts/test/CompoundGovernorVotesUpgradeable.t.sol Outdated Show resolved Hide resolved
contracts/test/CompoundGovernorVotesUpgradeable.t.sol Outdated Show resolved Hide resolved
@garyghayrat
Copy link
Member Author

Doesn't the CompoundGovernor.sol contract need to import from and inherit from this new contract that knows how to call the COMP token's getPriorVotes function?

Yes, it does. But I thought for this PR we can test the modified extension contract in isolation, so didn't want to do too much inheritance. We should definitely test CompoundGovernor with similar function calls once it inherits this modified extension contract.

@jferas jferas self-requested a review October 11, 2024 15:39
jferas
jferas approved these changes Oct 11, 2024
View reviewed changes
Copy link
Collaborator

@jferas jferas left a comment

Choose a reason for hiding this comment

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

Looks good!

wildmolasses
wildmolasses approved these changes Oct 11, 2024
View reviewed changes
Copy link
Collaborator

@wildmolasses wildmolasses left a comment

Choose a reason for hiding this comment

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

lgtm

contracts/extensions/CompoundGovernorVotesUpgradeable.sol Outdated Show resolved Hide resolved
contracts/test/CompoundGovernorVotesUpgradeable.t.sol Outdated Show resolved Hide resolved
contracts/extensions/GovernorVotesCompUpgradeable.sol Outdated
/// @notice Retrieves the voting weight for a specific account at a given timepoint.
/// @dev This function overrides the base _getVotes function to use Compound's getPriorVotes mechanism.
/// @param _account The address of the account to check the voting weight for.
/// @param _timepoint The block number at which to check the voting weight.
Copy link
Collaborator

Choose a reason for hiding this comment

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

consistency nit

Suggested change
/// @param _timepoint The block number at which to check the voting weight.
/// @param _timepoint The timepoint at which to check the voting weight.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should keep it block number because the contract is COMP specific and it requires a block number. Would it be better if the variable name was also blockNumber instead of timepoint?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the natspec in the rest of the contract, it says timepoint, not blockNumber. So for consistency, i'm advocating for timepoint. what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 8d5ee4c

contracts/test/GovernorVotesCompUpgradeable.t.sol Outdated
assertEq(address(governorVotes.token()), address(COMP_TOKEN_ADDRESS));
}

function test_Clock() public view {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can these test names better describe the functionality they're testing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 7d6d4bc

@garyghayrat garyghayrat force-pushed the feat/governor-votes-upgradeable-modified-for-comp branch from 762eaa3 to 7d6d4bc Compare October 11, 2024 17:35
@wildmolasses wildmolasses merged commit c450e5d into main Oct 14, 2024
3 checks passed
wildmolasses pushed a commit that referenced this pull request Nov 1, 2024
@garyghayrat @wildmolasses
Add GovernorVotesCompUpgradeable (#27)
002c872 
* Add `CompoundGovernorVotesUpgradeable` and `IComp`

* Add tests and misc

* Make tests `view`

* Rename and inherit contract

* Update storage location

* Make test names descriptive

* Update natspec
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wildmolasses wildmolasses wildmolasses approved these changes

@jferas jferas jferas approved these changes

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

Successfully merging this pull request may close these issues.

Feat: Modify GovernorVotesUpgradeable to support COMP token
4 participants
@garyghayrat @coveralls @jferas @wildmolasses