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

fix: staking payouts change claimed value #1429

Merged
merged 2 commits into from
Apr 21, 2024
Merged

Conversation

Imod7
Copy link
Contributor

@Imod7 Imod7 commented Apr 18, 2024

Issue

In the /accounts/{accountId}/staking-payouts endpoint, for some accounts if we query :

  1. at a specific eraand at a sample block height, e.g. 1400000 we do not get any payouts for the account
  2. but then if we query at the same era but 100k blocks later, e.g. 1500000 we do get payouts for that same account

Example Requests

1. endpoint

http://127.0.0.1:8080/accounts/GeYJhboY5bEc5WZFbrdxhEF9m6Y4NnbKzfCu1rBHxGWgviK/staking-payouts?at=1475279&depth=1&era=518&unclaimedOnly=false

era= 518
block = 1475279

response

{
  "at": {
    "height": "1475279",
    "hash": "0x41798cccf4575dac192c8783925084fa50d8d84a024782f02c3740f857ea47a0"
  },
  "erasPayouts": [
    {
      "era": "518",
      "totalEraRewardPoints": "56520",
      "totalEraPayout": "903617559682178",
      "payouts": []
    }
  ]
}

2. endpoint

http://127.0.0.1:8080/accounts/GeYJhboY5bEc5WZFbrdxhEF9m6Y4NnbKzfCu1rBHxGWgviK/staking-payouts?at=1575279&depth=1&era=518&unclaimedOnly=false

era = 518
block = 1575279 (1475279 + 100k)

response

{
  "at": {
    "height": "1575279",
    "hash": "0xb796181932fe45b7fbfa166005401ec718f4a0979779dea7175362de16c4f177"
  },
  "erasPayouts": [
    {
      "era": "518",
      "totalEraRewardPoints": "56520",
      "totalEraPayout": "903617559682178",
      "payouts": [
        {
          "validatorId": "GeYJhboY5bEc5WZFbrdxhEF9m6Y4NnbKzfCu1rBHxGWgviK",
          "nominatorStakingPayout": "7034530773962",
          "claimed": true,
          "totalValidatorRewardPoints": "440",
          "validatorCommission": "1000000000",
          "totalValidatorExposure": "29800674035283802",
          "nominatorExposure": "500000000000"
        }
      ]
    }
  ]
}

What is happening in general

If I run in debug mode, in both block heights the payouts are correctly calculated but :

  1. in the first case, we just do not give the results back because a check (which is linked to the claimed boolean value) that is in place results to false
  2. in the 2nd case we do give the results back because the same check results to true

What is happening in the code

  1. In this line of code we retrieve per account the totalExposure and nominatorExposure
  2. Then there are a series of checks to define if the rewards were claimed or not
  3. Then the exposure numbers found in step 1 (after the calc_payout) are pushed in the payouts array in this line
  4. And then this array is returned to the user as the field payouts in the response (here)
  5. It is this array (payouts) that is the issue in the above 2 requests
    1. In the 1st case, this array is not returned
    2. In the 2nd case, this array is returned

Why this happens (Root Cause)

  • In the 2nd step of the previous section, if some checks are false, the code will go into the continue case (here, here and here)
  • the continue means that the code will skip everything that is after this statement thus will never arrive at the line where it populates the payouts array and it will not give back any payouts (or more precisely it will give back an empty array).
  • In our cases mentioned above, the check that results in continue is this one where we are checking the lastReward
  • The lastReward is NULL so it will just skip everything after that.

Suggested Solution

With the proposed change in this PR, we basically give back the payouts in both block heights but the claimed variable/field is set to false in the 1st case and true in the 2nd case based on the value of lastReward.

More specifically, this change will result in the following :

  • If lastReward is NULL, we set indexOfEra = -1 instead of skipping/continue
  • which then will set claimed to false
  • then the code will arrive at the line where it populates the payouts array and
  • last it will return this array (payouts) to the user with the only difference that claimed field will be false

Important Note
The payouts in both block heights are the same.

Results from same requests after the change

1. endpoint

http://127.0.0.1:8080/accounts/GeYJhboY5bEc5WZFbrdxhEF9m6Y4NnbKzfCu1rBHxGWgviK/staking-payouts?at=1475279&depth=1&era=518&unclaimedOnly=false

response

{
  "at": {
    "height": "1475279",
    "hash": "0x41798cccf4575dac192c8783925084fa50d8d84a024782f02c3740f857ea47a0"
  },
  "erasPayouts": [
    {
      "era": "518",
      "totalEraRewardPoints": "56520",
      "totalEraPayout": "903617559682178",
      "payouts": [
        {
          "validatorId": "GeYJhboY5bEc5WZFbrdxhEF9m6Y4NnbKzfCu1rBHxGWgviK",
          "nominatorStakingPayout": "7034530773962",
          "claimed": false,
          "totalValidatorRewardPoints": "440",
          "validatorCommission": "1000000000",
          "totalValidatorExposure": "29800674035283802",
          "nominatorExposure": "500000000000"
        }
      ]
    }
  ]
}

2. endpoint

http://127.0.0.1:8080/accounts/GeYJhboY5bEc5WZFbrdxhEF9m6Y4NnbKzfCu1rBHxGWgviK/staking-payouts?at=1575279&depth=1&era=518&unclaimedOnly=false

response

{
  "at": {
    "height": "1575279",
    "hash": "0xb796181932fe45b7fbfa166005401ec718f4a0979779dea7175362de16c4f177"
  },
  "erasPayouts": [
    {
      "era": "518",
      "totalEraRewardPoints": "56520",
      "totalEraPayout": "903617559682178",
      "payouts": [
        {
          "validatorId": "GeYJhboY5bEc5WZFbrdxhEF9m6Y4NnbKzfCu1rBHxGWgviK",
          "nominatorStakingPayout": "7034530773962",
          "claimed": true,
          "totalValidatorRewardPoints": "440",
          "validatorCommission": "1000000000",
          "totalValidatorExposure": "29800674035283802",
          "nominatorExposure": "500000000000"
        }
      ]
    }
  ]
}

Things to confirm

  1. Returning payouts in both block heights is the right and expected behaviour.
  2. If lastReward is NULL, setting claimed to FALSE is the right logic.

Credits & Thanks

  • to @ruiparitydata for bringing this to our attention by providing useful examples and test cases and explaining the issue in detail. This makes the process of reproducing the error and debugging so much easier for us. Thanks again! 💯

@Imod7 Imod7 requested a review from a team as a code owner April 18, 2024 22:59
Copy link
Contributor

@IkerAlus IkerAlus left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation. It looks like a good and simple fix. Great work.

Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

LGTM, just one small nit.

Co-authored-by: Tarik Gul <47201679+TarikGul@users.noreply.github.com>
@Imod7 Imod7 merged commit abf4d9d into master Apr 21, 2024
14 checks passed
@Imod7 Imod7 deleted the domi-payouts-indexofera branch April 21, 2024 20:11
@Imod7 Imod7 mentioned this pull request Apr 24, 2024
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants