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

Fix compilation errors, upgrade Solidity #210

Closed
wants to merge 2 commits into from
Closed

Fix compilation errors, upgrade Solidity #210

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57c1114
upgrade GovernorBravoDelegateG2 to 0.8.10
jbeanz Jun 23, 2022
132dd66
Upgrade Solidity and get compile-able
arjun-io Jul 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions contracts/Governance/GovernorBravoDelegateG1.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// SPDX-License-Identifier: BSD-3-Clause
pragma solidity ^0.8.10;
pragma experimental ABIEncoderV2;

Expand Down
60 changes: 36 additions & 24 deletions contracts/Governance/GovernorBravoDelegateG2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
pragma solidity ^0.5.16;
// SPDX-License-Identifier: BSD-3-Clause
pragma solidity ^0.8.10;
pragma experimental ABIEncoderV2;

import "./GovernorBravoInterfaces.sol";
Expand Down Expand Up @@ -39,7 +40,7 @@ contract GovernorBravoDelegate is GovernorBravoDelegateStorageV2, GovernorBravoE
bytes32 public constant BALLOT_TYPEHASH = keccak256("Ballot(uint256 proposalId,uint8 support)");

/**
* @notice Used to initialize the contract during delegator contructor
* @notice Used to initialize the contract during delegator constructor
* @param timelock_ The address of the Timelock
* @param comp_ The address of the COMP token
* @param votingPeriod_ The initial voting period
Expand Down Expand Up @@ -91,24 +92,24 @@ contract GovernorBravoDelegate is GovernorBravoDelegateStorageV2, GovernorBravoE
uint endBlock = add256(startBlock, votingPeriod);

proposalCount++;
Proposal memory newProposal = Proposal({
id: proposalCount,
proposer: msg.sender,
eta: 0,
targets: targets,
values: values,
signatures: signatures,
calldatas: calldatas,
startBlock: startBlock,
endBlock: endBlock,
forVotes: 0,
againstVotes: 0,
abstainVotes: 0,
canceled: false,
executed: false
});

proposals[newProposal.id] = newProposal;
Proposal storage newProposal = proposals[proposalCount];
Copy link

Choose a reason for hiding this comment

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

This is incorrect since it doesn't create a new proprosal, but only creates link to a previous one.
You can test this

Copy link
Contributor Author

@arjun-io arjun-io Aug 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Your test is using memory structs while this is using storage. This is effectively creating a pointer to the struct at the array index in storage and modifying the fields there, not copying anything.

Copy link

Choose a reason for hiding this comment

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

Okay, nvm

Copy link

Choose a reason for hiding this comment

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

.

// This should never happen but add a check in case.
require(newProposal.id == 0, "GovernorBravo::propose: ProposalID collsion");
newProposal.id = proposalCount;
newProposal.proposer = msg.sender;
newProposal.eta = 0;
newProposal.targets = targets;
newProposal.values = values;
newProposal.signatures = signatures;
newProposal.calldatas = calldatas;
newProposal.startBlock = startBlock;
newProposal.endBlock = endBlock;
newProposal.forVotes = 0;
newProposal.againstVotes = 0;
newProposal.abstainVotes = 0;
newProposal.canceled = false;
newProposal.executed = false;

latestProposalIds[newProposal.proposer] = newProposal.id;

emit ProposalCreated(newProposal.id, msg.sender, targets, values, signatures, calldatas, startBlock, endBlock, description);
Expand Down Expand Up @@ -144,7 +145,7 @@ contract GovernorBravoDelegate is GovernorBravoDelegateStorageV2, GovernorBravoE
Proposal storage proposal = proposals[proposalId];
proposal.executed = true;
for (uint i = 0; i < proposal.targets.length; i++) {
timelock.executeTransaction.value(proposal.values[i])(proposal.targets[i], proposal.values[i], proposal.signatures[i], proposal.calldatas[i], proposal.eta);
timelock.executeTransaction{value:proposal.values[i]}(proposal.targets[i], proposal.values[i], proposal.signatures[i], proposal.calldatas[i], proposal.eta);
}
emit ProposalExecuted(proposalId);
}
Expand Down Expand Up @@ -180,7 +181,10 @@ contract GovernorBravoDelegate is GovernorBravoDelegateStorageV2, GovernorBravoE
/**
* @notice Gets actions of a proposal
* @param proposalId the id of the proposal
* @return Targets, values, signatures, and calldatas of the proposal actions
* @return targets of the proposal actions
* @return values of the proposal actions
* @return signatures of the proposal actions
* @return calldatas of the proposal actions
*/
function getActions(uint proposalId) external view returns (address[] memory targets, uint[] memory values, string[] memory signatures, bytes[] memory calldatas) {
Proposal storage p = proposals[proposalId];
Expand Down Expand Up @@ -292,7 +296,15 @@ contract GovernorBravoDelegate is GovernorBravoDelegateStorageV2, GovernorBravoE
* @return If the account is whitelisted
*/
function isWhitelisted(address account) public view returns (bool) {
return (whitelistAccountExpirations[account] > now);
uint currentBlockTimestamp = getBlockTimestamp();
Copy link

Choose a reason for hiding this comment

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

Strange idea, it would change nothing only making deployment gas cost increase.

return (whitelistAccountExpirations[account] > currentBlockTimestamp);
}

/**
* @dev Function to simply retrieve block timestamp
*/
function getBlockTimestamp() internal view returns (uint) {
return block.timestamp;
}

/**
Expand Down Expand Up @@ -424,7 +436,7 @@ contract GovernorBravoDelegate is GovernorBravoDelegateStorageV2, GovernorBravoE
return a - b;
}

function getChainIdInternal() internal pure returns (uint) {
function getChainIdInternal() internal view returns (uint) {
uint chainId;
assembly { chainId := chainid() }
return chainId;
Expand Down