Simple liquidity rewards #645

mhluongo · 2020-12-14T15:03:25Z

Parameterized Synthetix's Unipool implementation for more flexible reward delivery.

@dimpar I'll let you take the PR from here — I haven't tried to build or thought about deploys, but the change to Unipool is simple. I'm also confident in the original source that I pulled in from mainnet.

Initially we need LPRewards deployed for these pairs

TBTC/ETH - https://info.uniswap.org/pair/0x854056fd40c1b52037166285b2e54fee774d33f6
KEEP/ETH - https://info.uniswap.org/pair/0xe6f19dab7d43317344282f803f8e8d240708174a
KEEP/TBTC - https://info.uniswap.org/pair/0x38c8ffee49f286f25d25bad919ff7552e5daf081

Not interested in doing the OpenZeppelin and Solidity versioning dances. If we depart further from the contract than planned, we can integrate it properly into the rest of the repo

The hard-coded token address pattern is getting old.

Should be consturctor -> external -> public

nkuba

Leaving a first part of comments. I hope we will add more tests to cover all the functions in the contracts.

solidity/contracts/LPRewards.sol

solidity/test/LPRewardsTest.js

dimpar · 2020-12-16T16:05:28Z

Leaving a first part of comments. I hope we will add more tests to cover all the functions in the contracts.

Yes, I was planning on adding some more tests for our purposes to mainly show the different flows, but please note that Unipool.sol was already tested here. I take that this is a trusted source. These contracts are heavily based on Unipool.sol with some minor changes.

mhluongo · 2020-12-16T22:56:53Z

Indeed, this has been audited and in highly-visible production for the past 8 months. We just need confidence on the diff

dimpar · 2020-12-17T07:52:34Z

I compared the imported libs to make sure we're not breaking anything under the hood by using OpenZeppelin v2.3.0 instead of Unipool's OpenZeppeling v2.4.0. Here is OZ Changelog for v2.4.0. I don't see anything in "Breaking changes" that can affect us and it looks like we can use imports from v2.3.0.

I compared the initial commit df98c9b with OZ v2.3.0 and here's my findings:

Math.sol - same
SafeMath.sol - updated to throw custom errors on subtraction overflows.
Context.sol - instead of using _msgSender() which returns msg.sender now we just return msg.sender
Ownable.sol - same, it used '_msgSender()' from Context.sol which just returned msg.sender
IERC20.sol - same, changes were only in the comments
Address.sol - is used by SafeERC20. We are interested in one function Address.isContract(). This function changed in v2.4.0 to lower the gas consumption, HOWEVER later they figured out that it is not cheaper and switched the code back. Master has exactly the same code as v2.3.0 (which what we currently use). I think we are good on this front.
SafeERC20.sol - minor change in error message when subtracting.

In contracts we use "wrapped" token but in tests the same token is named "lpToken". To reference the same token, this commit changes "lp" to "wrapped" for ERC-20 pair token.

solidity/contracts/LPRewards.sol

solidity/test/LPRewardsTest.js

nkuba · 2020-12-17T16:13:28Z

solidity/test/LPRewardsTest.js

+  })
+
+  describe("tokens allocation", () => {
+    it("should successfully allocate KEEP tokens", async () => {


Suggested change

it("should successfully allocate KEEP tokens", async () => {

it("should successfully allocate KEEP tokens via receiveApproval function", async () => {

I am not sure if it's an allocation we are testing here. receiveApproval is just transferring tokens. The real allocation is receiveApproval + notifyRewardsAmount. Thoughts on renaming test scenario?

Or, maybe even better, we can add notifyRewardAmount call to this scenario. I do not think there is a scenario where we call receiveApproval without calling notifyRewardsAmount.

Nope, heh. receiveApproval needs to call notifyRewardsAmount because PhasedEscrow's beneficiary will be the reward allocation contract.

b148edc Now we transfer funds in notifyRewardsAmount() and receiveApproval is gone

nkuba · 2020-12-17T16:19:58Z

solidity/test/LPRewardsTest.js

+    keepToken = await KeepToken.new()
+    // This is a "Pair" Uniswap Token which is created here:
+    // https://github.com/Uniswap/uniswap-v2-core/blob/master/contracts/UniswapV2Factory.sol#L23
+    //
+    // There are 3 addresses for the following pairs:
+    // - KEEP/ETH (https://info.uniswap.org/pair/0xe6f19dab7d43317344282f803f8e8d240708174a)
+    // - TBTC/ETH (https://info.uniswap.org/pair/0x854056fd40c1b52037166285b2e54fee774d33f6)
+    // - KEEP/TBTC (https://info.uniswap.org/pair/0x38c8ffee49f286f25d25bad919ff7552e5daf081)
+    wrappedToken = await WrappedToken.new()
+    lpRewards = await LPRewards.new(keepToken.address, wrappedToken.address)


Can we add new variables keepTokenContractOwner, wrappedTokenContractOwner and lpRewardsContractOwner that would be different account than default 0 and deploy contracts from these accounts?

solidity/contracts/LPRewards.sol

nkuba · 2020-12-17T16:41:31Z

solidity/test/LPRewardsTest.js

+      expect(keepBalance).to.eq.BN(rewards)
+    })
+
+    it("should successfully allocate wrapped tokens", async () => {


Can we organize test cases per function? e.g.LPTokens -> stake -> should successfully allocate wrapped tokens?

Shouldn't this test be named ... stake wrapped tokens? We are testing stake function here.

Piotr: dc0f47c

solidity/contracts/LPRewards.sol

KEEP token is the only token that we reward people for participating in the keep network.

nkuba · 2020-12-18T08:52:57Z

solidity/test/LPRewardsTest.js

+      const wrappedTokenWalletBalance = await wrappedToken.balanceOf(wallet1)
+      expect(wrappedTokenWalletBalance).to.eq.BN(wrappedTokenWalletBallance)
+    })
+  })


We also need a test for getReward function.

exit() function above in this test calls getReward() and withdraw(). Then we check if a user actually received the rewards by calling getReward()

solidity/test/LPRewardsTest.js

…ontract naming

solidity/test/LPRewardsTest.js

pdyraga · 2020-12-18T12:17:30Z

solidity/test/LPRewardsTest.js

+      expect(keepBalance).to.eq.BN(rewards)
+    })
+
+    it("should successfully allocate wrapped tokens", async () => {


Shouldn't this test be named ... stake wrapped tokens? We are testing stake function here.

solidity/test/LPRewardsTest.js

nkuba · 2020-12-18T11:57:32Z

solidity/test/LPRewardsTest.js

+        from: staker2,
+      })
+
+      wrappedTokenBalance = await wrappedToken.balanceOf(lpRewards.address)


Let's also check the state of lpRewards:

lpRewards.totalSupply()

lpRewards.balanceOf(staker1)

lpRewards.balanceOf(staker2)

nkuba · 2020-12-18T11:59:25Z

solidity/test/LPRewardsTest.js

+      const future = (await time.latest()).add(time.duration.days(7))
+      await timeIncreaseTo(future)


Could we use periodFinish for that?

Suggested change

const future = (await time.latest()).add(time.duration.days(7))

await timeIncreaseTo(future)

await timeIncreaseTo(await lpRewards.periodFinish.call())

Not sure about that. Having a separate duration of 7 days in test also checks duration in the contract.

nkuba · 2020-12-18T12:02:05Z

solidity/test/LPRewardsTest.js

+      const future = (await time.latest()).add(time.duration.days(7))
+      await timeIncreaseTo(future)
+
+      const actualEarnings = (await lpRewards.earned(staker1)).div(


I'm not a fan of doing .div(tokenDecimalMultiplier) and .mul(tokenDecimalMultiplier) in so many places. Could we define values at the begining on the test including tokenDecimalMultiplier and don't use it aferwards?

I know it doesn't look pretty, but I think there is a value when testing with tokenDecimalMultiplier which reflects the scenario when working with real tokens. When transferring tokens to a contract and then retrieving them, we need to divide by 10**18 which will also show a precision error that we're dealing with. This is a similar approach in other tests for rewards, ex. TestECDSARewards.js.
How about instead of multiplication we would use web3.utils.toWei() function 4e8d014?

nkuba · 2020-12-18T12:45:36Z

solidity/test/LPRewardsTest.js

+      expect(actualRewardPerToken).to.gte.BN(
+        expectedRewardPerWrappedToken.subn(precision)
+      )
+      expect(actualRewardPerToken).to.lte.BN(
+        expectedRewardPerWrappedToken.addn(precision)
+      )


Woud could try to calculate the exact amount like in the example below. But also we can hardcode the expected value.

const duration = await lpRewards.DURATION.call() const rewardRate = keepRewards.div(duration) const expectedRewardPerWrappedToken = rewardRate .mul(duration) .mul(tokenDecimalMultiplier) .div(stakedAmount)

Hardcoding can be the cleaniest solution. Of course we need to add a comment next to the value definition with steps how it was calculated.

I've added a comment showing the math behind a reward per token. b0165bd

I think checking math on contract with a simple expected value in test should work, especially this is considered to be a trusted source.

nkuba · 2020-12-18T12:47:35Z

solidity/test/LPRewardsTest.js

+      expect(keepEarnedRewards).to.gte.BN(rewardsAmount.subn(precision))
+      expect(keepEarnedRewards).to.lte.BN(rewardsAmount.addn(precision))


I think we can hardcode expectedEarnedRewards so we can have just one exact equality check.

How would that help? It's a time precision issue to my understanding.

Precision error is very small, we had the same issue with TestECDSARewards.js and handled it the same way there. Besides, looks like Unipool.js also tests values from Solidy in a similar fashion by using almost equal approach.

…enario

We need 3 dedicated LPRewards contracts- each for Uniswap pair KEEP/ETH, TBTC/ETH, KEEP/TBTC. We are going to store a separate artifact for each Uniswap pair, so we should replace paths to artifacts in a followup work. Ref: keep-network/keep-ecdsa#645

Instead of adding approveAndCall function, it is cleaner to add a 'transfer' function in notifyRewardAmount which has to be called anyway by the reward distributor. Reward distributor in our case is escrow's beneficiary which is going to be set by PhasedEscrow contract. In tests, the role of PhasedEscrow beneficiary takes rewardDistribution account. We need to transfer funds from KEEP token owner to rewardDistribution address first, before we can call notifyRewardAmount().

pdyraga · 2020-12-21T11:59:50Z

@dimpar Can you update this branch with master please?

pdyraga

Five comments to tests, I consider neither of them blocking. Need some time to go through the contract code one more time. @dimpar if you can address them in the meantime - perfect, if not, I do not want to block here on them.

solidity/test/LPRewardsTest.js

This reverts commit 4e8d014.

- removed a delay before increasing time on chain - fixing typo - removed initial balance check

BatchedPhasedEscrow BatchedPhasedEsrow` is a token holder contract allowing contract owner to approve a set of beneficiaries of tokens held by the contract, to appoint a separate drawee role, and allowing that drawee to withdraw tokens to approved beneficiaries in phases. This contract is not the ultimate solution for a secure escrow with a separate role withdrawing tokens. Notably, it does not limit the amount or frequency of withdrawals so compromised drawee can drain the escrow funds to one of the selected approved beneficiaries. The escrow owner should proceed with caution and do not transfer all funds to `BatchedPhasedEscrow`. Instead, funds should be transferred in tranches, depending on how drawee spends them. `CurveRewards` and `LPTokenRewards` contracts from `keep-ecdsa` that we are are going to deploy for Uniswap pairs (see keep-network/keep-ecdsa#645) have the same `notifyRewardAmount` interface. To do not duplicate the code, I abstracted `CurveRewardsEscrowBeneficiary` contract into a generic `StakingPoolRewardsEscrowBeneficiary` contract.

mhluongo and others added 12 commits December 14, 2020 09:46

Pull the Synthetix Unipool contract from mainnet

df98c9b

Not interested in doing the OpenZeppelin and Solidity versioning dances. If we depart further from the contract than planned, we can integrate it properly into the rest of the repo

Paramaterize the Synethetix Unipool for LP rewards

16350f4

The hard-coded token address pattern is getting old.

Importing OpenZeppelin Math.sol

e646258

Importing OpenZeppelin SafeMath.sol

2bc0029

Importing OpenZeppelin Ownable.sol

268dd19

Remove unused Context contract

0b00d43

Importing OpenZeppelin SafeERC20.sol

cb6048f

Changing the function order

015d450

Should be consturctor -> external -> public

Linting only

f47a37f

Cleanup comments

82327a0

Adding receiveApproval() for KEEP tokens

08bb479

Adding basic tests for token allocations

9af85f7

nkuba reviewed Dec 16, 2020

View reviewed changes

Aligning name for a Uni pair token

2f93ad3

In contracts we use "wrapped" token but in tests the same token is named "lpToken". To reference the same token, this commit changes "lp" to "wrapped" for ERC-20 pair token.

pdyraga reviewed Dec 17, 2020

View reviewed changes

solidity/contracts/LPRewards.sol Outdated Show resolved Hide resolved

solidity/contracts/LPRewards.sol Show resolved Hide resolved

solidity/test/LPRewardsTest.js Show resolved Hide resolved

pdyraga added the ⛓chain label Dec 17, 2020

Adding testing flow for earning the rewards and claiming them

9696385

nkuba reviewed Dec 17, 2020

View reviewed changes

nkuba reviewed Dec 18, 2020

View reviewed changes

solidity/contracts/LPRewards.sol Outdated Show resolved Hide resolved

dimpar added 2 commits December 18, 2020 09:33

Renaming 'wrapped' token to 'wrappedToken'

31c5939

Renaming rewardToken to keepToken

85819dc

KEEP token is the only token that we reward people for participating in the keep network.

nkuba reviewed Dec 18, 2020

View reviewed changes

dimpar added 4 commits December 18, 2020 11:22

Adding a comment regarding the original source of contracts

70e7328

Combining constants

1d96dfc

Renaming user's 'wallet' to 'staker' which indicate his address

f49f7c8

Owner account[0] -> rewardDistribution account[5] to align with the c…

044bcab

…ontract naming

pdyraga reviewed Dec 18, 2020

View reviewed changes

solidity/test/LPRewardsTest.js Outdated Show resolved Hide resolved

nkuba reviewed Dec 18, 2020

View reviewed changes

Deploying contracts from dedicated accounts to test more realistic sc…

0e2e07c

…enario

nkuba mentioned this pull request Dec 18, 2020

Dedicated LPRewards contracts for three pairs #650

Merged

dimpar added 6 commits December 18, 2020 16:43

Changing test description 'allocate' -> 'stake'

dc0f47c

Removing unnecessary assertions

09d89f3

Checking additional states in LP rewards contract

7beae72

Using web3 conversion to wei instead of multiplying by 10^18

4e8d014

Refactoring mintAndApproveWrappedTokens for reusability

d00548e

dimpar added 2 commits December 21, 2020 13:11

Merge remote-tracking branch 'origin' into liquidity-rewards

b0b2079

Explaining calculation of reward per token

b0165bd

pdyraga reviewed Dec 21, 2020

View reviewed changes

dimpar added 8 commits December 21, 2020 13:44

Revert "Using web3 conversion to wei instead of multiplying by 10^18"

8f1fdac

This reverts commit 4e8d014.

Cleaning up

264f253

- removed a delay before increasing time on chain - fixing typo - removed initial balance check

Removing accidental code

fe46b3a

Linting only

d1853ea

Removing typos ballance -> balance

17048ba

Fixing assignment to const value

cca746d

Removing unnecessary approval before KEEP transfer

91b3bf3

Linting only

c9b317b

pdyraga approved these changes Dec 21, 2020

View reviewed changes

pdyraga merged commit 1601dce into master Dec 21, 2020

pdyraga deleted the liquidity-rewards branch December 21, 2020 13:47

pdyraga mentioned this pull request Dec 21, 2020

BatchedPhasedEscrow keep-network/keep-core#2230

Merged

pdyraga added this to the solidity/v1.5.0 milestone Dec 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple liquidity rewards #645

Simple liquidity rewards #645

mhluongo commented Dec 14, 2020 •

edited by dimpar

Loading

nkuba left a comment

dimpar commented Dec 16, 2020 •

edited

Loading

mhluongo commented Dec 16, 2020

dimpar commented Dec 17, 2020 •

edited

Loading

nkuba Dec 17, 2020

pdyraga Dec 18, 2020

pdyraga Dec 18, 2020

pdyraga Dec 18, 2020

dimpar Dec 18, 2020

nkuba Dec 17, 2020

dimpar Dec 18, 2020

nkuba Dec 17, 2020

pdyraga Dec 18, 2020

dimpar Dec 21, 2020 •

edited

Loading

nkuba Dec 18, 2020

dimpar Dec 21, 2020

pdyraga Dec 18, 2020

nkuba Dec 18, 2020

dimpar Dec 21, 2020

nkuba Dec 18, 2020

dimpar Dec 21, 2020 •

edited

Loading

nkuba Dec 18, 2020

dimpar Dec 21, 2020

nkuba Dec 18, 2020

nkuba Dec 18, 2020

dimpar Dec 21, 2020

nkuba Dec 18, 2020

pdyraga Dec 18, 2020

dimpar Dec 21, 2020

pdyraga commented Dec 21, 2020

pdyraga left a comment

	it("should successfully allocate KEEP tokens", async () => {
	it("should successfully allocate KEEP tokens via receiveApproval function", async () => {

		const future = (await time.latest()).add(time.duration.days(7))
		await timeIncreaseTo(future)

	const future = (await time.latest()).add(time.duration.days(7))
	await timeIncreaseTo(future)
	await timeIncreaseTo(await lpRewards.periodFinish.call())

		expect(keepEarnedRewards).to.gte.BN(rewardsAmount.subn(precision))
		expect(keepEarnedRewards).to.lte.BN(rewardsAmount.addn(precision))

Simple liquidity rewards #645

Simple liquidity rewards #645

Conversation

mhluongo commented Dec 14, 2020 • edited by dimpar Loading

nkuba left a comment

Choose a reason for hiding this comment

dimpar commented Dec 16, 2020 • edited Loading

mhluongo commented Dec 16, 2020

dimpar commented Dec 17, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimpar Dec 21, 2020 • edited Loading

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

dimpar Dec 21, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdyraga commented Dec 21, 2020

pdyraga left a comment

Choose a reason for hiding this comment

mhluongo commented Dec 14, 2020 •

edited by dimpar

Loading

dimpar commented Dec 16, 2020 •

edited

Loading

dimpar commented Dec 17, 2020 •

edited

Loading

dimpar Dec 21, 2020 •

edited

Loading

dimpar Dec 21, 2020 •

edited

Loading