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 Global Mint Reimbursement Request #878

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented Oct 14, 2024

Summary

This PR adds

  • GMRR TLM
  • Ensures that the application has enough funds to cover for it.
  • Deducts the total global mint amount from the application's stake.
  • Sends the global mint amount from the application module account to the tokenomics module account to the PNF account.
  • The application still gets its global inflation share sent to its account.

Issue

Type of change

Select one or more from the following:

  • New feature, functionality or library

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 application Changes related to the Application actor on-chain On-chain business logic tokenomics Token Economics - what else do you need? labels Oct 14, 2024
@red-0ne red-0ne added this to the Shannon Beta TestNet Launch milestone Oct 14, 2024
@red-0ne red-0ne self-assigned this Oct 14, 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

Comment on lines +88 to +92
proofParams := s.keepers.ProofKeeper.GetParams(s.ctx)
proofParams.ProofRequestProbability = 0
proofRequirementThreshold := cosmostypes.NewInt64Coin(volatile.DenomuPOKT, math.MaxInt64)
proofParams.ProofRequirementThreshold = &proofRequirementThreshold
s.keepers.ProofKeeper.SetParams(s.ctx, proofParams)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wdyt about extracting this to a TokenomicsModuleKeeperOpt factory and passing it above, do you think it would get decent reuse?

Perhaps something like WithMaxProofRequirementThreshold().

Copy link
Contributor Author

@red-0ne red-0ne Oct 15, 2024

Choose a reason for hiding this comment

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

Brilliant! 😄

}

// Update the application's on-chain stake
newAppStake, err := application.Stake.SafeSub(newMintCoin)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious to understand if/how this interacts with the application min. stake and/or maxClaimableAmount. 🤔

TL;DR, it seems to me like min. stake SHOULD be collateral for app overservicing AND GMRR. Is this that unnecessary? Do we need to update MaxClaimableAmount?

MaxClaimableAmount prevents actualSettlementCoin from exceeding application stake - min. stake (once we add the unsettled sessions coefficient 😅) but it seems like with GMRR we would also need to also account for the potential magnitude of actualSettlementCoin + newMintCoin. Wouldn't min. stake actually need to cover actualSettlementCoin + newMintCoin for each claim in each session between the current and currently settling sessions?

Since newMintCoin is a function of actualSettlementCoin, it seems like we should be able to consider it in the same scope as maxClaimableAmount if that's the way to go here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application Changes related to the Application actor on-chain On-chain business logic tokenomics Token Economics - what else do you need?
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

3 participants