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

Add fork test tooling #419

Merged
merged 13 commits into from
May 22, 2024
18 changes: 17 additions & 1 deletion .github/workflows/main.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,20 @@ jobs:
uses: foundry-rs/foundry-toolchain@v1

- name: Run tests
run: forge test -vvv
run: forge test --no-match-contract "(Fork)" -vvv

foundry-fork-tests:
name: Foundry Fork tests
runs-on: ubuntu-latest
env:
PROVIDER_URL: ${{ secrets.PROVIDER_URL }}
steps:
- uses: actions/checkout@v3
with:
submodules: recursive

- name: Install Foundry
uses: foundry-rs/foundry-toolchain@v1

- name: Run tests
run: forge test --match-contract "ForkTest" --fork-url $PROVIDER_URL
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,5 @@ out/
.vscode
brownie-deploy/
.idea
deployments-fork*.json
broadcast/*
3 changes: 2 additions & 1 deletion .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
url = https://github.com/openzeppelin/openzeppelin-contracts-upgradeable
[submodule "lib/forge-std"]
path = lib/forge-std
url = https://github.com/brockelmore/forge-std
url = https://github.com/foundry-rs/forge-std
branch = 978ac6fadb62f5f0b723c996f64be52eddba6801
[submodule "lib/prb-math"]
path = lib/prb-math
url = https://github.com/paulrberg/prb-math
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,13 @@ forge install
forge test
```

## Running fork tests (forge)

```bash
forge install
forge test --fork-url $ALCHEMY_PROVIDER_URL -vvv --mc "ForkTest"
```

## Running a local node

Copy `dev.env` to `.env` and fill out the `PROVIDER_URL`
Expand Down
4 changes: 4 additions & 0 deletions brownie-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,7 @@ dependencies:
- OpenZeppelin/openzeppelin-contracts@4.6.0
- OpenZeppelin/openzeppelin-contracts-upgradeable@4.6.0
- paulrberg/prb-math@2.5.0
compiler:
solc:
remappings:
- forge-std/=./lib/forge-std/src/
13 changes: 13 additions & 0 deletions build/deployments.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"1": {
"executions": {
"010_xOGNSetup": 1716312107
},
"contracts": {
"OGN_REWARDS_SOURCE": "0x7609c88E5880e934dd3A75bCFef44E31b1Badb8b",
"OGN_REWARDS_SOURCE_IMPL": "0x16890bdd817Ed1c4654430d67329CB20b0B71bB0",
"XOGN": "0x63898b3b6Ef3d39332082178656E9862bee45C57",
"XOGN_IMPL": "0x97711c7a5D64A064a95d10e37f786d2bD8b1F3c8"
}
}
}
21 changes: 10 additions & 11 deletions contracts/FixedRateRewardsSource.sol
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,12 @@ contract FixedRateRewardsSource is Governable, Initializable {
/// @dev Initialize the proxy implementation
/// @param _strategistAddr Address of the Strategist
/// @param _rewardsTarget Address that receives rewards
/// @param _rewardsPerSecond Rate of reward emission
function initialize(address _strategistAddr, address _rewardsTarget, uint192 _rewardsPerSecond)
external
initializer
{
function initialize(address _strategistAddr, address _rewardsTarget) external initializer {
_setStrategistAddr(_strategistAddr);
_setRewardsTarget(_rewardsTarget);

// Rewards start from the moment the contract is initialized
rewardConfig.lastCollect = uint64(block.timestamp);

_setRewardsPerSecond(_rewardsPerSecond);
}

/// @dev Collect pending rewards
Expand Down Expand Up @@ -90,10 +84,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

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 +129,15 @@ contract FixedRateRewardsSource is Governable, Initializable {
// Update storage
RewardConfig storage _config = rewardConfig;
emit RewardsPerSecondChanged(_rewardsPerSecond, _config.rewardsPerSecond);
if (_config.rewardsPerSecond == 0) {
/* 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.
*/
_config.lastCollect = uint64(block.timestamp);
}
_config.rewardsPerSecond = _rewardsPerSecond;
}
}
6 changes: 3 additions & 3 deletions contracts/Governance.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ contract Governance is
GovernorPreventLateQuorum
{
constructor(ERC20Votes _token, TimelockController _timelock)
Governor("OUSD Governance")
GovernorSettings(1, /* 1 block */ 17280, /* ~3 days (86400 / 15) * 3 */ 10000000 * 1e18 /* 10 mio veOgv */ )
Governor("Origin DeFi Governance")
GovernorSettings(1, /* 1 block */ 14416, /* ~2 days (86400 / 12) * 2 */ 100000 * 1e18 /* 100k xOGN */ )
GovernorVotes(_token)
GovernorVotesQuorumFraction(20) // Default quorum denominator is 100, so 20/100 or 20%
GovernorTimelockControl(_timelock)
GovernorPreventLateQuorum(11520) // ~2 days (86400 / 15) * 2
GovernorPreventLateQuorum(7208) // ~1 days (86400 / 12)
{}

// The following functions are overrides required by Solidity.
Expand Down
2 changes: 1 addition & 1 deletion contracts/OgvStaking.sol
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
10 changes: 10 additions & 0 deletions contracts/interfaces/IMintableERC20.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// 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 totalSupply() external view returns (uint256);
function transfer(address to, uint256 amount) external returns (bool);
function approve(address spender, uint256 allowance) external;
}
15 changes: 15 additions & 0 deletions contracts/interfaces/IOGNGovernance.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// SPDX-License-Identifier: Unlicense
pragma solidity 0.8.10;

interface IOGNGovernance {
function state(uint256 proposalId) external view returns (uint256);
function proposalCount() external view returns (uint256);
function queue(uint256 proposalId) external;
function execute(uint256 proposalId) external;
function propose(
address[] memory targets,
string[] memory signatures,
bytes[] memory calldatas,
string memory description
) external returns (uint256);
}
2 changes: 1 addition & 1 deletion contracts/tests/MockRewardsSource.sol
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
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");
}
}
11 changes: 7 additions & 4 deletions contracts/utils/Addresses.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,17 @@ library Addresses {
address public constant STRATEGIST = 0xF14BBdf064E3F67f51cd9BD646aE3716aD938FDC;
address public constant GOVERNOR_FIVE = 0x3cdD07c16614059e66344a7b579DAB4f9516C0b6;

address public constant OGN_GOVERNOR = 0x72426BA137DEC62657306b12B1E869d43FeC6eC7;
address public constant GOV_MULTISIG = 0xbe2AB3d3d8F6a32b96414ebbd865dBD276d3d899;

address public constant INITIAL_DEPLOYER = address(0x1001);
address public constant OGN = 0x8207c1FfC5B6804F6024322CcF34F29c3541Ae26;
address public constant OGV = 0x9c354503C38481a7A7a51629142963F98eCC12D0;
address public constant OGV_REWARDS_PROXY = 0x7d82E86CF1496f9485a8ea04012afeb3C7489397;
address public constant VEOGV = 0x0C4576Ca1c365868E162554AF8e385dc3e7C66D9;

address public constant OUSD_BUYBACK = address(34);
address public constant OETH_BUYBACK = address(35);
address public constant OUSD_BUYBACK_IMPL = address(34);
address public constant OETH_BUYBACK_IMPL = address(35);
address public constant OUSD_BUYBACK = 0xD7B28d06365b85933c64E11e639EA0d3bC0e3BaB;
address public constant OETH_BUYBACK = 0xFD6c58850caCF9cCF6e8Aee479BFb4Df14a362D2;
address public constant OUSD_BUYBACK_IMPL = 0x386d8fEC5b6d5B5E36a48A376644e36239dB65d6;
address public constant OETH_BUYBACK_IMPL = 0x4F11d31f781B57051764a3823b24d520626b4833;
}
67 changes: 67 additions & 0 deletions contracts/utils/GovFive.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// 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 printTxData(GovFiveProposal storage prop) internal {
console.log("-----------------------------------");
console.log("Create following tx on Gnosis safe:");
console.log("-----------------------------------");
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))));

console.log("### Tx", i + 1);
console.log("Address:", propAction.receiver);
console.log("Data:");
console.logBytes(abi.encodePacked(sig, propAction.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");
}
}
}
}
6 changes: 6 additions & 0 deletions foundry.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,15 @@ 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",
"OpenZeppelin/openzeppelin-contracts-upgradeable@4.6.0/=./lib/openzeppelin-contracts-upgradeable",
"paulrberg/prb-math@2.5.0/=./lib/prb-math"
]
fs_permissions = [{ access = "read-write", path = "./build"}]
extra_output_files = [
"metadata"
]
Loading
Loading