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

[Utility] Per-chain Relays To Token Multiplier #494

Closed
9 tasks
Olshansk opened this issue Apr 25, 2024 · 11 comments
Closed
9 tasks

[Utility] Per-chain Relays To Token Multiplier #494

Olshansk opened this issue Apr 25, 2024 · 11 comments
Assignees
Labels
community A ticket intended to potentially be picked up by a community member protocol General core protocol related changes utility

Comments

@Olshansk
Copy link
Member

Olshansk commented Apr 25, 2024

Objective

Feature parity with RTTM from Morse.

Origin Document

Goals

  • Achieve RTTM parity w/ Morse

Deliverables

  • Research Morse: Study & understand how RTTM works in Morse
  • Governance: Add the RTTM governance parameter
  • Implement: Implement RTTM in Shannon
  • Integrate: Make sure the RTTM values are probability capture in the Sparse Merkle Trie
  • CLI: Update the poktrolld (if necessary) to support updating the RTTM governance parameter
  • Test: Add unit, integration and E2E tests
  • Tooling: Add Makefile targets to easily trigger (on LocalNet) and test (e2e) modifying / updating / testing different RTTM values
  • Document: Add a new page to dev.poktroll.com explaining how RTTM works and how to use it
  • Comments: Add/update TODOs and comments alongside the source code so it is easier to follow.

Non-goals / Non-deliverables

  • Expanding beyond what Morse has for RTTM (except integration into the SMT)

Estimated Days of Work

2 days

Disclaimer: This is the total projected number of estimated hours to completion & merge. The owner of this tickets is expected to use this GitHub issue to communicate with the core protocol team along the way, with update & feedback for each deliverable throughout the duration of this work._


Creator: @Olshansk
Co-Owners: @moatus

@Olshansk Olshansk added protocol General core protocol related changes utility labels Apr 25, 2024
@Olshansk Olshansk added this to the Shannon MainNet milestone Apr 25, 2024
@Olshansk Olshansk added this to Shannon Apr 25, 2024
@Olshansk Olshansk added the community A ticket intended to potentially be picked up by a community member label Apr 25, 2024
@Olshansk Olshansk moved this to 🔖 Ready in Shannon Apr 25, 2024
@rBurgett
Copy link
Contributor

@Olshansk I'd be happy to take this.

@Olshansk
Copy link
Member Author

@rBurgett Sounds great!

Few questions / suggestions (some redundancy from our offline conversations but posting here for posterity)

  1. Q1: When do you think you'll be able to start?
  2. S1: Please start with a draft PR and cc the team (@bryanchriswhite, @Olshansk, @red-0ne) before diving too deep into the implementation, tests, docs, etc...
  3. S2: Make sure to look into ignite to see if there is any scaffolding opportunity (less work, standards, etc...)

CCing @moatus for visibility.

@rBurgett
Copy link
Contributor

I'll start tomorrow and work on it as I can around my current, full work schedule. I'll have a draft PR up by Monday.

@Olshansk Olshansk moved this from 🔖 Ready to 🏗 In progress in Shannon Jun 4, 2024
@Olshansk
Copy link
Member Author

Olshansk commented Jun 5, 2024

Screenshot 2024-06-05 at 1 01 39 PM
Screenshot 2024-06-05 at 1 01 43 PM
Screenshot 2024-06-05 at 1 01 47 PM
Screenshot 2024-06-05 at 1 02 20 PM
Screenshot 2024-06-05 at 1 02 28 PM
Screenshot 2024-06-05 at 1 02 35 PM
Screenshot 2024-06-05 at 1 02 39 PM

@Olshansk
Copy link
Member Author

Olshansk commented Jun 5, 2024

@rBurgett I found this thread in the cosmos docs: error while using map in txs w/ the tl;dr is that you can use maps in txs.

Screenshot 2024-06-05 at 1 03 49 PM

@Olshansk
Copy link
Member Author

Olshansk commented Jun 5, 2024

Also, I noticed that we're "re-inventing the wheel" on how params are updated. I did so by surching for "update-params"

Screenshot 2024-06-05 at 1 04 16 PM

@Olshansk
Copy link
Member Author

Olshansk commented Jun 5, 2024

I decided to dive deeper into the whole thing and have an alternative suggestion. CCing @bryanchriswhite and @moatus for context too.

  1. We keep the default ComputeUnitToToken multiple as is.
  2. We update the Service protobuf to have a compute_units_to_token_multiplier
message Service {
  // For example, what if we want to request a session for a certain service but with some additional configs that identify it?
  string id = 1; // Unique identifier for the service

  // TODO_TECHDEBT: Name is currently unused but acts as a reminder that an optional onchain representation of the service is necessary
  string name = 2; // (Optional) Semantic human readable name for the service
  
+  uint64 compute_units_to_token_multiplier = 3;
}
  1. The owner of the Service (i.e. the Source owner) is the only one who can update the multiplier on their source.
  2. The default for it is based on (1)
  3. During session settlement, we query the service and leverage it there
  4. We won't need a global RTTM map (simplifying things)

@rBurgett
Copy link
Contributor

rBurgett commented Jun 5, 2024

@Olshansk that is an interesting alternative. Using a global RTTM map and serializing it as a string isn't complicated, imo, but I'll defer to your suggestion. Now, regarding number four where you say that the default should be the compute_units_to_tokens_multiplier, do you mean that 1) when a service is created we should set that as the service's compute_units_to_tokens_multiplier value? Or, 2) that should we treat the cuttm as an optional service param and fallback to the default during session settlement if a service-specific cuttm is not set?

@Olshansk
Copy link
Member Author

Olshansk commented Jun 6, 2024

tl;dr Let's do the following:

  • Keep the global param variable compute_units_to_tokens_multipler (this will have a default value in genesis)
  • Each Service have it's own compute_units_per_relay that can only be set the service owner/creator (this will need a default value upon service creation)
  • Scratch the original map approach; we have found something easier

This is a result per an offline discussion with @moatus @RawthiL and @studna.

Screenshot_2024-06-05_at_2 59 01_PM

Screenshot 2024-06-05 at 7 47 03 PM

@rBurgett
Copy link
Contributor

rBurgett commented Jun 6, 2024

Sounds like a plan.

@red-0ne
Copy link
Contributor

red-0ne commented Aug 5, 2024

Closing as per #552 and #706.

@red-0ne red-0ne closed this as completed Aug 5, 2024
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Shannon Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community A ticket intended to potentially be picked up by a community member protocol General core protocol related changes utility
Projects
Status: ✅ Done
Development

No branches or pull requests

4 participants