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 fork test tooling #419

Merged
merged 13 commits into from
May 22, 2024
Merged

Add fork test tooling #419

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
df1e6f4
Add fork test tooling
shahthepro May 15, 2024
aaae7d6
Update tooling to record script executions
shahthepro May 16, 2024
d3f4806
Merge branch 'DanielVF/DeployFiles' into shah/fork-test-tooling
shahthepro May 16, 2024
28e29a7
Add a bit of tests
shahthepro May 16, 2024
cc01b28
Fix brownie tests
shahthepro May 17, 2024
516937b
Add fork tests
shahthepro May 19, 2024
383f2df
Print safe tx data
shahthepro May 19, 2024
e5582e1
Print tx data only on mainnet
shahthepro May 19, 2024
8526d1d
Add Migrator tests
shahthepro May 19, 2024
9728d78
Fix CI
shahthepro May 19, 2024
5a7e530
Brownie remap
shahthepro May 19, 2024
efd24a5
Cherry pick CR comments
shahthepro May 21, 2024
14d1073
Deploy 010 - OGN Rewards Source and xOGN (#420)
shahthepro May 22, 2024
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
9 changes: 5 additions & 4 deletions contracts/FixedRateRewardsSource.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,6 @@ contract FixedRateRewardsSource is Governable, Initializable {
function previewRewards() public view returns (uint256 rewardAmount) {
RewardConfig memory _config = rewardConfig;

if (_config.lastCollect == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Currently deploy script 10 deploys the contract and deploy script 12 sets the rewards per second. I would leave this check in since it is a sanity check and not very gas expensive one at that.

With the current code of the contract, the contract needs to be deployed and funded in the correct order, or it will not function properly. Since one can:

  • run deploy script 10 that leaves the rewardsPerSecond to 0
  • fund the contract
  • call collectRewards and the function will probably revert, since previewRewards will return a too big of a number.

I think the correct functioning of the contract should be assured even if the deploy is not configured perfectly as expected. For that reason I would:

  • re-introduce the above removed lines of code and add comments why it is imperative that they are present
  • remove the potentially confusing _rewardsPerSecond from the initialize function. Any deployment of this contract will suffer the same issues on initialisation of rewards source and start of emission.
  • add additional comments explicitly stating that the contract is basically paused until the setRewardsPerSecond isn't called with a non 0 value.
  • currently it is also possible that the rewards source is already running (funded and non 0 rewardsPerSecond) and governor calls the setRewardsPerSecond with a 0 value. This would in effect pause the contract and remove any rewards accrued since the last collect. Is that intentional? If not change the _setRewardsPerSecond to:
    • either make it impossible to call the function with the 0 parameter
    • or collect the rewards and send it to a configured target. And pause the rewards source by setting the rewards to 0

Copy link
Collaborator Author

@shahthepro shahthepro May 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Currently deploy script 10 deploys the contract and deploy script 12 sets the rewards per second. I would leave this check in since it is a sanity check and not very gas expensive one at that.

As of now, The deploy script initializes the proxy and implementation in the same call. So, once the proxy is initialized, it'll always have a lastCollect set and will never be zero. That's the reason I removed that check as it seemed redundant.

call collectRewards and the function will probably revert, since previewRewards will return a too big of a number

If the rewardsPerSecond isn't changed, won't previewRewards just return zero (and 0 reward collected)? Why would it revert?

remove the potentially confusing _rewardsPerSecond from the initialize function

Agree with this, will make this change

Is that intentional?
collect the rewards and send it to a configured target. And pause the rewards source by setting the rewards to 0

Yep. xOGN Governance (rewardsTarget) is only contract that can call collectRewards. Also, xOGN Governance doesn't account for any rewards sent to it directly (It does a balance check before and after collectRewards to figure out the diff and uses that as reward collected). So, collecting rewards internally won't be possible when changing the rate

either make it impossible to call the function with the 0 parameter

I'm not sure if we want the ability to pause rewards in future for any reason. Let me check with @DanielVF

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for correcting me on first 2 points; you're right they are not an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm good with a change to rewards rate potentially changing how much users receive for the past, as well as the future. We'll probably usually couple it with a xOGN collect in most changes though.

Having a "pause" feature that stops rewards could be a useful thing.

return 0;
}

rewardAmount = (block.timestamp - _config.lastCollect) * _config.rewardsPerSecond;
uint256 balance = IERC20(rewardToken).balanceOf(address(this));
if (rewardAmount > balance) {
Expand Down Expand Up @@ -139,6 +135,11 @@ contract FixedRateRewardsSource is Governable, Initializable {
// Update storage
RewardConfig storage _config = rewardConfig;
emit RewardsPerSecondChanged(_rewardsPerSecond, _config.rewardsPerSecond);
if (_config.rewardsPerSecond == 0) {
// When changing rate from zero to non-zero,
// Update lastCollect timestamp as well
Copy link
Member

Choose a reason for hiding this comment

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

I only have 1 more nitpick:
I would expand this comment to

/* This contract code allows for contract deployment & initialization and then the contract can be live for quite
 * some time before it is funded and `_rewardsPerSecond` are set to non 0 value. In that case the vesting period 
 * from contract initialization until now would be taken into account instead of the time since the contract has been
 * "activated" by setting the `setRewardsPerSecond`. To mitigate the issue we update the `_config.lastCollect`
 * to current time. 
 */

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is done

_config.lastCollect = uint64(block.timestamp);
}
_config.rewardsPerSecond = _rewardsPerSecond;
}
}
2 changes: 1 addition & 1 deletion contracts/OgvStaking.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ contract OgvStaking is ERC20Votes {
/// @param duration number of seconds to stake for
/// @return points staking points that would be returned
/// @return end staking period end date
function previewPoints(uint256 amount, uint256 duration) public view returns (uint256, uint256) {
function previewPoints(uint256 amount, uint256 duration) public pure returns (uint256, uint256) {
revert StakingDisabled();
}

Expand Down
9 changes: 9 additions & 0 deletions contracts/interfaces/IMintableERC20.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// SPDX-License-Identifier: Unlicense
pragma solidity 0.8.10;

interface IMintableERC20 {
function mint(address to, uint256 amount) external;
function balanceOf(address owner) external view returns (uint256);
function transfer(address to, uint256 amount) external returns (bool);
function approve(address spender, uint256 allowance) external;
}
2 changes: 1 addition & 1 deletion contracts/tests/MockRewardsSource.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ pragma solidity 0.8.10;
contract MockRewardsSource {
constructor() {}

function previewRewards() external view returns (uint256) {
function previewRewards() external pure returns (uint256) {
return 0;
}

Expand Down
2 changes: 1 addition & 1 deletion contracts/tests/TestToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ pragma solidity ^0.8.4;
import "../GovernanceToken.sol";

contract TestToken is OriginDollarGovernance {
function proof() public {
function proof() public pure {
revert("Upgraded");
}
}
4 changes: 2 additions & 2 deletions contracts/utils/Addresses.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,6 @@ library Addresses {

address public constant OUSD_BUYBACK = 0xD7B28d06365b85933c64E11e639EA0d3bC0e3BaB;
address public constant OETH_BUYBACK = 0xFD6c58850caCF9cCF6e8Aee479BFb4Df14a362D2;
address public constant OUSD_BUYBACK_IMPL = 0xbc77B8EFafabdF46f94Dfb4A422d541c5037799C;
address public constant OETH_BUYBACK_IMPL = 0x69D343A52bC13Dc19cBD0d2A77baC320CCB69B9a;
address public constant OUSD_BUYBACK_IMPL = 0x386d8fEC5b6d5B5E36a48A376644e36239dB65d6;
address public constant OETH_BUYBACK_IMPL = 0x4F11d31f781B57051764a3823b24d520626b4833;
}
52 changes: 52 additions & 0 deletions contracts/utils/GovFive.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// SPDX-License-Identifier: MIT

pragma solidity 0.8.10;

import {Vm} from "forge-std/Vm.sol";
import {Addresses} from "contracts/utils/Addresses.sol";
import "forge-std/console.sol";

import "OpenZeppelin/openzeppelin-contracts@4.6.0/contracts/utils/Strings.sol";

library GovFive {
struct GovFiveAction {
address receiver;
string fullsig;
bytes data;
}

struct GovFiveProposal {
string name;
string description;
GovFiveAction[] actions;
}

function setName(GovFiveProposal storage prop, string memory name) internal {
prop.name = name;
}

function setDescription(GovFiveProposal storage prop, string memory description) internal {
prop.description = description;
}

function action(GovFiveProposal storage prop, address receiver, string memory fullsig, bytes memory data)
internal
{
prop.actions.push(GovFiveAction({receiver: receiver, fullsig: fullsig, data: data}));
}

function execute(GovFiveProposal storage prop) internal {
address VM_ADDRESS = address(uint160(uint256(keccak256("hevm cheat code"))));
Vm vm = Vm(VM_ADDRESS);
for (uint256 i = 0; i < prop.actions.length; i++) {
GovFiveAction memory propAction = prop.actions[i];
bytes memory sig = abi.encodePacked(bytes4(keccak256(bytes(propAction.fullsig))));
vm.prank(Addresses.TIMELOCK);
(bool success, bytes memory data) = propAction.receiver.call(abi.encodePacked(sig, propAction.data));
if (!success) {
console.log(propAction.fullsig);
revert("Multisig action failed");
}
}
}
}
2 changes: 2 additions & 0 deletions foundry.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ src = 'contracts'
test = 'tests'
remappings = [
"contracts/=./contracts",
"script/=./script",
"tests/=./tests",
"OpenZeppelin/openzeppelin-contracts@02fcc75bb7f35376c22def91b0fb9bc7a50b9458/=./lib/openzeppelin-contracts",
"OpenZeppelin/openzeppelin-contracts-upgradeable@a16f26a063cd018c4c986832c3df332a131f53b9/=./lib/openzeppelin-contracts-upgradeable",
"OpenZeppelin/openzeppelin-contracts@4.6.0/=./lib/openzeppelin-contracts",
Expand Down
1 change: 0 additions & 1 deletion script/deploy/DeployManager.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ contract DeployManager is Script {
/**
* Pre-deployment
*/
BaseMainnetScript.DeployRecord[] memory deploys;
string memory networkDeployments = "";
string[] memory existingContracts = vm.parseJsonKeys(fileContents, contractsKey);
for (uint256 i = 0; i < existingContracts.length; ++i) {
Expand Down
16 changes: 12 additions & 4 deletions script/deploy/mainnet/010_xOGNSetupScript.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@ import {OgvStaking} from "contracts/OgvStaking.sol";
import {Migrator} from "contracts/Migrator.sol";
import {Timelock} from "contracts/Timelock.sol";

contract XOGNSetupScript is BaseMainnetScript {
FixedRateRewardsSourceProxy ognRewardsSourceProxy;
OgvStaking veOgvImpl;
import {IMintableERC20} from "contracts/interfaces/IMintableERC20.sol";

contract XOGNSetupScript is BaseMainnetScript {
string public constant override DEPLOY_NAME = "010_xOGNSetup";

constructor() {}
Expand Down Expand Up @@ -51,7 +50,8 @@ contract XOGNSetupScript is BaseMainnetScript {

//
// 3. Rewards implimentation and init
uint256 rewardsPerSecond = 0; //TODO: Decide on the params
// Reward rate is 0, Will be set later when we want initiate rewards
uint256 rewardsPerSecond = 0;
FixedRateRewardsSource fixedRateRewardsSourceImpl = new FixedRateRewardsSource(Addresses.OGN);
_recordDeploy("OGN_REWARDS_SOURCE_IMPL", address(fixedRateRewardsSourceImpl));

Expand All @@ -62,4 +62,12 @@ contract XOGNSetupScript is BaseMainnetScript {
);
ognRewardsSourceProxy.initialize(address(fixedRateRewardsSourceImpl), Addresses.TIMELOCK, implInitData);
}

function _fork() internal override {
IMintableERC20 ogn = IMintableERC20(Addresses.OGN);

// Mint enough OGN to fund 100 days of rewards
vm.prank(Addresses.OGN_GOVERNOR);
ogn.mint(deployedContracts["OGN_REWARDS_SOURCE"], 30_000_000 ether);
}
}
41 changes: 1 addition & 40 deletions script/deploy/mainnet/011_OgnOgvMigrationScript.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,46 +16,7 @@ import {FixedRateRewardsSource} from "contracts/FixedRateRewardsSource.sol";
import {OgvStaking} from "contracts/OgvStaking.sol";
import {Migrator} from "contracts/Migrator.sol";
import {Timelock} from "contracts/Timelock.sol";

// To be extracted
library GovFive {
struct GovFiveAction {
address receiver;
string fullsig;
bytes data;
}

struct GovFiveProposal {
string name;
string description;
GovFiveAction[] actions;
}

function setName(GovFiveProposal storage prop, string memory name) internal {
prop.name = name;
}

function setDescription(GovFiveProposal storage prop, string memory description) internal {
prop.description = description;
}

function action(GovFiveProposal storage prop, address receiver, string memory fullsig, bytes memory data)
internal
{
prop.actions.push(GovFiveAction({receiver: receiver, fullsig: fullsig, data: data}));
}

function execute(GovFiveProposal storage prop) internal {
address VM_ADDRESS = address(uint160(uint256(keccak256("hevm cheat code"))));
Vm vm = Vm(VM_ADDRESS);
for (uint256 i = 0; i < prop.actions.length; i++) {
GovFiveAction memory action = prop.actions[i];
bytes memory sig = abi.encodePacked(bytes4(keccak256(bytes(action.fullsig))));
vm.prank(Addresses.TIMELOCK);
action.receiver.call(abi.encodePacked(sig, action.data));
}
}
}
import {GovFive} from "contracts/utils/GovFive.sol";

contract OgnOgvMigrationScript is BaseMainnetScript {
using GovFive for GovFive.GovFiveProposal;
Expand Down
59 changes: 17 additions & 42 deletions script/deploy/mainnet/012_xOGNGovernanceScript.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,56 +18,21 @@ import {Migrator} from "contracts/Migrator.sol";
import {Timelock} from "contracts/Timelock.sol";
import {Governance} from "contracts/Governance.sol";

import {GovFive} from "contracts/utils/GovFive.sol";

import "OpenZeppelin/openzeppelin-contracts@4.6.0/contracts/token/ERC20/extensions/ERC20Votes.sol";
import "OpenZeppelin/openzeppelin-contracts@4.6.0/contracts/governance/TimelockController.sol";

// To be extracted
library GovFive {
struct GovFiveAction {
address receiver;
string fullsig;
bytes data;
}

struct GovFiveProposal {
string name;
string description;
GovFiveAction[] actions;
}

function setName(GovFiveProposal storage prop, string memory name) internal {
prop.name = name;
}

function setDescription(GovFiveProposal storage prop, string memory description) internal {
prop.description = description;
}

function action(GovFiveProposal storage prop, address receiver, string memory fullsig, bytes memory data)
internal
{
prop.actions.push(GovFiveAction({receiver: receiver, fullsig: fullsig, data: data}));
}

function execute(GovFiveProposal storage prop) internal {
address VM_ADDRESS = address(uint160(uint256(keccak256("hevm cheat code"))));
Vm vm = Vm(VM_ADDRESS);
for (uint256 i = 0; i < prop.actions.length; i++) {
GovFiveAction memory action = prop.actions[i];
bytes memory sig = abi.encodePacked(bytes4(keccak256(bytes(action.fullsig))));
vm.prank(Addresses.TIMELOCK);
action.receiver.call(abi.encodePacked(sig, action.data));
}
}
}

contract XOGNGovernanceScript is BaseMainnetScript {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the real world this will be the multisig calling the timelock. However this is a lower priority. First we need to make sure that main switch over has enough tests on it.

using GovFive for GovFive.GovFiveProposal;

GovFive.GovFiveProposal govFive;

string public constant override DEPLOY_NAME = "012_xOGNGovernance";

uint256 constant OGN_EPOCH = 1717041600; // May 30, 2024 GMT
uint256 constant REWARDS_PER_SECOND = 300000 ether / uint256(24 * 60 * 60); // 300k per day

constructor() {}

function _execute() internal override {
Expand All @@ -86,14 +51,24 @@ contract XOGNGovernanceScript is BaseMainnetScript {

address xognGov = deployedContracts["XOGN_GOV"];

govFive.setName("Enable OGN Governance");
// Todo: Fuller description
govFive.setName("Enable OGN Governance & Begin Rewards");

govFive.setDescription("Grant roles on Timelock to OGN Governance");

govFive.action(Addresses.TIMELOCK, "grantRole(bytes32,address)", abi.encode(timelock.PROPOSER_ROLE(), xognGov));
govFive.action(Addresses.TIMELOCK, "grantRole(bytes32,address)", abi.encode(timelock.CANCELLER_ROLE(), xognGov));
govFive.action(Addresses.TIMELOCK, "grantRole(bytes32,address)", abi.encode(timelock.EXECUTOR_ROLE(), xognGov));

// Enable rewards
govFive.action(
deployedContracts["OGN_REWARDS_SOURCE"],
"setRewardsPerSecond(uint192)",
abi.encode(uint192(REWARDS_PER_SECOND))
);

// Go to the start of everything
vm.warp(OGN_EPOCH);

govFive.execute(); // One day lives up a level, and this contract returns a generic governance struct with a function pointers
}
}
Loading
Loading