Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create altruistic allocations and close stale allocations #549

Merged
merged 13 commits into from
Jul 13, 2022

Conversation

abarmat
Copy link
Contributor

@abarmat abarmat commented Mar 14, 2022

Supersedes: #543

Goal

Provide a mechanism so that indexers can opt-out of rewards by allocating zero tokens to a subgraph. In addition, this implementation allows anyone to close stale allocations after the max allocation period, however, nobody can close an altruistic allocation.

Motivation

  1. Stale allocations affect the stability of the network when an indexer that has a big proportional allocated stake on a subgraph does not keep it active, as other indexer won't want to allocate if it already has a big allocation. Stale allocation also reduce the speed of how value flows through the protocol, reducing the frequency at which curators and delegators get their share of rewards/fees. Currently delegators for an indexer are the only one that can close them, but this is an unnecessary validation considering that anyone could turn into a delegator, even with the smallest amount of tokens, and then close it.

The mechanism for closing state allocations after the maxAllocationEpoch was introduced to avoid indexers holding off rewards forever and never distributing them to delegators. Additionally, it promotes liveness in the protocol state changes that effect other stakeholders decisions.

  1. Indexers might want to index and serve queries on a particular subgraph altruistically by opting out of rewards. Today they can effectively do that by presenting an empty POI on closeAllocation, however this is not the most efficient way to do it. By allowing them allocating zero tokens, we can skip all the code paths for updating rewards getting a huge gas improvement.

Highlight

  • Indexers can allocate zero-tokens to a subgraph to advertise they are indexing and serving queries but opted out of rewards.
  • Even if indexers allocate zero token they are required to have the minimum stake.
  • Altruistic allocations cannot be closed after the maxAllocationEpoch by the public, only by indexer or operator.
  • Anyone can now close a standard allocation after the maxAllocationEpoch.

Gas Efficiency

Altruistic allocations are roughly 50% more gas efficient because the contract doesn't need to recalculate rewards snapshots.

Standard Allocation

|  Staking  ·  allocate         ·  277077  │
|  Staking  ·  closeAllocation  ·  233146  │

Zero Allocation

|  Staking  ·  allocate         ·  144355  │
|  Staking  ·  closeAllocation  ·  129128  │

Reference

Forum: https://forum.thegraph.com/t/rewarded-force-close-mechanism-to-eliminate-stale-allocations/2951/8

@abarmat abarmat changed the title Allocation of zero tokens Altruistic allocations Mar 15, 2022
@abarmat abarmat changed the title Altruistic allocations Create altruistic allocations and close stale allocations Mar 15, 2022
@codecov
Copy link

codecov bot commented May 27, 2022

Codecov Report

Merging #549 (97df72b) into dev (bc90412) will decrease coverage by 0.28%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #549      +/-   ##
==========================================
- Coverage   90.58%   90.29%   -0.29%     
==========================================
  Files          37       35       -2     
  Lines        1795     1752      -43     
  Branches      293      293              
==========================================
- Hits         1626     1582      -44     
- Misses        169      170       +1     
Flag Coverage Δ
unittests 90.29% <100.00%> (-0.29%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
contracts/epochs/EpochManager.sol 86.84% <100.00%> (ø)
contracts/staking/Staking.sol 96.28% <100.00%> (+0.01%) ⬆️
contracts/staking/libs/Rebates.sol 100.00% <100.00%> (ø)
contracts/tests/RebatePoolMock.sol 100.00% <100.00%> (ø)
contracts/rewards/RewardsManager.sol 95.95% <0.00%> (-4.05%) ⬇️
contracts/tests/testnet/GSRManager.sol
contracts/tests/testnet/GDAI.sol

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc90412...97df72b. Read the comment docs.

@abarmat abarmat requested review from tmigone and pcarranzav June 27, 2022 22:03
} else {
// Allocating zero-tokens still needs to have stake
// Minimum indexer stake is managed by stake/unstake
require(stakes[_indexer].tokensSecureStake() > 0, "!stake");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
require(stakes[_indexer].tokensSecureStake() > 0, "!stake");
require(stakes[_indexer].tokensSecureStake() > minimumIndexerStake, "!stake");

Why not use minimumIndexerStake? It's probably a stretch but there is an edge case where having this check rely on stake/unstake could result in the check passing when it shouldn't.

If an indexer stakes minimumIndexerStake and then the minimum is raised by governance then they would still pass this require check but they wouldn't really have the actual minimum amount minimumIndexerStake as it has been increased.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I was looking into the same thing. Only additional change I'd do to that proposal is to use >=

Copy link
Contributor

@tmigone tmigone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a comment and a very tiny nit

Copy link
Member

@pcarranzav pcarranzav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few details for now, but otherwise I think it looks good (still taking a closer look)

contracts/staking/Staking.sol Show resolved Hide resolved
contracts/staking/libs/Rebates.sol Show resolved Hide resolved
test/lib/testHelpers.ts Outdated Show resolved Hide resolved
test/staking/allocation.test.ts Show resolved Hide resolved
tmigone
tmigone previously approved these changes Jul 1, 2022
Copy link
Member

@pcarranzav pcarranzav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@abarmat abarmat merged commit 6bfec1a into dev Jul 13, 2022
@abarmat abarmat deleted the ariel/zero-alloc branch July 13, 2022 14:37
abarmat added a commit that referenced this pull request Jul 18, 2022
- feat: add support to deploy protocol to goerli network (#615)
- chore: add hardhat task to verify all contracts with etherscan (#626)
- chore: add hardhat task to verify all contracts with sourcify (#625)
- chore: add confirmation step to a few more commands
- Create altruistic allocations and close stale allocations (#549)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants