feat: Add ClaimAndStake contract #32

nicholaspai · 2022-10-27T17:24:08Z

To make dApp more useful, add new contract ClaimAndStake that can atomically claim on behalf of a user from a MerkleDistributor and stake into the AcceleratingDistributor. The user does not need to sign an allowance, they only need to trust the ClaimAndStake contract to correctly stake on their behalf.

This PR adds a new stakeFor method to the AcceleratingDistributor that enables this claim and stake to work atomically.

To make dApp more useful, add method to claim from external merkle distributor and stake atomically

dohaki

LGTM 👍

contracts/AcceleratingDistributor.sol

test/AcceleratingDistributor.ClaimAndStake.ts

contracts/AcceleratingDistributor.sol

pxrl

I've given this one pass over and don't have any comments. I'll take another look tomorrow, with fresh eyes.

into npai/claim-and-stake

kevinuma · 2022-11-02T13:00:57Z

contracts/AcceleratingDistributor.sol

+
+        UserDeposit storage userDeposit = stakingTokens[stakedToken].stakingBalances[staker];
+
+        uint256 averageDepositTime = getAverageDepositTimePostDeposit(stakedToken, staker, amount);


This is existing code but in getAverageDepositTimePostDeposit, I see

uint256 amountWeightedTime = (((amount * 1e18) / (userDeposit.cumulativeBalance + amount)) *
(getTimeSinceAverageDeposit(stakedToken, account))) / 1e18;

Why not multiple first before dividing by (userDeposit.cumulativeBalance + amount) to minimize rounding errors? Also there are way too many parentheses there.

uint256 amountWeightedTime = (amount * 1e18) * getTimeSinceAverageDeposit(stakedToken, account) / (userDeposit.cumulativeBalance + amount) / 1e18;

These are good points but I don't love modifying existing already-audited code. @chrismaree wdyt?

i dont think we touch this.

@kevinuma In general, we would prefer to modify the code as little as possible. The only exception is if we would classify this as a bug. Is the rounding error substantial enough to consider it a bug?

The rounding error is likely not significant unless a user has staked a significant amount of tokens and then add a very small amount.

contracts/AcceleratingDistributorClaimAndStake.sol

test/AcceleratingDistributor.ClaimAndStake.ts

mrice32 · 2022-11-02T15:16:32Z

contracts/AcceleratingDistributor.sol

+
+        UserDeposit storage userDeposit = stakingTokens[stakedToken].stakingBalances[staker];
+
+        uint256 averageDepositTime = getAverageDepositTimePostDeposit(stakedToken, staker, amount);


@kevinuma In general, we would prefer to modify the code as little as possible. The only exception is if we would classify this as a bug. Is the rounding error substantial enough to consider it a bug?

mrice32 · 2022-11-02T16:23:10Z

contracts/AcceleratingDistributorClaimAndStake.sol

+     * @param claims Claim leaves to retrieve from MerkleDistributor.
+     * @param stakedToken The address of the token to stake.
+     */
+    function claimAndStake(MerkleDistributorInterface.Claim[] memory claims, address stakedToken)


I don't want to throw a wrench into anything, but I was brainstorming a bit and wanted to ask about a slightly different implementation/strategy.

This would introduce less code in this contract, but may add more code (or more complex code) in the merkle distributor and may have a slightly better business outcome (one transaction, no approval):

Add a stakeFor() method in this contract (basically just a wrapper function around _stake() that you have above).

Drop the claimAndStake() method.

Add a claimFor method in the merkle distributor that allows a contract to claim on behalf of a user (and actually receive the tokens). This could be done via signature or whitelist.

Build a special intermediary contract that does the following:

This contract would have a single method to do 2 things:

Claim their airdrop (and pull it into this contract).

Call stakeFor() to stake on behalf of the user.

This process has a few advantages:

Lighter modifications to the AcceleratingDistributor (still some, though), which holds user funds.

Single transaction claim (no approvals).

Downsides:

More cross contract calls.

One more contract to deploy.

I think this is a good idea I'll start work on the AcrossMerkleDistributor

nicholaspai · 2022-11-02T20:11:47Z

contracts/AcceleratingDistributor.sol

@@ -182,26 +182,24 @@ contract AcceleratingDistributor is ReentrancyGuard, Ownable, Multicall {
     * @param amount The amount of the token to stake.
     */
    function stake(address stakedToken, uint256 amount) external nonReentrant onlyEnabled(stakedToken) {
-        _updateReward(stakedToken, msg.sender);


This should be copied exactly to new _stake internal method

mrice32

Looks awesome! Only minor comments

mrice32 · 2022-11-02T22:21:39Z

contracts/ClaimAndStake.sol

+    event SetMerkleDistributor(AcrossMerkleDistributorInterface indexed newMerkleDistributor);
+    event SetAcceleratingDistributor(AcceleratingDistributorInterface indexed newAcceleratingDistributor);
+
+    constructor() {}


Did you want to set the merkleDistributor and acceleratingDistributor contracts as immutable? IMO, this would reduce security surface area and have few (if any) downsides since this contract could always be redeployed with different parameters.

Thoughts?

To be honest the main reason why I didn't do this is because I couldn't figure out how to get the deploy scripts to work with a MerkleDistributor contract address imported from contracts-v2. I'll re review

Ok this was easy since we import contracts-v2

mrice32 · 2022-11-02T22:23:33Z

contracts/ClaimAndStake.sol

+     */
+    function claimAndStake(MerkleDistributorInterface.Claim memory _claim, address stakedToken) external nonReentrant {
+        require(_claim.account == msg.sender, "claim account not caller");
+        require(merkleDistributor.getRewardTokenForWindow(_claim.windowIndex) == stakedToken, "unexpected claim token");


nit: why not drop the stakedToken argument and pull it from the merkleDistributor directly?

Good point, the contract should revert if the claimed token isn't enabled for staking.

mrice32 · 2022-11-02T22:32:56Z

contracts/ClaimAndStake.sol

+        require(_claim.account == msg.sender, "claim account not caller");
+        require(merkleDistributor.getRewardTokenForWindow(_claim.windowIndex) == stakedToken, "unexpected claim token");
+        merkleDistributor.claimFor(_claim);
+        IERC20(stakedToken).safeApprove(address(acceleratingDistributor), _claim.amount);


I wanted to get your thoughts on the use of this function. We usually prefer to not use it, but it might actually be good to use it in this case.

See this check.

I was originally thinking that it would be bad if a leftover approval from a previous call caused the entire contract to begin reverting. However, maybe the opposite is true. A leftover nonzero approval might mean that there is a bug in the contract, so it could be preferable for it to begin reverting in that case.

This probably doesn't matter much, but the alternative would be this function if we wanted to not fail if there was leftover approval. Thoughts?

Oh good point this was an oversight i think its always safer to use safeIncreaseAllowance

mrice32

LGTM! Only minor nits

mrice32 · 2022-11-03T03:22:36Z

contracts/ClaimAndStake.sol

+ * AcceleratingDistributor to spend its staking tokens.
+ */
+
+contract ClaimAndStake is ReentrancyGuard, Ownable, Multicall {


nit: why does this contract need to be ownable?

it no longer does! now that I've removed the setDistributorContracts

mrice32 · 2022-11-03T03:25:49Z

contracts/ClaimAndStake.sol

+interface AcceleratingDistributorInterface {
+    function stakeFor(
+        address beneficiary,
+        address stakedToken,
+        uint256 amount
+    ) external;
+}
+
+interface AcrossMerkleDistributorInterface is MerkleDistributorInterface {
+    function claimFor(MerkleDistributorInterface.Claim memory _claim) external;
+}


nit: I know we do this elsewhere, but is there any reason not to just import these contracts directly? I only suggest it since it minimizes the chance of error (like a typo in the minimal interface that doesn't match the real contract).

For contract size purposes but I guess that's not really a concern since this contract is so small

mrice32 · 2022-11-03T03:28:59Z

contracts/AcceleratingDistributor.sol

+    function stakeFor(
+        address beneficiary,
+        address stakedToken,
+        uint256 amount
+    ) external nonReentrant onlyEnabled(stakedToken) {
+        _stake(stakedToken, amount, beneficiary);


tiny nit: any reason the args are in a different order in stakeFor vs _stake?

Note: no need to change if there is some reason, just a random thing I noticed.

ah good catch ill add benefciary last

mrice32 · 2022-11-03T03:29:25Z

contracts/AcceleratingDistributor.sol

+    /**
+     * @notice Stake tokens for rewards on behalf of `beneficiary`.
+     * @dev The caller of this function must approve this contract to spend amount of stakedToken.
+     * @dev The caller of this function is effectively donating their tokens to the beneficiary. The beneficiary


kevinuma · 2022-11-03T14:42:38Z

contracts/ClaimAndStake.sol

+    function claimAndStake(MerkleDistributorInterface.Claim memory _claim) external nonReentrant {
+        require(_claim.account == msg.sender, "claim account not caller");
+        address stakedToken = merkleDistributor.getRewardTokenForWindow(_claim.windowIndex);
+        merkleDistributor.claimFor(_claim);


Should we assert that this contract has received the right claim amount for additional safety?

i don't think we need to since we can assume that claimFor works as expected and because the merkleDistributor address is a trusted one (and set upon deployment). If we assume claimFor works

kevinuma · 2022-11-03T14:46:17Z

contracts/ClaimAndStake.sol

+    function claimAndStake(MerkleDistributorInterface.Claim memory _claim) external nonReentrant {
+        require(_claim.account == msg.sender, "claim account not caller");
+        address stakedToken = merkleDistributor.getRewardTokenForWindow(_claim.windowIndex);
+        merkleDistributor.claimFor(_claim);


To call claimFor, this contract has to be whitelisted in the merkle distributor contract: https://github.com/across-protocol/contracts-v2/blob/master/contracts/merkle-distributor/AcrossMerkleDistributor.sol#L41. I don't see this in the deployment step. Is this something we'll do separately?

yep! Usually we use the deployment scripts jsut to deploy contracts, and all the setup is done afterwards either manually or via scripts

I was thinking it could be a step in the same deployment script as ClaimAndStake if the deployer still has ownership if merkle distributor. It's less manual and ensures that we don't need to make sure the addresses are right later.

kevinuma · 2022-11-03T14:47:11Z

contracts/ClaimAndStake.sol

+     * @dev Will revert if this contract is not a "whitelisted claimer" on the MerkleDistributor contract.
+     * @param _claim Claim leaf to retrieve from MerkleDistributor.
+     */
+    function claimAndStake(MerkleDistributorInterface.Claim memory _claim) external nonReentrant {


I wonder if we can add a multi claim function as well since merkle distributor supports claimMulti anyway

This contract is Multicaller so we can just multicall it. I think this is preferable to implementing claimMultiFor which would require re-writing most of the logic in claimMulti to be rewritten. This is because the claimMulti code doesn't simply call claim()

Alright sounds good

mrice32

LGTM!

nicholaspai · 2022-11-16T17:19:54Z

contracts/ClaimAndStake.sol

+     * @dev Will revert if this contract is not a "whitelisted claimer" on the MerkleDistributor contract.
+     * @param _claim Claim leaf to retrieve from MerkleDistributor.
+     */
+    function claimAndStake(MerkleDistributorInterface.Claim memory _claim) external nonReentrant {


New claimAndStake function that atomically calls claimFor on AcrossMerkleDistributor and then calls stakeFor for original claimer.

nicholaspai added 2 commits October 27, 2022 13:23

feat: Add claimAndStake method

834502c

To make dApp more useful, add method to claim from external merkle distributor and stake atomically

store merkle distributor in state

ed5e42a

nicholaspai requested a review from mrice32 October 28, 2022 18:18

use contracts 1.0.5

2394332

nicholaspai requested review from dohaki and kevinuma October 28, 2022 20:47

nicholaspai marked this pull request as ready for review October 28, 2022 20:47

nicholaspai added 3 commits October 28, 2022 16:52

Fix

13c87c5

Rename

23e9451

Update AcceleratingDistributor.ClaimAndStake.ts

0dbd45b

dohaki approved these changes Oct 31, 2022

View reviewed changes

nicholaspai mentioned this pull request Oct 31, 2022

feat(merkle-distributor): Make claim only callable by recipient or whitelisted claimer across-protocol/contracts-v3#195

Merged

nicholaspai commented Oct 31, 2022

View reviewed changes

contracts/AcceleratingDistributor.sol Outdated Show resolved Hide resolved

nicholaspai commented Oct 31, 2022

View reviewed changes

test/AcceleratingDistributor.ClaimAndStake.ts Outdated Show resolved Hide resolved

nicholaspai marked this pull request as draft October 31, 2022 21:18

chrismaree reviewed Nov 1, 2022

View reviewed changes

contracts/AcceleratingDistributor.sol Outdated Show resolved Hide resolved

pxrl reviewed Nov 1, 2022

View reviewed changes

nicholaspai added 4 commits November 1, 2022 16:44

Add ADClaimAndStake

7af3054

Update AcceleratingDistributor.ClaimAndStake.ts

1af5cb6

Update AcceleratingDistributorClaimAndStake.sol

8ffcf6f

Merge branch 'master' of https://github.com/across-protocol/across-token

90f982f

into npai/claim-and-stake

nicholaspai marked this pull request as ready for review November 1, 2022 20:49

kevinuma reviewed Nov 2, 2022

View reviewed changes

kevinuma approved these changes Nov 2, 2022

View reviewed changes

contracts/AcceleratingDistributorClaimAndStake.sol Outdated Show resolved Hide resolved

test/AcceleratingDistributor.ClaimAndStake.ts Outdated Show resolved Hide resolved

nicholaspai requested a review from kevinuma November 2, 2022 13:49

mrice32 reviewed Nov 2, 2022

View reviewed changes

Refactor with ClaimAndStake

0fa124e

nicholaspai changed the title ~~feat: Add claimAndStake method~~ feat: Add ClaimAndStake contract Nov 2, 2022

Update AcceleratingDistributor.sol

c630a94

nicholaspai commented Nov 2, 2022

View reviewed changes

nicholaspai requested a review from mrice32 November 2, 2022 20:19

mrice32 reviewed Nov 2, 2022

View reviewed changes

matts comments

c6816bc

nicholaspai requested a review from mrice32 November 3, 2022 02:50

mrice32 approved these changes Nov 3, 2022

View reviewed changes

nicholaspai added 2 commits November 3, 2022 08:40

Update ClaimAndStake.sol

8c193e2

Reverse param order for stakeFor

fcd29da

nicholaspai requested a review from mrice32 November 3, 2022 12:43

nicholaspai added 3 commits November 3, 2022 08:44

Update yarn.lock

030b6c6

fix

5f0661f

fix

fe02e96

kevinuma reviewed Nov 3, 2022

View reviewed changes

mrice32 approved these changes Nov 3, 2022

View reviewed changes

nicholaspai merged commit dde7aed into master Nov 3, 2022

mrice32 deleted the npai/claim-and-stake branch November 3, 2022 16:57

nicholaspai commented Nov 16, 2022

View reviewed changes


		UserDeposit storage userDeposit = stakingTokens[stakedToken].stakingBalances[staker];

		uint256 averageDepositTime = getAverageDepositTimePostDeposit(stakedToken, staker, amount);

feat: Add ClaimAndStake contract #32

feat: Add ClaimAndStake contract #32

Conversation

nicholaspai commented Oct 27, 2022 • edited

dohaki left a comment

Choose a reason for hiding this comment

pxrl left a comment

Choose a reason for hiding this comment

kevinuma Nov 2, 2022 • 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

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

mrice32 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

Choose a reason for hiding this comment

mrice32 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

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

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicholaspai commented Oct 27, 2022 •

edited

kevinuma Nov 2, 2022 •

edited