[Tokenomics] Harden settlement (part 1: TLM isolation) #889

bryanchriswhite · 2024-10-22T13:35:09Z

Summary

Refactors claim settlement and token logic module processing:

Isolates TLM processors from the tokenomics keeper
Restructures settlement and TLM processing to avoid non-determinism
Consolidates and postpones ALL state modification until the end of settlement

Issue

[Tokenomics] Settlement safety #881

Type of change

Select one or more from the following:

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

Testing

Documentation: make docusaurus_start; only needed if you make doc changes
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

github-actions · 2024-10-22T13:35:20Z

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 889)
Grafana network dashboard for devnet-issue-889

github-actions

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

linter-name (fail-on-found)

x/tokenomics/keeper/settle_pending_claims.go

x/tokenomics/token_logic_module/result.go

testutil/keeper/tokenomics.go

testutil/integration/options.go

tests/integration/tokenomics/token_logic_modules_test.go

x/tokenomics/token_logic_module/result.go

testutil/sample/sample.go

tests/integration/tokenomics/token_logic_modules_test.go

x/tokenomics/token_logic_module/distribution.go

x/tokenomics/token_logic_module/global_mint.go

- Isolate TLM processors from the tokenomics keeper - Restructure TLM processors to avoid non-deterministic execution - Consolidate and postpone ALL state modification until the end of settlement

…able

…ocessors

tests/integration/tokenomics/token_logic_modules/commutative_test.go

tests/integration/tokenomics/token_logic_modules/tlm_processor_suite_test.go

x/tokenomics/module/abci.go

x/tokenomics/token_logic_module/global_mint.go

…nLogicModule to TokenLogicModuleId

bryanchriswhite · 2024-10-25T12:17:50Z

@red-0ne, @Olshansk, I still need to fix a few tests but otherwise I think this is ready for initial review.

bryanchriswhite · 2024-10-25T14:47:07Z

Log entire PendingSettlementResult immediately prior to execution.

gitguardian · 2024-10-25T16:14:46Z

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
14297015	Triggered	Generic High Entropy Secret	`aecfe41`	testutil/testclient/localnet.go	View secret
12819930	Triggered	Generic Password	`3187418`	localnet/kubernetes/observability-prometheus-stack.yaml	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secrets safely. Learn here the best practices.
Revoke and rotate these secrets.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}

…tlement # By Redouane Lakrache (3) and others # Via GitHub * pokt/main: [Observability] Foundation for load testing telemetry (#832) [Tokenomics] Implement Global Mint Reimbursement Request (#878) [Docs] Assign owners to all `TODO_BETA` (#898) [Supplier] Add supplier staking fee (#883) [Application] Single Service Applications (#886) # Conflicts: # testutil/keeper/tokenomics.go # x/tokenomics/keeper/settle_pending_claims.go # x/tokenomics/keeper/token_logic_modules.go # x/tokenomics/keeper/token_logic_modules_test.go # x/tokenomics/module/abci.go # x/tokenomics/types/errors.go

github-actions · 2024-11-04T10:59:25Z

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 889)
Grafana network dashboard for devnet-issue-889

github-actions · 2024-11-04T10:59:25Z

The image is going to be pushed after the next commit.

You can use make trigger_ci to push an empty commit.

If you also want to run E2E tests, please add devnet-test-e2e label.

x/tokenomics/token_logic_module/validation.go

x/tokenomics/token_logic_module/reimbursement_requests.go

Olshansk · 2024-11-06T19:30:48Z

config.yml

+    # This is the address that will receive the dao/foundation rewards during claim settlement (global mint TLM).
+    # DEV_NOTE: This is a temporary solution which allows us to configure the dao reward address between networks
+    # (e.g. localnet, testnet, etc.)
+    # TODO_TECHDEBT: Make this a tokenomics module parameter.


This is pretty important and I'd rather do it sooner rather than later.

Would prefer that we add a PR (before this) and avoid this TECHDEBT altogether. Wouldn't that be easier/faster/cleaner?

@bryanchriswhite

How long would you say it takes you to add a new param right now?

Do you think there's a way (a need?) to streamline/automate [1] in some way?

[1] https://dev.poktroll.com/develop/developer_guide/adding_params?_highlight=para

Olshansk · 2024-11-06T19:33:47Z

config.yml

@@ -1,6 +1,13 @@
 version: 1
 build:
  main: cmd/poktrolld
+  ldflags:
+    # The dao reward address SHOULD match that of the "pnf" below (i.e. `make poktrolld_addr ACC_NAME=pnf`).


Why is this a SHOULD and not a MUST?

Olshansk · 2024-11-06T19:35:23Z

x/tokenomics/types/params.go

@@ -4,6 +4,15 @@ import (
 	paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
 )

+// DaoRewardAddress is the address that will receive the dao/foundation rewards
+// during claim settlement (global mint TLM). It is intended to be assigned via
+// -ldflags="-X github.com/pokt-network/poktroll/app.FoundationRewardAddress=<foundation_address>"


In config.yaml, the name is DaoRewardAddress and here its FoundationRewardAddress.

Per my other comment, I would like to avoid merging this flag-based approach to main altogether.

Olshansk

Really great work @bryanchriswhite!

My comments/messages are terse but I tried to go through the whole PR.

Overall, it's VERY clean. VERY easy to read. VERY well organized.

I honestly see no blockers and most of my comments are just minor NITS & comment improvements.

Again, very well designed 🔥

Olshansk · 2024-11-06T19:43:15Z

tests/integration/tokenomics/token_logic_modules/unhaltable_test.go

@@ -0,0 +1,5 @@
+package token_logic_modules
+
+// TODO_TEST(@bryanchriswhite): Settlement proceeds in the face of errors


Please create tests w/ a skip inside of them (cleaner)

Please add more details/steps on what you intended to test (it'll help you not to forget how to implement the test later)

Olshansk · 2024-11-06T19:59:03Z

config.yml

+    # This is the address that will receive the dao/foundation rewards during claim settlement (global mint TLM).
+    # DEV_NOTE: This is a temporary solution which allows us to configure the dao reward address between networks
+    # (e.g. localnet, testnet, etc.)
+    # TODO_TECHDEBT: Make this a tokenomics module parameter.


Use TODO_TECHDEBT only when it's like a post mainnet and doesn't have a timeline sort of thing.

Olshansk · 2024-11-06T21:42:24Z

testutil/keeper/tokenomics.go

-// TokenomicsKeepersOpt is a function which receives and potentially modifies the context
-// and tokenomics keepers during construction of the aggregation.
-type TokenomicsModuleKeepersOpt func(context.Context, *TokenomicsModuleKeepers) context.Context
+// tokenomicsModuleKeepersConfig is a configuration struct for a TokenomicsModuleKeepers


Can you add test in the naming of all of these so its explicit its not for core business logic in prod?

Olshansk · 2024-11-06T21:42:53Z

testutil/keeper/tokenomics.go

+	}
+}
+
+func WithTLMProcessors(processors []tlm.TokenLogicModule) TokenomicsModuleKeepersOptFn {


Olshansk · 2024-11-06T21:43:27Z

testutil/testclient/localnet.go

@@ -38,6 +39,9 @@ var (
 func init() {
 	cmd.InitSDKConfig()

+	// TODO_TECHDEBT: Remove once the DAO reward address is promoted to a tokenomics module param.


TODO_IN_THIS or PRIOR TO THIS

Olshansk · 2024-11-06T22:44:00Z

tests/integration/tokenomics/token_logic_modules/commutative_test.go

+		}
+
+		// The test description is a unique identifier for each permutation.
+		// E.g.: "permutaiton_1_of_2__TLMRelayBurnEqualsMint_TLMGlobalMint"


Thoughts on something like this?

Suggested change

// E.g.: "permutaiton_1_of_2__TLMRelayBurnEqualsMint_TLMGlobalMint"

// E.g.: "permutaiton_1_of_2:TLMRelayBurnEqualsMint_TLMGlobalMint"

Olshansk · 2024-11-06T22:46:24Z

tests/integration/tokenomics/token_logic_modules/commutative_test.go

+			s.createClaims(&s.keepers, 1000)
+			settledResults, expiredResults := s.settleClaims(t)
+
+			// Set the expected state based on the effects of the first iteration;


Suggested change

// Set the expected state based on the effects of the first iteration;

// First iteration only.

// Set the expected state based on the effects of the first iteration.

Olshansk · 2024-11-06T22:46:52Z

tests/integration/tokenomics/token_logic_modules/commutative_test.go

+			settledResults, expiredResults := s.settleClaims(t)
+
+			// Set the expected state based on the effects of the first iteration;
+			// this decouples the assertions from any specific tlm effects.


Thoughts on actually having a function to assert on a results to make sure we don't assert on (for example) 0=0 everywhere?

Olshansk · 2024-11-06T22:54:39Z

tests/integration/tokenomics/token_logic_modules/commutative_test.go

+		keeper.WithApplication(*s.app),
+		keeper.WithSupplier(*s.supplier),
+		keeper.WithModuleParams(map[string]types.Msg{
+			// TODO_TECHDEBT: Set tokenomics mint allocation params to maximize coverage, once available.


Olshansk · 2024-11-06T22:57:10Z

tests/integration/tokenomics/token_logic_modules/commutative_test.go

+// assertExpectedSettlementState asserts that the current network state matches the
+// expected settlement state, and that actualSettledResults and actualExpiredResults
+// match their corresponding expectations.
+func (s *tlmProcessorTestSuite) assertExpectedSettlementState(


Is there any way to update the test such that we moe forward one (or two) more sessions and confirm the state/balances are unchanged (since there are no new claims).

This is likely a different test, but just want to sanity checks the session start/end edge case + lingering claims.

bryanchriswhite added on-chain On-chain business logic code health Cleans up some code devnet-test-e2e tokenomics Token Economics - what else do you need? labels Oct 22, 2024

bryanchriswhite added this to the Shannon Beta TestNet Launch milestone Oct 22, 2024

bryanchriswhite self-assigned this Oct 22, 2024

github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Oct 22, 2024

github-actions bot reviewed Oct 22, 2024

View reviewed changes

bryanchriswhite linked an issue Oct 23, 2024 that may be closed by this pull request

[Tokenomics] Settlement safety #881

Open

6 tasks

bryanchriswhite force-pushed the issues/881/refactor/settlement branch from 2df9645 to 77a3944 Compare October 23, 2024 17:56

github-actions bot reviewed Oct 23, 2024

View reviewed changes

github-actions bot reviewed Oct 24, 2024

View reviewed changes

bryanchriswhite force-pushed the issues/881/refactor/settlement branch from 3731434 to 65c9499 Compare October 24, 2024 17:31

bryanchriswhite added 8 commits October 24, 2024 22:09

wip: refactor:

656994e

- Isolate TLM processors from the tokenomics keeper - Restructure TLM processors to avoid non-deterministic execution - Consolidate and postpone ALL state modification until the end of settlement

chore: configurable tlm processors and dao reward address global vari…

aab77f5

…able

refactor: rename session testkeeper WithSharedModuleParams helper

f1339ef

refactor: TokenomicsModuleKeepers helpers to support injecting TLM pr…

bec435f

…ocessors

refactor: eliminate non-deterministic iteration from claim settlement

2f33a71

test: ALL TLMs are commutative

c39c096

refactor: val & cons addr helpers

86e28cb

chore: don't schedule transfers for 0 amounts

24ceb12

bryanchriswhite force-pushed the issues/881/refactor/settlement branch from d5eb029 to 24ceb12 Compare October 24, 2024 20:40

bryanchriswhite added 3 commits October 24, 2024 22:56

test: improve permute test

a111ecc

wip: chore: add comments

c04a919

test: split up tlm processor test suite

29a75b2

github-actions bot reviewed Oct 24, 2024

View reviewed changes

bryanchriswhite commented Oct 24, 2024

View reviewed changes

x/tokenomics/module/abci.go Outdated Show resolved Hide resolved

bryanchriswhite commented Oct 24, 2024

View reviewed changes

x/tokenomics/token_logic_module/global_mint.go Show resolved Hide resolved

bryanchriswhite added 3 commits October 25, 2024 13:42

refactor: rename TokenLogicModuleProcessor to TokenLogicModule & Toke…

d279222

…nLogicModule to TokenLogicModuleId

chore: resolve outstanding TODOs

6b96a1e

chore: self-review improvements & fix linter errors

67777cc

bryanchriswhite force-pushed the issues/881/refactor/settlement branch from ace0637 to 67777cc Compare October 25, 2024 11:50

chore: self-review improvements

bcc4f77

bryanchriswhite marked this pull request as ready for review October 25, 2024 12:17

bryanchriswhite requested review from red-0ne and Olshansk October 25, 2024 12:18

wip: fix: failing tests

aecfe41

fix: failing test and simplify

cbb81c4

bryanchriswhite assigned red-0ne Oct 25, 2024

bryanchriswhite mentioned this pull request Oct 25, 2024

[Tokenomics] Implement Global Mint Reimbursement Request #878

Merged

9 tasks

bryanchriswhite added push-image CI related - pushes images to ghcr.io devnet devnet-test-e2e and removed push-image CI related - pushes images to ghcr.io devnet devnet-test-e2e labels Nov 4, 2024

github-actions bot reviewed Nov 4, 2024

View reviewed changes

x/tokenomics/token_logic_module/validation.go Outdated Show resolved Hide resolved

x/tokenomics/token_logic_module/reimbursement_requests.go Outdated Show resolved Hide resolved

chore: add godoc comments

6a02d5e

Olshansk unassigned red-0ne Nov 4, 2024

Olshansk requested changes Nov 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tokenomics] Harden settlement (part 1: TLM isolation) #889

[Tokenomics] Harden settlement (part 1: TLM isolation) #889

bryanchriswhite commented Oct 22, 2024 •

edited

Loading

github-actions bot commented Oct 22, 2024

github-actions bot left a comment

bryanchriswhite commented Oct 25, 2024 •

edited

Loading

bryanchriswhite commented Oct 25, 2024

gitguardian bot commented Oct 25, 2024 •

edited

Loading

github-actions bot commented Nov 4, 2024

github-actions bot commented Nov 4, 2024

Olshansk Nov 6, 2024

Olshansk Nov 6, 2024

Olshansk Nov 6, 2024

Olshansk Nov 6, 2024

Olshansk left a comment

Olshansk Nov 6, 2024

Olshansk Nov 6, 2024

Olshansk Nov 6, 2024

Olshansk Nov 6, 2024

Olshansk Nov 6, 2024

Olshansk Nov 6, 2024

Olshansk Nov 6, 2024

Olshansk Nov 6, 2024

Olshansk Nov 6, 2024

Olshansk Nov 6, 2024

		@@ -0,0 +1,5 @@
		package token_logic_modules

		// TODO_TEST(@bryanchriswhite): Settlement proceeds in the face of errors

	// E.g.: "permutaiton_1_of_2__TLMRelayBurnEqualsMint_TLMGlobalMint"
	// E.g.: "permutaiton_1_of_2:TLMRelayBurnEqualsMint_TLMGlobalMint"

	// Set the expected state based on the effects of the first iteration;
	// First iteration only.
	// Set the expected state based on the effects of the first iteration.

[Tokenomics] Harden settlement (part 1: TLM isolation) #889

Are you sure you want to change the base?

[Tokenomics] Harden settlement (part 1: TLM isolation) #889

Conversation

bryanchriswhite commented Oct 22, 2024 • edited Loading

Summary

Issue

Type of change

Testing

Sanity Checklist

github-actions bot commented Oct 22, 2024

github-actions bot left a comment

Choose a reason for hiding this comment

bryanchriswhite commented Oct 25, 2024 • edited Loading

bryanchriswhite commented Oct 25, 2024

gitguardian bot commented Oct 25, 2024 • edited Loading

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

github-actions bot commented Nov 4, 2024

github-actions bot commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanchriswhite commented Oct 22, 2024 •

edited

Loading

bryanchriswhite commented Oct 25, 2024 •

edited

Loading

gitguardian bot commented Oct 25, 2024 •

edited

Loading