Set indexer rewards destination #450

abarmat · 2021-02-15T14:22:07Z

Indexer can set a rewards destination for indexing rewards after closing an allocation and for rebate claims
If rewards destination is set to zero, indexing rewards are restaked

pi0neerpat

call setRewardsDestination(address _destination) from your indexer.
close allocation, which will call _distributeRewards and then _sendRewards

Clean and simple, I like it!

pi0neerpat · 2021-02-15T20:05:47Z

contracts/staking/StakingStorage.sol

@@ -82,3 +82,7 @@ contract StakingV1Storage is Managed {
    // Allowed AssetHolders: assetHolder => is allowed
    mapping(address => bool) public assetHolders;
 }
+
+contract StakingV2Storage is StakingV1Storage {


Nice proxy skillz

pi0neerpat · 2021-02-15T20:10:20Z

contracts/staking/Staking.sol

-                require(graphToken.transfer(alloc.indexer, tokensToClaim), "!transfer");
-            }
-        }
+        _sendRewards(graphToken, tokensToClaim, alloc.indexer, restake);


This means the indexer agent has to be updated too right? Since this is only for query fees, I think its ok to not have this feature in the agent for now

So to not change the current claim() interface, if you pass this method restake=true it will restake if restake=false it will send rewards out.

pi0neerpat · 2021-02-15T20:24:56Z

Any tests planned to be added?

abarmat · 2021-02-15T21:08:22Z

Any tests planned to be added?

Yes, I'm pushing them in a minute

davekay100

In general looks good, looking forward to see the tests

davekay100 · 2021-02-15T21:25:53Z

contracts/staking/Staking.sol

+     * @param _destination Rewards destination address. If set to zero, rewards will be restaked
+     */
+    function setRewardsDestination(address _destination) public override {
+        rewardsDestination[msg.sender] = _destination;


Ariel slacked me saying he will add an event here, good idea

Can we make it external? This is what the interface shows, seems we are no using it within the contract

event added :)

I'll change it to be external. An event was added to this function too.

Will _destination be set up via Remix or some key in Agent config?

setRewardsDestination() is a function that needs to be called by the indexer address (the one that staked), in the case of an indexer through a TokenLock it needs be called initially through Remix. The feature can be potentially integrated in the frontend too.

davekay100 · 2021-02-15T21:26:25Z

contracts/staking/StakingStorage.sol

@@ -82,3 +82,7 @@ contract StakingV1Storage is Managed {
    // Allowed AssetHolders: assetHolder => is allowed
    mapping(address => bool) public assetHolders;
 }
+
+contract StakingV2Storage is StakingV1Storage {
+    mapping(address => address) public rewardsDestination;


We could add a comment here to describe what rewardsDestination is

davekay100 · 2021-02-15T21:27:47Z

contracts/staking/Staking.sol

@@ -717,6 +717,14 @@ contract Staking is StakingV1Storage, GraphUpgradeable, IStaking {
        _withdraw(msg.sender);
    }

+    /**
+     * @dev Set the destination where to send rewards.
+     * @param _destination Rewards destination address. If set to zero, rewards will be restaked


great idea that set to 0 = restaked. this way, when the upgrade happens, it will automatically be set so that everyone is re-staking. we won't run into any breaks

davekay100 · 2021-02-15T21:41:50Z

contracts/staking/Staking.sol

+            address destination = rewardsDestination[_beneficiary];
+            require(
+                _graphToken.transfer(
+                    destination == address(0) ? _beneficiary : destination,


I believe this ternary is not needed. If we got this far, we already know that rewardsDestination[_beneficiary] != 0

this would work:

require(_graphToken.transfer(rewardsDestination[beneficiary]), _amount, "!transfer");

wdyt?

It depends, when the function is called from _claim() it might not be the case.

you are right ! there is nothing wrong with this code

davekay100

Tests added look good!

davekay100 · 2021-02-15T22:40:09Z

test/rewards/rewards.test.ts

+        expect(toRound(afterIndexer1Balance)).eq(
+          toRound(beforeIndexer1Balance.add(expectedIndexingRewards)),
+        )
+        // Check indexing rewards are sent from the staking contract


is it more accurate to say check indexing rewards were not sent to the staking contract ? (basically they get minted and send directly to the indexer AFAICT)

Yes, what I tried to say is that rewards are out of the staking contract

davekay100 · 2021-02-15T22:40:21Z

test/rewards/rewards.test.ts

+        const afterStakingBalance = await grt.balanceOf(staking.address)
+
+        // Check that rewards are put into indexer stake
+        // NOTE: calculated manually on a spreadsheet


Note sure what this note really means

It is an outdated comment from the test I used as base for this one, I'll update it

abarmat · 2021-02-16T13:25:44Z

I did some benchmark of gas usage for the solution that send rewards directly and the one that uses a rewards pool:

            |  Contract      ·  Method                 ·  Min        ·  Max        ·  Avg        ·  # calls    ·  usd (avg)   │
RewardPool: |  Staking       ·  closeAllocation        ·          -  ·          -  ·     256898  ·          1  ·           -  │
DirectTx:   |  Staking       ·  closeAllocation        ·          -  ·          -  ·     269114  ·          1  ·           -  │

Sending directly to the beneficiary on close is about 4% more expensive. So it might be a good idea to have a rewards pool where multiple rewards across many closeAllocations() and claim() are accumulated before calling withdrawRewards(), this will be only more efficient in the cases an indexer waits to call withdraw.

@davekaj wdyt?

davekay100

changes look good to me

davekay100

Looks good to me. It would be nice to run some tests before actually merging to master.

which makes me think - what should our strategy be for merging to master?

My first thought is that we should only merge to master when the changes are actually deployed to mainnet. Meaning we could create a dev branch or something right now.

I am open to other suggestions!

pi0neerpat · 2021-02-19T21:20:07Z

Just a suggestion, but you're welcome to copy some of what we do here https://github.com/superfluid-finance/protocol-monorepo

Set up a canary branch ("main" or "dev")
Set this as default branch when people land on the repo, or make any PRs
Every time a PR is merged to master, the new contracts are deployed (upgraded) to testnet
When things are upgraded/deployed to mainnet then we merge to "master" to reflect this as you mentioned @davekaj

Patch versions are updated on releasing to canary
Minor versions are updated on merging to master

…atible with previously deployed contracts

staking: set rewards destination

5c77dfe

pi0neerpat approved these changes Feb 15, 2021

View reviewed changes

staking: emit an event when setting rewards destination + add tests

fed6862

davekay100 suggested changes Feb 15, 2021

View reviewed changes

davekay100 reviewed Feb 15, 2021

View reviewed changes

staking: improve comments and make function external

04747ef

abarmat mentioned this pull request Feb 16, 2021

Set indexer rewards destination using rewards pool #451

Closed

abarmat added 2 commits February 17, 2021 14:18

staking: fix set rewards destination test

7c4de43

staking: refactor test to make it send rewards to new generated address

13ad8f1

abarmat added economic GIP labels Feb 17, 2021

abarmat added 2 commits February 18, 2021 12:54

staking: remove unused comment

f505a23

staking: use auto gas price and limit by default so that tests can run

00b2474

davekay100 approved these changes Feb 18, 2021

View reviewed changes

staking: reorg to keep older implemntation contracts

640e13f

davekay100 approved these changes Feb 19, 2021

View reviewed changes

abarmat changed the title ~~staking: set rewards destination~~ Set indexing rewards destination Feb 21, 2021

abarmat changed the title ~~Set indexing rewards destination~~ Set indexer rewards destination Feb 21, 2021

abarmat added 3 commits February 23, 2021 18:10

upgrades: add a test to verify that new implementation layout is comp…

f2effa2

…atible with previously deployed contracts

staking: move CloseAllocationRequest to StakingData interface

7501405

staking: remove unused pragma

6512d29

abarmat merged commit f1fa480 into master Mar 29, 2021

abarmat deleted the ariel/rewards-pool branch November 10, 2021 06:39

Set indexer rewards destination #450

Set indexer rewards destination #450

Uh oh!

Conversation

abarmat commented Feb 15, 2021

Uh oh!

pi0neerpat 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!

pi0neerpat commented Feb 15, 2021

Uh oh!

abarmat commented Feb 15, 2021

Uh oh!

davekay100 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davekay100 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!

abarmat commented Feb 16, 2021

Uh oh!

davekay100 left a comment

Choose a reason for hiding this comment

Uh oh!

davekay100 left a comment

Choose a reason for hiding this comment

Uh oh!

pi0neerpat commented Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pi0neerpat commented Feb 19, 2021 •

edited

Loading