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

[Tokenomics] Implement difficulty proportional rewards #840

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented Sep 26, 2024

Summary

This PR incorporated the mining difficulty into the claim reward calculation.

  • Encapsulates the claimed tokens into a claim.GetClaimeduPOKT(params, difficulty)
  • Implements a test to assert that the difficulty based rewards are proportional to the off-chain relays served.
  • Removes unused min_relay_difficulty_bits gov. param.

It is based on the preparation work of PR #836.

Issue

Type of change

Select one or more from the following:

Testing

  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code
  • I create and reference any new tickets, if applicable
  • I have left TODOs throughout the codebase, if applicable

@red-0ne red-0ne added the tokenomics Token Economics - what else do you need? label Sep 26, 2024
@red-0ne red-0ne added this to the Shannon Beta TestNet Launch milestone Sep 26, 2024
@red-0ne red-0ne self-assigned this Sep 26, 2024
Copy link
Contributor

@bryanchriswhite bryanchriswhite left a comment

Choose a reason for hiding this comment

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

image

Reviewed everything except for keeper_settle_pending_claims_test.go. Looking really clean! 👌 😎 🧼

pkg/crypto/rings/client.go Show resolved Hide resolved
x/proof/types/claim.go Outdated Show resolved Hide resolved
x/service/keeper/update_relay_mining_difficulty.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/token_logic_modules_test.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/token_logic_modules.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/settle_pending_claims.go Outdated Show resolved Hide resolved
## Summary

This PR prepares for claimed amount calculation to use the
`RelayDifficultyMultiplier`.
* It removes the `RelayDifficultyTargetHash` `proof` param.
* Replace `prooftypes.DefaultRelayDifficultyTargetHash` occurrences with
`protocol.BaseRelayDifficultyHashBz`.
* Moves the `RleayMiningDifficulty` logic to the `service` module due to
cyclic dependencies.
* Rename `num_compute_units` to `num_claimed_compute_units` and add
`num_estimated_compute_units` and `claimed_amount_upokt`
* Remove the no longer used tokenomics querier to retrieve a service
difficulty target hash.

The PR is only renaming and moving of files most (~13200LOC) changes are
auto-generated code.

## Type of change

Select one or more from the following:

- [ ] New feature, functionality or library
- [x] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [x] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [x] **Unit Tests**: `make go_develop_and_test`
- [x] **LocalNet E2E Tests**: `make test_e2e`
- [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [x] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [ ] I have left TODOs throughout the codebase, if applicable
Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks.

You may need to run make trigger_ci to submit an empty commit that'll trigger the tests.

GCP workloads (requires changing the namespace to 840)
Grafana network dashboard for devnet-issue-840

@github-actions github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Oct 10, 2024
Copy link
Contributor

@bryanchriswhite bryanchriswhite left a comment

Choose a reason for hiding this comment

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

Preemptively approving - just a few suggestions, otherwise LGTM! 🙌

Please do merge main before merging this.

🚢

tests/integration/service/relay_mining_difficulty_test.go Outdated Show resolved Hide resolved

// Prepare the test supplier.
supplierAddress := sample.AccAddress()
supplierStake := sdk.NewInt64Coin(volatile.DenomuPOKT, 1000)
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
supplierStake := sdk.NewInt64Coin(volatile.DenomuPOKT, 1000)
supplierStake := suppliertypes.DefaultMinStake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Supplier min stake is not merged yet

x/tokenomics/keeper/keeper_settle_pending_claims_test.go Outdated Show resolved Hide resolved
x/tokenomics/keeper/keeper_settle_pending_claims_test.go Outdated Show resolved Hide resolved
require.NoError(t, err)

// +1 to push threshold above s.claim's compute units
proofRequirementThreshold, err := tokenomics.NumComputeUnitsToCoin(sharedParams, numClaimComputeUnits+1)
require.NoError(t, err)
proofRequirementThreshold = proofRequirementThreshold.Add(uPOKTCoin(1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Loving this pattern ❤️; nice and 🧼! 😎

x/tokenomics/keeper/settle_pending_claims.go Outdated Show resolved Hide resolved
Comment on lines +514 to +515
// The current test suite sets the CUPR to 1, making sum == count * CUPR
// valid. So, we can change the last byte to 'b' to make it invalid.
Copy link
Contributor

Choose a reason for hiding this comment

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

👌 Thank you!

bryanchriswhite and others added 5 commits October 11, 2024 17:26
## Summary

Updates `ProcessTokenLogicModules()` logic to unbond applications whose
stake is below the minimum **after** processing all TLMs.

## Dependencies

- #809
- #843 
- #844 
- #845
- #847

## Dependents
 
- #849
- #850
- #857
- #852 
- #861 
- #851 
- #863 

## Issue

- #612

## Type of change

Select one or more from the following:

- [x] New feature, functionality or library
- [ ] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [ ] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [ ] **Documentation**: `make docusaurus_start`; only needed if you
make doc changes
- [x] **Unit Tests**: `make go_develop_and_test`
- [ ] **LocalNet E2E Tests**: `make test_e2e`
- [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [x] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [x] I have left TODOs throughout the codebase, if applicable

---------

Co-authored-by: Redouane Lakrache <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: red-0ne <[email protected]>
@red-0ne red-0ne merged commit aab06af into feat/prepare-difficulty-proportional-rewards Oct 11, 2024
4 checks passed
red-0ne added a commit that referenced this pull request Oct 14, 2024
**This is a repost of PR #840 which did not get merged into main**

## Summary

This PR incorporated the mining difficulty into the claim reward
calculation.
* Encapsulates the claimed tokens into a `claim.GetClaimeduPOKT(params,
difficulty)`
* Implements a test to assert that the difficulty based rewards are
proportional to the off-chain relays served.
* Removes unused `min_relay_difficulty_bits` gov. param.

It is based on the preparation work of PR #836.

## Issue

- #781 
- #758 

## Type of change

Select one or more from the following:

- [x] New feature, functionality or library
- [ ] Consensus breaking; add the `consensus-breaking` label if so. See
#791 for details
- [ ] Bug fix
- [ ] Code health or cleanup
- [ ] Documentation
- [ ] Other (specify)

## Testing

- [x] **Unit Tests**: `make go_develop_and_test`
- [x] **LocalNet E2E Tests**: `make test_e2e`
- [ ] **DevNet E2E Tests**: Add the `devnet-test-e2e` label to the PR.

## Sanity Checklist

- [x] I have tested my changes using the available tooling
- [x] I have commented my code
- [x] I have performed a self-review of my own code; both comments &
source code
- [ ] I create and reference any new tickets, if applicable
- [x] I have left TODOs throughout the codebase, if applicable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devnet devnet-test-e2e push-image CI related - pushes images to ghcr.io tokenomics Token Economics - what else do you need?
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

2 participants