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] Settlement safety #881

Closed
6 tasks
bryanchriswhite opened this issue Oct 14, 2024 · 3 comments · Fixed by #889
Closed
6 tasks

[Tokenomics] Settlement safety #881

bryanchriswhite opened this issue Oct 14, 2024 · 3 comments · Fixed by #889
Assignees
Labels
code health Cleans up some code on-chain On-chain business logic tokenomics Token Economics - what else do you need?

Comments

@bryanchriswhite
Copy link
Contributor

bryanchriswhite commented Oct 14, 2024

Objective

Improve saftey in claim settlement code.

Status

See: Unhaltable settlement notion doc

Origin Document

Discussion with @red-0ne based on recent observations: Claim Settlement Safety - notion doc.

Proposal Draft
type TLMMintBurn struct {
	TLMName string
	DestinationAddr string
	Coin *cosmostypes.Coin
}

type TLMTransfer struct {
	TLMName string
	SourceAddr: string
	DestinationAddr: string	
	Coin *cosmostypes.Coin
}

type TLMProcessingResult { // Supersedes PendingClaimResult
	Claims: []*sharedtypes.Claim
	Mints: []TLMMintBurn
	Burns: []TLMMintBurn
	Transfers: []TLMTransfer
}

func (tlmPR) GetNumComputeUnits() int64
func (tlmPR) GetNumRelays() int64
func (tlmPR) GetNumClaims() int64
func (tlmPR) GetApplicationAddrs() []string
func (tlmPR) GetSupplierAddrs() []string
func (tlmPR) GetSessionEndHeight() int64
func (tlmPR) GetServices() []string // use for determinstic loops
func (tlmPR) RelaysPerServiceMap() map[string]uint64 // SHOULD NEVER be used for loops

func (tlmPR) Append(TLMProcessingResult) TLMProcessingResult
var tokenLogicModuleProcessors = []TokenLogicModule{...}

type TokenLogicModule struct {
	Name string
	Process func(
		context.Context,
		*sharedtypes.Service,
		*sessiontypes.SessionHeader,
		*apptypes.Application,
		*sharedtypes.Supplier,
		cosmostypes.Coin, // This is the "actualSettlementCoin" rather than just the "claimCoin" because of how settlement functions; see ensureClaimAmountLimits for details.
		*servicetypes.RelayMiningDifficulty,
	) (TLMProcessingResult, error)
}

Goals

  • Harden claim settlement business logic against chain halt vectors
    • Isolate ALL TLM logic from keepers and the multistore
    • Mutate the state once, after TLM processing
    • Continue processing other claims if there is an error processing a particular claim
      • Emit an error event
    • Remove map type data structures from claim settlement business logic
  • Improve the TLM processing foundation
    • Redefine TLMs as objects

Deliverables

  • A PR which implements TLMProcessingResult, TokenLogicModule, and dependencies.
  • A PR which refactors each TLM method to be a static TokenLogicModule in its own file and tokenLogicModuleProcessorMap map as a []TokenLogicModule slice.

Non-goals / Non-deliverables

  • Implementing or refactoring any TLM business logic.

General deliverables

  • Comments: Add/update TODOs and comments alongside the source code so it is easier to follow.
  • Testing: Add new tests (unit and/or E2E) to the test suite.
  • Makefile: Add new targets to the Makefile to make the new functionality easier to use.
  • Documentation: Update architectural or development READMEs; use mermaid diagrams where appropriate.

Creator: @bryanchriswhite
Co-Owners: @red-0ne

@bryanchriswhite bryanchriswhite added on-chain On-chain business logic code health Cleans up some code tokenomics Token Economics - what else do you need? labels Oct 14, 2024
@bryanchriswhite bryanchriswhite self-assigned this Oct 14, 2024
@bryanchriswhite bryanchriswhite changed the title [On-Chain] Settlement safety [Tokenomics] Settlement safety Oct 14, 2024
@bryanchriswhite bryanchriswhite moved this to 🔖 Ready in Shannon Oct 14, 2024
@Olshansk
Copy link
Member

Olshansk commented Oct 16, 2024

@bryanchriswhite Leaving feedback on the notion doc here.

Using a map REQUIRES that all TLM functions are commutative. While it is probably a great requirement or goal, it doesn’t mean that we need to make a design choice which makes it a hard constraint, thereby introducing a chain halt pathway.

@bryanchriswhite I want us to keep this requirement.

  • Keeps TLMs independent of one another
  • Makes it easier to ensure lack of dependency (execution results) from one on another
  • Reduces risk of adding/removing them in the future
  • Enables experimantion easier

Isolate TLM logic from keepers

👍 if possible.

Mutate all state deterministically, after TLM processing/calculations

👍 if possible

Ensure a single unexpected claim error doesn’t halt claim processing

Makes sense.

TLMName string

Can you make this an iota (enum)

type TLMProcessingResult { // Supersedes PendingClaimResult
	Claims: []*sharedtypes.Claim
	Mints: []TLMMintBurn
	Burns: []TLMMintBurn
	Transfers: []TLMTransfer
}

I'm not a fan of parallel arrays and would rather do something like this:

// Supersedes PendingClaimResult
type TLMProcessingResult struct { 
	Claim *sharedtypes.Claim
	Mints []TLMMintBurn
	Burns []TLMMintBurn
	Transfers []TLMTransfer
}

type TLMProcessingResults struct {
	TLMResults []*TLMProcessingResult
}

Other asks, ensure:

  1. Sufficient error/warning/info logging + events along the way
  2. Ensure (1) for each individual result
  3. Would love to be able to do a simple SQL query of (2) when the time is right

@bryanchriswhite bryanchriswhite moved this from 🔖 Ready to 🏗 In progress in Shannon Oct 21, 2024
@bryanchriswhite
Copy link
Contributor Author

bryanchriswhite commented Oct 21, 2024

Using a map REQUIRES that all TLM functions are commutative. While it is probably a great requirement or goal, it doesn’t mean that we need to make a design choice which makes it a hard constraint, thereby introducing a chain halt pathway.

@bryanchriswhite I want us to keep this requirement.

  • Keeps TLMs independent of one another
  • Makes it easier to ensure lack of dependency (execution results) from one on another
  • Reduces risk of adding/removing them in the future
  • Enables experimantion easier

No argument here. My point was just that there's no need to aim a footgun at ourselves by using a map.

TLMName string

Can you make this an iota (enum)

👍

I'm not a fan of parallel arrays and would rather do something like this:

// Supersedes PendingClaimResult
type TLMProcessingResult struct { 
	Claim *sharedtypes.Claim
	Mints []TLMMintBurn
	Burns []TLMMintBurn
	Transfers []TLMTransfer
}

type TLMProcessingResults struct {
	TLMResults []*TLMProcessingResult
}

Good point, agreed. 👍

  1. Sufficient error/warning/info logging + events along the way

👍

  1. Ensure (1) for each individual result
  2. Would love to be able to do a simple SQL query of (2) when the time is right

@Olshansk, can you elaborate on these, I'm not sure what's being referenced or asked.

@Olshansk
Copy link
Member

Ensure (1) for each individual result

Request that for each successful / failed claim, there is sufficient (use your best judgment) error/warning/info logging + events along the way.

Would love to be able to do a simple SQL query of (2) when the time is right

Once these events are indexed in pocketdex, adding a few examples like this would go a long way:

  • Query all successful claims
  • Query all failed claims
  • Query all mints
  • Query all burns
  • Etc....

bryanchriswhite added a commit that referenced this issue Nov 19, 2024
## 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

- #881 

## 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
- [ ] **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

---------

Co-authored-by: Redouane Lakrache <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Shannon Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Cleans up some code on-chain On-chain business logic tokenomics Token Economics - what else do you need?
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants