Add fork test tooling #419

shahthepro · 2024-05-15T18:44:14Z

If you made a contract change, make sure to complete the checklist below before merging it in master.

Contract change checklist:

Code reviewed by 2 reviewers.
Copy & paste code review security checklist below this checklist.
Unit tests pass
Slither tests pass with no warning
Echidna tests pass if PR includes changes to OUSD contract (not automated, run manually on local)

DanielVF · 2024-05-16T21:10:44Z

script/deploy/mainnet/012_xOGNGovernanceScript.sol

+    }
+}
+
+contract XOGNGovernanceScript is BaseMainnetScript {


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.

DanielVF · 2024-05-17T14:08:56Z

script/deploy/mainnet/010_xOGNSetupScript.sol

+
+        //
+        // 3. Rewards implimentation and init
+        uint256 rewardsPerSecond = 0; //TODO: Decide on the params


This will be 300,000 per day. (300000 * 1e18 raw)

Probably easiest to write it for clarity as the per day rate divided by the number of seconds in a day.

DanielVF · 2024-05-20T15:40:29Z

script/deploy/mainnet/010_xOGNSetupScript.sol

+
+        //
+        // 3. Rewards implimentation and init
+        uint256 rewardsPerSecond = 0; //TODO: Decide on the params


We can remove the TODO comment.

sparrowDom · 2024-05-20T16:03:32Z

contracts/FixedRateRewardsSource.sol

@@ -90,10 +90,6 @@ contract FixedRateRewardsSource is Governable, Initializable {
    function previewRewards() public view returns (uint256 rewardAmount) {
        RewardConfig memory _config = rewardConfig;

-        if (_config.lastCollect == 0) {


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

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

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

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.

sparrowDom · 2024-05-20T17:19:54Z

contracts/FixedRateRewardsSource.sol

@@ -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


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. */

This is done

sparrowDom

Rewards source emission bit LGTM

DanielVF · 2024-05-20T17:45:25Z

script/deploy/mainnet/011_OgnOgvMigrationScript.sol

+        _buildGnosisTx();
+    }
+
+    function _buildGnosisTx() internal {


I'd rather call this something like _buildGovernance. Most of the time gnosis will only indirectly be involved, and we will be making governance proposals. Even outside this we'll talking to the timelock.

DanielVF · 2024-05-20T17:46:18Z

script/deploy/mainnet/011_OgnOgvMigrationScript.sol

+            abi.encode(timelock.EXECUTOR_ROLE(), Addresses.GOVERNOR_FIVE)
+        );
+
+        govFive.action(Addresses.OUSD_BUYBACK, "upgradeTo(address)", abi.encode(Addresses.OUSD_BUYBACK_IMPL)); // Todo, use latest deployed address


Are these TODO's still TODO's?

DanielVF · 2024-05-20T17:50:08Z

script/deploy/mainnet/011_OgnOgvMigrationScript.sol

+        // Mint token proposal from OGN governance
+        IMintableERC20 ogv = IMintableERC20(Addresses.OGV);
+        // Mint additional OGN, will get returned after starting migration
+        uint256 ognToMint = ((ogv.totalSupply() * 0.09137 ether) / 1 ether) + 1_000_000 ether;


Todo here.

I think we'll be minting the exact amount mentioned in the snapshot, then transferring in the remaining

We probably want to sim out offline what the inflation will be by the 30th, and then use that as the total required.

DanielVF · 2024-05-20T17:51:25Z

script/deploy/mainnet/011_OgnOgvMigrationScript.sol

+        bytes[] memory calldatas = new bytes[](1);
+
+        // OGN Gov 1: Mint OGN
+        targets[0] = Addresses.OGN;


TODO: This will change when the ownership on OGN changes to the DeFi timelock

DanielVF · 2024-05-20T18:58:11Z

script/deploy/DeployManager.sol

+            deployedContracts[name] = addr;
+        }
+
+        vm.writeJson(networkDeployments, deploymentsFilePath, contractsKey);


I'm still getting a lot of random failure on these writeJson's (about 2/3 of the 50% of tests fail, about 1/6 of the time, all tests succeed)

Yeah. It's mostly about writing twice to the same file back to back. Thats why I added a small delay. Will work on a clean fix to do a single write instead

* Deploy 010 * Fix fork tests * Deploy only once for fork tests * Add another sleep

* Add OGNRewardsSource contract * Make collectRewards only callable by RewardsTarget * Draft xOGN staking contract * Correct maxStakeDuration * Add penalty event * Change names * Fix lockup ID * Revert change and cast properly * Gas opts * Remove casting * Add `getLockupsCount` method (#411) * Allow non-duration change amount increase staking extends * Add tests, add move lockupid code * Add Migrator (#410) * Add Migrator contract * Fix some tests * Code review changes * Update OgvStaking tests * Disable delegation tests * Allow just unstakes * Fix comment * More cleanup * Fix brownie tests * Return excess OGN rather than burn * Simplify calculation * Return 0 if uninitialized (#415) * Check available balance in `previewRewards` (#413) * Check available balance in `previewRewards` * Chore: forge fmt --------- Co-authored-by: Daniel Von Fange <daniel@leancoder.com> * Fix: Remove unused errors (#416) * First draft of deploy file * Add fork test tooling (#419) --------- Co-authored-by: Shahul Hameed <10547529+shahthepro@users.noreply.github.com>

shahthepro added 4 commits May 15, 2024 22:42

Add fork test tooling

df1e6f4

Update tooling to record script executions

aaae7d6

Merge branch 'DanielVF/DeployFiles' into shah/fork-test-tooling

d3f4806

Add a bit of tests

28e29a7

DanielVF reviewed May 16, 2024

View reviewed changes

Fix brownie tests

cc01b28

DanielVF reviewed May 17, 2024

View reviewed changes

shahthepro added 6 commits May 19, 2024 20:44

Add fork tests

516937b

Print safe tx data

383f2df

Print tx data only on mainnet

e5582e1

Add Migrator tests

8526d1d

Fix CI

9728d78

Brownie remap

5a7e530

DanielVF reviewed May 20, 2024

View reviewed changes

sparrowDom reviewed May 20, 2024

View reviewed changes

DanielVF reviewed May 20, 2024

View reviewed changes

shahthepro added 2 commits May 21, 2024 22:54

Cherry pick CR comments

efd24a5

Deploy 010 - OGN Rewards Source and xOGN (#420)

14d1073

* Deploy 010 * Fix fork tests * Deploy only once for fork tests * Add another sleep

shahthepro merged commit a699776 into DanielVF/DeployFiles May 22, 2024
4 checks passed

shahthepro deleted the shah/fork-test-tooling branch May 22, 2024 11:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fork test tooling #419

Add fork test tooling #419

shahthepro commented May 15, 2024

DanielVF May 16, 2024

DanielVF May 17, 2024 •

edited

DanielVF May 20, 2024

sparrowDom May 20, 2024

shahthepro May 20, 2024 •

edited

sparrowDom May 20, 2024

DanielVF May 20, 2024

sparrowDom May 20, 2024

shahthepro May 22, 2024

sparrowDom left a comment

DanielVF May 20, 2024

DanielVF May 20, 2024

DanielVF May 20, 2024

DanielVF May 20, 2024

DanielVF May 20, 2024

shahthepro May 21, 2024

Add fork test tooling #419

Add fork test tooling #419

Conversation

shahthepro commented May 15, 2024

Choose a reason for hiding this comment

DanielVF May 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shahthepro May 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sparrowDom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielVF May 17, 2024 •

edited

shahthepro May 20, 2024 •

edited