Honey DRIP #6

anisoptera · 2020-11-23T00:31:15Z

This implements a Honeycomb DRIP (Dividend Reinvestment Program) in our Unipool fork, as described in https://forum.1hive.org/t/proposal-make-honey-sticky-automatic-dividend-reinvestment-vault/916 , just in time for the new farming proposal.

It uses essentially the same mechanism as the original external contract did, and just piggybacks on the existing earnings tracking to keep track of the dust.

Requires threading the router address through the infra. Existing tests pass. New tests don't exist yet.

fix up path to use the token, not the pool actually approve the router for our contract

Avoid shavings of token B piling up

willjgriff

Good start!

contracts/Unipool.sol

contracts/UnipoolFactory.sol

test/Unipool.js

contracts/Unipool.sol

contracts/UnipoolFactory.sol

contracts/mock/UniswapRouterMock.sol

willjgriff · 2020-12-01T10:40:04Z

migrations/2_deploy.js

+        await deployer.deploy(UniswapRouterMock);
+
+        await deployer.deploy(UnipoolMock, uniswapToken.address, HoneyTokenMock.address, UniswapRouterMock.address);
+        await deployer.deploy(UnipoolForeignPairMock, uniswapToken2.address, HoneyTokenMock.address, UniswapRouterMock.address);


Deployments to Rinkeby can actually use the Rinkeby deployment of Uniswap, instead of mocks. See if you can use that instead.

I probably can, and if I'd started out planning to do it that way it makes a lot of sense, but the tests are built assuming we can do things like mint arbitrary amounts of the pool tokens. And we'd still need the mocks for the local tests anyway.

If you think it's super important I can grind it out and rewrite the tests, but I'm not sure it's worth it at this stage.

The unit tests aren't run on Rinkeby. Rinkeby deployment's are only for QA testing and we shouldn't use mocks on Rinkeby if we can avoid it, which I think we can here.

migrations/2_deploy.js

test/UnipoolFactory.js

test/Unipool.js

(mostly missed naming issues)

willjgriff

Looking good!

You seem to have missed some of the comments from my previous review though which are still "unresolved", please check and do them.

When I run the tests some of them fail. I run with npm run test.

Some of the import statement comments in the first test file can be considered for all of the test files, please check and update the other test files where required.

willjgriff · 2021-01-04T14:48:53Z

contracts/Unipool.sol

+        } else if (pair.token1() == rewardToken) {
+            return pair.token0();
+        } else {
+            // We can only reinvest if we're giving rewards in one side of the pair (currently)


I wouldn't imply future development, we have no idea what we will and won't do so remove the "(currently)".

willjgriff · 2021-01-04T15:13:22Z

migrations/2_deploy.js

+const UnipoolForeignPairMock = artifacts.require('./UnipoolMock.sol');
 const HoneyTokenMock = artifacts.require('./HoneyTokenMock.sol');
-const UniswapTokenMock = artifacts.require('./UniswapTokenMock.sol');
+const HONEY_TOKEN_XDAI_ADDRESS = '0x71850b7E9Ee3f13Ab46d67167341E4bDc905Eef9';


This should be separate from the artifacts.require imports. Put it at a line below them.

willjgriff · 2021-01-04T15:28:29Z

test/Unipool.js


-const UniswapToken = artifacts.require('UniswapTokenMock');
+const UniswapPair = artifacts.require('UniswapPairMock');
+const OtherToken = artifacts.require('UniswapTokenMock');


This const is of type "UniswapToken" not "OtherToken". OtherToken is an instance of UniswapToken. Rename this to "UniswapToken"

willjgriff · 2021-01-04T15:31:35Z

test/Unipool.js

-const UniswapToken = artifacts.require('UniswapTokenMock');
+const UniswapPair = artifacts.require('UniswapPairMock');
+const OtherToken = artifacts.require('UniswapTokenMock');
 const TradedToken = artifacts.require('HoneyTokenMock');


Rename "HoneyTokenMock" to "TokenMock" and rename this const to "Token". Then the "OtherToken" above can be removed and where it's created below it can use this new "Token" object.

willjgriff · 2021-01-04T15:43:11Z

test/Unipool.js

-            this.unipool = await Unipool.new(this.uniswapToken.address, this.tradedToken.address);
+            this.otherToken = await OtherToken.new();
+            this.rewardToken = await TradedToken.new(wallet1);
+            this.uniswapToken = await UniswapPair.new(this.rewardToken.address, this.otherToken.address);


Call this "uniswapPair" as it's of type UniswapPair.

willjgriff · 2021-01-04T15:57:50Z

test/Unipool.js

+
+            await this.unipool.reinvestReward({ from: wallet1 });
+
+            expect(await this.unipool.earned(wallet1)).to.be.bignumber.almostEqualDiv1e18(web3.utils.toWei('0'));


Shouldn't need to convert the 0 to wei here, just use 0.

willjgriff · 2021-01-04T16:01:44Z

migrations/2_deploy.js


        await deployer.deploy(HoneyTokenMock, senderAccount);
+        await deployer.deploy(OtherTokenMock);
+        await deployer.deploy(UniswapPairMock, HoneyTokenMock.address, OtherTokenMock.address);


This can be

const uniswapToken = await deployer.deploy(UniswapPairMock, HoneyTokenMock.address, OtherTokenMock.address);

Although I'd rename it to uniswapPair if that's what the contract is called. Then you can remove the below. Do elsewhere if possible too.

willjgriff · 2021-01-04T16:05:34Z

migrations/2_deploy.js

+        await deployer.deploy(UniswapRouterMock);
+
+        await deployer.deploy(UnipoolMock, uniswapToken.address, HoneyTokenMock.address, UniswapRouterMock.address);
+        await deployer.deploy(UnipoolForeignPairMock, uniswapToken2.address, HoneyTokenMock.address, UniswapRouterMock.address);


The unit tests aren't run on Rinkeby. Rinkeby deployment's are only for QA testing and we shouldn't use mocks on Rinkeby if we can avoid it, which I think we can here.

willjgriff · 2021-01-04T16:08:10Z

test/Unipool.js

+
+            await this.unipool.reinvestReward({ from: wallet1 });
+
+            expect(await this.unipool.balanceOf(wallet1)).to.be.bignumber.almostEqualDiv1e18(web3.utils.toWei('2'));


I would copy this expect statement and put it before the reinvestReward call to check the balance is 1 before.

willjgriff · 2021-01-04T16:14:20Z

contracts/Unipool.sol

+        uint256 acceptablePrice = immediatePrice.mul(10000 - MAX_SLIPPAGE_BASIS_POINTS).div(10000);
+        // Preflight here mostly for the benefit of unit tests
+        require(uniswapRouter.getAmountOut(tradeSize, rewardReserve, foreignReserve) >= acceptablePrice, 
+            "Slippage too high for automagic reinvestment");


I don't think we're wizards... I think you can remove the term "automagic" altogether.

anisoptera added 3 commits November 8, 2020 01:02

Add reinvest function to Unipool contract

44faabd

Requires threading the router address through the infra. Existing tests pass. New tests don't exist yet.

add check for HNY/x pair

87723ea

fix up path to use the token, not the pool actually approve the router for our contract

Add unit tests, fix bugs found with them

737af6d

Avoid shavings of token B piling up

willjgriff suggested changes Nov 23, 2020

View reviewed changes

anisoptera added 4 commits November 27, 2020 01:24

Address PR comments, all tests pass

5303791

Add tests, fix bugs

8939ccf

didn't like that .sub(1)

0304b62

whitespace

45a633c

willjgriff reviewed Nov 27, 2020

View reviewed changes

contracts/Unipool.sol Outdated Show resolved Hide resolved

willjgriff reviewed Nov 27, 2020

View reviewed changes

contracts/Unipool.sol Outdated Show resolved Hide resolved

Address PR comments

4b3f04d

anisoptera requested a review from willjgriff November 28, 2020 03:19

Fix bugs found in E2E tests

a0f8201

willjgriff reviewed Dec 1, 2020

View reviewed changes

contracts/Unipool.sol Outdated Show resolved Hide resolved

willjgriff reviewed Dec 1, 2020

View reviewed changes

contracts/Unipool.sol Show resolved Hide resolved

willjgriff suggested changes Dec 1, 2020

View reviewed changes

Address PR feedback

0f560ac

(mostly missed naming issues)

willjgriff reviewed Jan 4, 2021

View reviewed changes

pxbee mentioned this pull request Jun 14, 2021

[NEEDS DESIGN] Add auto compounding to farms 1Hive/honeyswap-farm#3

Open


		await this.unipool.reinvestReward({ from: wallet1 });

		expect(await this.unipool.earned(wallet1)).to.be.bignumber.almostEqualDiv1e18(web3.utils.toWei('0'));


		await this.unipool.reinvestReward({ from: wallet1 });

		expect(await this.unipool.balanceOf(wallet1)).to.be.bignumber.almostEqualDiv1e18(web3.utils.toWei('2'));

Uh oh!

Honey DRIP #6

Are you sure you want to change the base?

Honey DRIP #6

Uh oh!

Conversation

anisoptera commented Nov 23, 2020

Uh oh!

willjgriff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willjgriff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants