[Utility] Feat: Chain-specific compute unit to tokens multipliers #552
Conversation
Olshansk
added
protocol
Olshansk
modified the milestones:
Research, Writing & Publication,
Shannon Beta TestNet Launch
Olshansk
changed the title
There was a problem hiding this comment.
@rBurgett great start!
In the future, add TODO_IN_THIS_PR_DISCUSS: ... so we can keep all the conversations in separate threads in line.
I did an initial quick review (not in depth) to unblock you, and will go deeper during the the next one. I think the answer below and in some comments should provide the context you need.
Should new parameter be stored as a map<string, int64> or as a string (e.g. 0021=42,0001=42) and then parsed when used? Currently, update messages expect one of string, int64, or bytes and a map is not valid within oneof unless wrapped in a new message. Tests will not pass until I implement the update functionality.
Replied in comments.
The issue uses the term "chain" but as far as I can find, the project refers to what used to be chains as "services" so should the parameter be called service-specific rather than chain-specific?
Yes.
I added necessary validation on chain (service) IDs. I could not find any other place in the codebase that checks service IDs. Currently, I check if all characters are uppercase hex with a length of at least four. That covers all current supported chains in Pocket, but
Can you add a `TODO_BLOCKER_BETA: Discuss and align on what service IDs should be like
I don't know what exact requirements are expected or if they have changed with Shannon. In tests, there are chain IDs with non-hex characters. If we would enforce hex, I'd need to update those tests as well.
This is still an open design space in Shannon.
Now is not the right time to dive into it (it will be in a month or so), but I anticipate they will either be semantic strings (e.g. ETH), or a serailized structure that contains metadata.
Is this meant to replace the compute_units_to_tokens_multiplier param? Or, will we continue supporting a general/default cuttm? Deprecating the general compute_units_to_tokens_multiplier will require updates to many tests.
Yea, let's get rid of the global constant!
| // The chain-specific amount of upokt that a compute unit should translate to when settling a session. | ||
| map<string, uint64> chain_compute_units_to_tokens_multipliers = 2 [(gogoproto.jsontag) = "chain_compute_units_to_tokens_multipliers", (gogoproto.moretags) = "yaml:\"chain_compute_units_to_tokens_multipliers\""]; |
There was a problem hiding this comment.
| // The chain-specific amount of upokt that a compute unit should translate to when settling a session. | |
| map<string, uint64> chain_compute_units_to_tokens_multipliers = 2 [(gogoproto.jsontag) = "chain_compute_units_to_tokens_multipliers", (gogoproto.moretags) = "yaml:\"chain_compute_units_to_tokens_multipliers\""]; | |
| // The service-specific amount of upokt that a compute unit should translate to when settling a session. | |
| map<string, uint64> service_compute_units_to_tokens_multipliers = 2 [(gogoproto.jsontag) = "service_compute_units_to_tokens_multipliers", (gogoproto.moretags) = "yaml:\"service_compute_units_to_tokens_multipliers\""]; |
proto/poktroll/tokenomics/tx.proto
Outdated
| @@ -55,6 +55,10 @@ message MsgUpdateParam { | |||
| string as_string = 3 [(gogoproto.jsontag) = "as_string"]; | |||
| int64 as_int64 = 6 [(gogoproto.jsontag) = "as_int64"]; | |||
| bytes as_bytes = 7 [(gogoproto.jsontag) = "as_bytes"]; | |||
| // cannot add a map<string, int64> here | |||
There was a problem hiding this comment.
| // cannot add a map<string, int64> here | |
| // TODO_IN_THIS_PR: Discuss the issue below | |
| // cannot add a map<string, int64> here |
There was a problem hiding this comment.
This automatically fails the build. See Makefile for more helpers.
proto/poktroll/tokenomics/tx.proto
Outdated
| @@ -55,6 +55,10 @@ message MsgUpdateParam { | |||
| string as_string = 3 [(gogoproto.jsontag) = "as_string"]; | |||
| int64 as_int64 = 6 [(gogoproto.jsontag) = "as_int64"]; | |||
| bytes as_bytes = 7 [(gogoproto.jsontag) = "as_bytes"]; | |||
| // cannot add a map<string, int64> here | |||
| // do we want to create a custom message type | |||
| // or just store the chain-specific multipliers | |||
There was a problem hiding this comment.
Add a type called:
message ServiceComputeUnitsToTokenMultiplers {
map<string, int64> multipliers = 1;
}There was a problem hiding this comment.
@Olshansk do we want the property named ServiceComputeUnitsToTokenMultiplers or ServiceComputeUnitsToTokensMultiplers? The previous property was called compute_units_to_tokens_multiplier with a plural tokens so I have thus far used that convention. Either works for me, I just want to be consistent.
There was a problem hiding this comment.
@Olshansk do we want the tokenomics param to use this type as well or should I just use this for update transactions? Currently, the service_compute_units_to_tokens_multipliers param is a raw map<string, int64>.
There was a problem hiding this comment.
This should be the end state:
message ComputeUnitsToTokensMultipliers {
map<string, int64> service_compute_units_to_tokens_map = 1;
}
message Params {
ComputeUnitsToTokensMultipliers compute_units_to_tokens_multipliers = 1;
}I don't think MsgUpdateParam will be able to handle this though so CCing @bryanchriswhite for thoughts / input.
@rBurgett I don't fully understand the second question. Can you reiterate?
There was a problem hiding this comment.
@Olshansk yeah, MsgUpdateParam did not like that. You answered my questions with your comment above. I should be good to go. Thanks.
There was a problem hiding this comment.
I see the issue now with MsgUpdateParam. 🤔 Using a new type to encapsulate the map would be valid a workaround for this, although it might be a bit verbose.
// proto/poktroll/tokenomics/tx.proto
message MsgUpdateParam {
...
oneof as_type {
string as_string = 3 [(gogoproto.jsontag) = "as_string"];
int64 as_int64 = 6 [(gogoproto.jsontag) = "as_int64"];
bytes as_bytes = 7 [(gogoproto.jsontag) = "as_bytes"];
StringInt64Map as_string_int64_map = 8 [(gogoproto.jsontag) = "as_string_int64_map"];
}
}
// proto/poktroll/tokenomics/params.proto
message Params {
...
StringInt64Map compute_units_to_tokens_multipliers = 2 [(gogoproto.jsontag) = "compute_units_to_tokens_multipliers", (gogoproto.moretags) = "yaml:\"compute_units_to_tokens_multipliers\""];
}
message StringInt64Map {
map<string, int64> entries = 1;
}I don't see any reason why we wouldn't be able to continue to extend support for new types of params in this way.
There was a problem hiding this comment.
@bryanchriswhite for the moment, I have it implemented as @Olshansk suggested above, although I like the idea of generalizing it as you suggest. It is already verbose, so I don't see that as an issue. I am happy to implement it however you folks prefer.
There was a problem hiding this comment.
@rBurgett I would argue that if the reason for changing the data structure like this is to support MsgUpdateParam, then the additional types that result should be describing "generic" types that can be used in as_type. This would be as opposed to having use-case specific types, like @Olshansk's example.
I also think that these new types should be defined in the shared module's params.proto so that they can be reused between modules.
There was a problem hiding this comment.
@bryanchriswhite yeah, that absolutely makes sense. I was going to push up what was supposed to be the full PR this weekend, but a head cold knocked me out. As I finish it, I'll go ahead and make this generic and extensible as you suggest. I assume @Olshansk will say something if he disagrees with that.
| if found { | ||
| settlementAmt = k.getCoinFromComputeUnitsByChainMultiplier(root, multiplier) | ||
| } else { | ||
| // ToDo: should this be an error rather than a warning? should chain-specific multipliers be mandatory? |
There was a problem hiding this comment.
- Use the appropriate TODOs: https://github.com/pokt-network/poktroll/blob/main/Makefile#L456
- We also have this if you want a reminder to avoid merging the code in:
poktroll/.github/workflows/reviewdog.yml
Line 11 in 6f2fe5b
| @@ -47,8 +47,8 @@ func TestSettleSessionAccounting_HandleAppGoingIntoDebt(t *testing.T) { | |||
| SessionHeader: &sessiontypes.SessionHeader{ | |||
| ApplicationAddress: app.Address, | |||
| Service: &sharedtypes.Service{ | |||
| Id: "svc1", | |||
| Name: "svcName1", | |||
| Id: "0021", | |||
There was a problem hiding this comment.
Let's revert this for now.
We haven't made a decision on service IDs in shannon, but there's an opportunity to make them semantic (e.g. ETH), JWTs, structures, etc....
We'll discuss this in the future, but let's not port over legacy yet.
Leaving one comment but ditto elsewhere.
x/tokenomics/types/errors.go
Outdated
| @@ -22,4 +22,5 @@ var ( | |||
| ErrTokenomicsApplicationNewStakeInvalid = sdkerrors.Register(ModuleName, 1115, "application stake cannot be reduced to a -ve amount") | |||
| ErrTokenomicsParamNameInvalid = sdkerrors.Register(ModuleName, 1116, "the provided param name is invalid") | |||
| ErrTokenomicsParamInvalid = sdkerrors.Register(ModuleName, 1117, "the provided param is invalid") | |||
| ErrTokenomicsChainMultiplierNotFound = sdkerrors.Register(ModuleName, 1118, "multiplier for chain ID not found") | |||
| @@ -47,6 +47,9 @@ func (msg *MsgUpdateParam) ValidateBasic() error { | |||
| switch msg.Name { | |||
| case ParamComputeUnitsToTokensMultiplier: | |||
| return msg.paramTypeIsInt64() | |||
| case ParamChainComputeUnitsToTokensMultiplier: | |||
| // see note in tx.proto | |||
There was a problem hiding this comment.
- Make sure to have TODO_IN_THIS_PR if this comment is temporary
- #PUC (Please Update Comment) of what we want to see in tx.proto
- Consider providing full relative path to tx.proto to simplify it for future devs
rBurgett
force-pushed
the
issues/494/per-chain_cuttm
branch
from
91de8e5
to
37555c7
Compare
|
@Olshansk I updated my PR moving away from the chain-specific multiplier to a service-based compute units to relay which is combined with the compute units to tokens multiplier when generating rewards. I updated the PR description with my changes. The tests are still all over the place on my computer. I can run the tests individually and they all pass, but when I run them all together with the make file they randomly fail. The e2e tests fail as well, although I am not sure if that is because of my changes or because of the same reasons the other tests randomly fail. I am currently looking at the e2e tests to try and figure out what is going on, but the main functionality has been pushed up and all of the regular tests pass so feel free to look it over and let me know if it is implemented as you expected based on the updated requirements. Thank you! On a related note, I still cannot get the e2e tests working. I am not sure how to debug it. I thought that I would try running the localnet in order to make sure that works, but that doesn't work either. Following the quickstart guide, I have never been able to get the localnet up since beginning work on this and I don't have the slightest idea how to debug it due to my lack of familiarity with the tools. All attempts so far have been fruitless. The Tilt ui lists everything as "Update pending" except for an error on |
proto/poktroll/shared/service.proto
Outdated
| @@ -13,6 +13,9 @@ message Service { | |||
|
|
|||
| // TODO_BETA: 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 | |||
|
|
|||
| // Used with compute_units_to_tokens_multipler to calculate the cost of a relay | |||
There was a problem hiding this comment.
| // Used with compute_units_to_tokens_multipler to calculate the cost of a relay | |
| // Used alongside the global 'compute_units_to_tokens_multipler' to calculate the cost of a relay for this service |
proto/poktroll/shared/service.proto
Outdated
| @@ -13,6 +13,9 @@ message Service { | |||
|
|
|||
| // TODO_BETA: 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 | |||
|
|
|||
| // Used with compute_units_to_tokens_multipler to calculate the cost of a relay | |||
| uint64 compute_units_per_relay = 3; // Compute units required per relay | |||
There was a problem hiding this comment.
| uint64 compute_units_per_relay = 3; // Compute units required per relay | |
| uint64 compute_units_per_relay = 3; // Compute units required per relay for this service |
x/service/module/tx_add_service.go
Outdated
| @@ -15,18 +16,27 @@ var _ = strconv.Itoa(0) | |||
|
|
|||
| func CmdAddService() *cobra.Command { | |||
| cmd := &cobra.Command{ | |||
| Use: "add-service <service_id> <service_name>", | |||
| Use: fmt.Sprintf("add-service <service_id> <service_name> <compute_units_per_relay: default={%d}>", types.DefaultComputeUnitsPerRelay), | |||
There was a problem hiding this comment.
Need to show that it's optional
| Use: fmt.Sprintf("add-service <service_id> <service_name> <compute_units_per_relay: default={%d}>", types.DefaultComputeUnitsPerRelay), | |
| Use: fmt.Sprintf("add-service <service_id> <service_name> [compute_units_per_relay: default={%d}]", types.DefaultComputeUnitsPerRelay), |
x/service/module/tx_add_service.go
Outdated
| if err != nil { | ||
| return types.ErrServiceInvalidComputUnitsPerRelay.Wrapf("unable to parse as uint64: %s", args[2]) | ||
| } | ||
| } |
There was a problem hiding this comment.
Please add a log statement in the else case saying: "Use default ..."
There was a problem hiding this comment.
@Olshansk from what I see, the loggers are tied to the keepers. In this context, I don't know how to access the appropriate logger. I looked in all of the other CLI handlers and I do not see any previous examples of printing logs from this context. Do you just want a simple PrintLn here? Or, would clientCtx.PrintString() be appropriate? Or, is there another way to get access to a logger from outside of a keeper?
| // Retrieve the existing tokenomics params | ||
| params := k.GetParams(ctx) | ||
|
|
||
| upokt := math.NewInt(int64(root.Sum() * params.ComputeUnitsToTokensMultiplier)) | ||
| upokt := math.NewInt(int64(root.Sum() * computeUnitsPerRelay * params.ComputeUnitsToTokensMultiplier)) |
There was a problem hiding this comment.
| upokt := math.NewInt(int64(root.Sum() * computeUnitsPerRelay * params.ComputeUnitsToTokensMultiplier)) | |
| claimedComputeUnits := root.Sum() | |
| upokt := math.NewInt(int64(claimedComputeUnits * computeUnitsPerRelay * params.ComputeUnitsToTokensMultiplier)) |
| @@ -36,11 +36,18 @@ func TestMsgAddService_ValidateBasic(t *testing.T) { | |||
| Service: sharedtypes.Service{Id: "svc1"}, // Name intentionally omitted | |||
| }, | |||
| expectedErr: ErrServiceMissingName, | |||
| }, { | |||
| desc: "valid service supplier address - no compute units per relay", | |||
There was a problem hiding this comment.
| desc: "valid service supplier address - no compute units per relay", | |
| desc: "valid service supplier address - zero compute units per relay", |
| // Retrieve the existing tokenomics params | ||
| params := k.GetParams(ctx) | ||
|
|
||
| upokt := math.NewInt(int64(root.Sum() * params.ComputeUnitsToTokensMultiplier)) | ||
| upokt := math.NewInt(int64(root.Sum() * computeUnitsPerRelay * params.ComputeUnitsToTokensMultiplier)) |
There was a problem hiding this comment.
TODO_MAINNET: Use math.BigInt to avoid potential overflows
There was a problem hiding this comment.
@Olshansk are you saying that I should add that as a comment?
| var args []string | ||
| // Prepare the arguments for the CLI command | ||
| args := []string{ | ||
| test.service.Id, | ||
| test.service.Name, | ||
| fmt.Sprintf("--%s=%s", flags.FlagFrom, test.supplierAddress), | ||
| // Only include compute units per relay argument if provided | ||
| if test.service.ComputeUnitsPerRelay > 0 { | ||
| args = []string{ | ||
| test.service.Id, | ||
| test.service.Name, | ||
| strconv.FormatUint(test.service.ComputeUnitsPerRelay, 10), | ||
| fmt.Sprintf("--%s=%s", flags.FlagFrom, test.supplierAddress), | ||
| } | ||
| } else { | ||
| args = []string{ | ||
| test.service.Id, | ||
| test.service.Name, | ||
| fmt.Sprintf("--%s=%s", flags.FlagFrom, test.supplierAddress), | ||
| } |
There was a problem hiding this comment.
Wdyt of this?
argsAndFlags := []string{
test.service.Id,
test.service.Name,
}
if test.service.ComputeUnitsPerRelay > 0 {
argsAndFlags := append(args, strconv.FormatUint(test.service.ComputeUnitsPerRelay, 10),)
}
argsAndFlags = append(args, fmt.Sprintf("--%s=%s", flags.FlagFrom, test.supplierAddress))
Re tests: Can you open up a new GitHub issue / bug and provide more details on running E2E and uni tests from main on your machine, we can debug there and cc others. |
There was a problem hiding this comment.
In terms of a draft. LGTM so far.
@bryanchriswhite recently added [1] for everything we need to make it work E2E (with helpers, unit tests, etc..). Can you PTAL?
[1] https://dev.poktroll.com/develop/developer_guide/adding_params
| @@ -50,14 +51,16 @@ func TestCLI_AddService(t *testing.T) { | |||
| // Wait for a new block to be committed | |||
| require.NoError(t, net.WaitForNextBlock()) | |||
|
|
|||
| // Prepare two valid services | |||
| // Prepare three valid services | |||
There was a problem hiding this comment.
Oops, I'll remove that.
| "github.com/pokt-network/poktroll/x/shared/types" | ||
| ) | ||
|
|
||
| const ( | ||
| DefaultComputeUnitsPerRelay uint64 = 1 | ||
| ComputeUnitsPerRelayMax uint64 = 2 ^ 16 |
There was a problem hiding this comment.
#PUC (Please Update Comment) why we added this
| @@ -184,10 +183,11 @@ func (k Keeper) SettleSessionAccounting( | |||
| return nil | |||
| } | |||
|
|
|||
| func (k Keeper) getCoinFromComputeUnits(ctx context.Context, root smt.MerkleRoot) sdk.Coin { | |||
| func (k Keeper) getCoinFromComputeUnits(ctx context.Context, root smt.MerkleRoot, computeUnitsPerRelay uint64) sdk.Coin { | |||
There was a problem hiding this comment.
NIT: thoughts on renaming this to "computeUnitsToCoins"?
There was a problem hiding this comment.
That would be confusing. It is the compute_units_per_relay param from the service, so imo the name is appropriate.
There was a problem hiding this comment.
- But this function is on the receiver, not on the service.
- There is no service in context (not the receiver, not the module, etc...)
- This function is designed in a "functional" manner: provide computeUnits and get coins. We're not "getting it from the service"
There was a problem hiding this comment.
@Olshansk I can rename it if you'd like, but I don't get why the compute units per relay would be named compute units to coins. It isn't compute units for coins, it's compute units per relay. The compute units to tokens multiplier tokenomics param defines the coin for compute units.
There was a problem hiding this comment.
How about computeUnitsPerRelayToNumCoins and we add a godoc with a further explanation?
There was a problem hiding this comment.
@Olshansk ohhh, you are talking about the function name. The parameter was highlighted, so I thought your comment was about the computeUnitsPerRelay parameter. I couldn't figure out why you wanted to name it something completely separate from what it was. I'll change the function name as you suggest.
There was a problem hiding this comment.
The parameter was highlighted
That's just hot GitHub works: highlighting the addition.
I'll change the function name as you suggest.
👍
There was a problem hiding this comment.
ANother few comments.
Still missing:
- Details from https://dev.poktroll.com/develop/developer_guide/adding_params
- E2E test
| @@ -81,6 +84,15 @@ func TestCLI_AddService(t *testing.T) { | |||
| supplierAddress: account.Address.String(), | |||
| service: svc1, | |||
| }, | |||
| { | |||
| desc: "valid - add new service without compute units per relay", | |||
There was a problem hiding this comment.
Shouldn't we make this invalid?
There was a problem hiding this comment.
Looked at the coce.
Please update test description to explain that it's valid because it defaults to the global value.
| "github.com/pokt-network/poktroll/x/shared/types" | ||
| ) | ||
|
|
||
| const ( | ||
| DefaultComputeUnitsPerRelay uint64 = 1 | ||
| // ComputeUnitsPerRelayMax is the maximum allowed compute_units_per_relay value when adding or updating a service |
There was a problem hiding this comment.
| // ComputeUnitsPerRelayMax is the maximum allowed compute_units_per_relay value when adding or updating a service | |
| // ComputeUnitsPerRelayMax is the maximum allowed compute_units_per_relay value when adding or updating a service. | |
| // TODO_MAINNET: The reason we have a maximum is to account for potential integer overflows. This is | |
| // something that needs to be revisited or reconsidered prior to mainnet. |
| @@ -184,10 +183,11 @@ func (k Keeper) SettleSessionAccounting( | |||
| return nil | |||
| } | |||
|
|
|||
| func (k Keeper) getCoinFromComputeUnits(ctx context.Context, root smt.MerkleRoot) sdk.Coin { | |||
| func (k Keeper) getCoinFromComputeUnits(ctx context.Context, root smt.MerkleRoot, computeUnitsPerRelay uint64) sdk.Coin { | |||
There was a problem hiding this comment.
- But this function is on the receiver, not on the service.
- There is no service in context (not the receiver, not the module, etc...)
- This function is designed in a "functional" manner: provide computeUnits and get coins. We're not "getting it from the service"
|
@rBurgett What do you think your ETA on finishing this is?
|
|
@Olshansk the only two remaining things I know are (unless something got lost in all these GitHub comments):
I can update documentation, but until I can run a localnet and e2e tests, I can't add a new e2e test. I'm still trying to figure out what is wrong there, but it's a losing battle so far. If I could successfully run a localnet and e2e tests, for sure I could have it ready by Monday. Otherwise, I can't say. |
Comments:
Call to action:
|
|
I just noticed the PR is open from the repo under
|
|
@Olshansk I'm waiting on one of you for a response. I wrote in my comment a week ago the status of the PR. If you need something else, please let me know. I want to have this finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- testutil/keeper/tokenomics.go (2 hunks)
- testutil/proof/fixture_generators.go (2 hunks)
- x/tokenomics/keeper/keeper_settle_pending_claims_test.go (3 hunks)
- x/tokenomics/keeper/settle_session_accounting.go (5 hunks)
- x/tokenomics/keeper/settle_session_accounting_test.go (16 hunks)
Additional comments not posted (7)
testutil/proof/fixture_generators.go (1)
44-44: Modification inClaimWithRandomHashto handleserviceId.The function now explicitly passes an empty string for
serviceIdwhen callingBaseClaim. This change ensures that the defaultserviceId("svc11") is used, aligning with the new logic inBaseClaim.This is a good pattern to ensure that the function's behavior remains predictable even with the addition of the new parameter.
x/tokenomics/keeper/settle_session_accounting.go (1)
201-203: Introduction ofrelayCountToCoinfunction.This new function encapsulates the logic to calculate the amount of uPOKT to mint based on the number of relays, service-specific compute units per relay, and the tokenomics parameters. This is a significant improvement in modularity and reusability.
This function abstracts complex calculations and makes the codebase more maintainable.
testutil/keeper/tokenomics.go (1)
Line range hint
79-114: Enhanced flexibility inTokenomicsKeeperWithActorAddrs.The function now accepts a
serviceparameter, allowing for dynamic configuration ofapplication.ServiceConfigs. This change increases the function's flexibility and utility in testing scenarios.This modification enhances the test setup's adaptability by allowing different service configurations to be injected directly.
x/tokenomics/keeper/settle_session_accounting_test.go (2)
Line range hint
33-67: Updated test setup to handleserviceconfigurations.The test
TestSettleSessionAccounting_HandleAppGoingIntoDebtnow includes aservicesetup, which is crucial for ensuring that the new service configurations are correctly handled in the test environment.This update is necessary to align the tests with the new functionality introduced in the main codebase.
Line range hint
91-139: Comprehensive service handling in tests.The test
TestSettleSessionAccounting_ValidAccountinghas been updated to include aserviceconfiguration, ensuring that the tests accurately reflect the new functionality.Properly updating the tests to include the new
serviceparameter ensures that the functionality is thoroughly tested and behaves as expected.x/tokenomics/keeper/keeper_settle_pending_claims_test.go (2)
Line range hint
62-100: Review of Service Creation and Configuration in SetupTestThe addition of a new service in
SetupTestis crucial for testing the new functionality. The service is correctly created and added to theApplicationstruct, ensuring it can be used in subsequent tests. This change integrates well with the existing structure and the objectives of the PR.
- Correctness: The service creation and its addition to the application are implemented correctly.
- Consistency: The new service fields align with the described changes in the PR summary.
- Maintainability: The code is clear and maintainable, with the service setup localized within the
SetupTestfunction.
414-414: Review of Service ID Usage in TestSettlePendingClaims_ClaimPendingAfterSettlementThe use of the service
Idfrom theSessionHeaderin theTestSettlePendingClaims_ClaimPendingAfterSettlementfunction is a critical part of testing the new functionality. This ensures that the service-related changes are being correctly utilized within the test scenarios.
- Correctness: The service
Idis correctly retrieved and used in the test.- Integration: This change integrates well with the service setup in the
SetupTestfunction.- Test Coverage: This modification helps in testing the new functionality effectively.
| serviceConfigs := application.ServiceConfigs | ||
| if len(serviceConfigs) == 0 { | ||
| logger.Warn(fmt.Sprintf("application with address %q has no service configs", applicationAddress)) | ||
| return types.ErrTokenomicsApplicationNoServiceConfigs | ||
| } | ||
|
|
||
| var computeUnitsPerRelay uint64 | ||
| for i, sc := range serviceConfigs { | ||
| service := sc.GetService() | ||
| if service.Id == sessionHeader.Service.Id { | ||
| computeUnitsPerRelay = service.ComputeUnitsPerRelay | ||
| break | ||
| } | ||
| if i == len(serviceConfigs)-1 { | ||
| logger.Warn(fmt.Sprintf("service with ID %q not found in application with address %q", sessionHeader.Service.Id, applicationAddress)) | ||
| return types.ErrTokenomicsApplicationNoServiceConfigs | ||
| } | ||
| } |
There was a problem hiding this comment.
Handling of service configurations in SettleSessionAccounting.
The function now retrieves service configurations and uses them to determine computeUnitsPerRelay. This is a critical change as it directly affects how tokens are calculated and minted.
Ensure that the service configurations are correctly set up in all relevant application instances to prevent runtime errors or incorrect token calculations.
Olshansk
changed the title
| @@ -52,8 +64,7 @@ func TestSettleSessionAccounting_HandleAppGoingIntoDebt(t *testing.T) { | |||
| SessionHeader: &sessiontypes.SessionHeader{ | |||
| ApplicationAddress: app.Address, | |||
| Service: &sharedtypes.Service{ | |||
There was a problem hiding this comment.
Why not just use the whole service structure here?
There was a problem hiding this comment.
@Olshansk the session header is the minimal amount of data required to hydrate & retrieve all data relevant to the session.
For reference:
https://github.com/pokt-network/poktroll/blob/main/proto/poktroll/session/session.proto#L15
x/service/module/tx_add_service.go
Outdated
| @@ -15,14 +16,14 @@ var _ = strconv.Itoa(0) | |||
|
|
|||
| func CmdAddService() *cobra.Command { | |||
| cmd := &cobra.Command{ | |||
| Use: "add-service <service_id> <service_name>", | |||
| Use: fmt.Sprintf("add-service <service_id> <service_name> [compute_units_per_relay: default={%d}]", types.DefaultComputeUnitsPerRelay), | |||
| Short: "Add a new service to the network", | |||
| Long: `Add a new service to the network that will be available for applications, | |||
| gateways and suppliers to use. The service id MUST be unique but the service name doesn't have to be. | |||
|
|
|||
| Example: | |||
| $ poktrolld tx service add-service "svc1" "service_one" --keyring-backend test --from $(SUPPLIER) --node $(POCKET_NODE) --home $(POKTROLLD_HOME)`, | |||
There was a problem hiding this comment.
Please update the example w/ compute units per relay.
|
|
||
| upokt := math.NewInt(int64(root.Sum() * params.ComputeUnitsToTokensMultiplier)) | ||
| // relayCountToCoin calculates the amount of uPOKT to mint based on the number of relays, the service-specific ComputeUnitsPerRelay, and the ComputeUnitsPerTokenMultiplier tokenomics param | ||
| func (k Keeper) relayCountToCoin(relays uint64, computeUnitsPerRelay uint64, computeUnitsToTokensMultiplier uint64) sdk.Coin { |
There was a problem hiding this comment.
| func (k Keeper) relayCountToCoin(relays uint64, computeUnitsPerRelay uint64, computeUnitsToTokensMultiplier uint64) sdk.Coin { | |
| func (k Keeper) relayCountToCoin(numRelays, computeUnitsPerRelay, computeUnitsToTokensMultiplier uint64) sdk.Coin { |
| upokt := math.NewInt(int64(root.Sum() * params.ComputeUnitsToTokensMultiplier)) | ||
| // relayCountToCoin calculates the amount of uPOKT to mint based on the number of relays, the service-specific ComputeUnitsPerRelay, and the ComputeUnitsPerTokenMultiplier tokenomics param | ||
| func (k Keeper) relayCountToCoin(relays uint64, computeUnitsPerRelay uint64, computeUnitsToTokensMultiplier uint64) sdk.Coin { | ||
| upokt := math.NewInt(int64(relays * computeUnitsPerRelay * computeUnitsToTokensMultiplier)) |
There was a problem hiding this comment.
Note: I noticed we're not using k Keeper at all in this function so the scoping seems incorrect. Going to leave a comment above on how to slightly restructure things.
| return types.ErrTokenomicsApplicationNoServiceConfigs | ||
| } | ||
|
|
||
| var computeUnitsPerRelay uint64 |
There was a problem hiding this comment.
I personally think capturing lines 102-120 inside relayCountToCoin will make it easier to read.
Olshansk
requested review from
coderabbitai
and removed request for
coderabbitai
…ed comments, and updated documentation
| // TODO_MAINNET: Make it possible to update a service (e.g. update # of compute units per relay | ||
| func CmdAddService() *cobra.Command { |
There was a problem hiding this comment.
Reminder: Address the TODO comment.
The TODO comment indicates that updating a service is a future task.
Do you want me to open a GitHub issue to track this task?
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
x/tokenomics/keeper/settle_session_accounting.go (1)
116-116: Improve logging for better clarity.Consider adding more context to the log message for better clarity.
- logger.Info(fmt.Sprintf("About to start settling claim for %d compute units with CUPR %d and CUTTM %d", claimComputeUnits, computeUnitsPerRelay, computeUnitsToTokensMultiplier)) + logger.Info(fmt.Sprintf("About to start settling claim for %d compute units with ComputeUnitsPerRelay %d and ComputeUnitsToTokensMultiplier %d", claimComputeUnits, computeUnitsPerRelay, computeUnitsToTokensMultiplier))
|
|
||
| // Create a service that can be registered in the application and used in the claims | ||
| service := sharedtypes.NewService( | ||
| "svc1", | ||
| "svcName1", | ||
| 1, | ||
| ) |
There was a problem hiding this comment.
Reminder: Add test coverage for handling app going into debt.
The test is currently skipped with a TODO comment. Ensure to add coverage for this scenario.
Do you want me to open a GitHub issue to track this task?
|
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 GCP workloads (requires changing the namespace to 552) |
github-actions
bot
added
devnet
push-image
Summary
compute_units_per_relayparam to new servicesSettleSessionAccounting()to compute units per relay along with the general compute units to tokens multipliersIssue
Type of change
Select one or more:
Testing
Documentation changes (only if making doc changes)
make docusaurus_start; only needed if you make doc changesLocal Testing (only if making code changes)
make go_develop_and_testmake test_e2ePR Testing (only if making code changes)
devnet-test-e2elabel to the PR.make trigger_ciif you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
New Features
ComputeUnitsPerRelayfield to services, enabling configuration of compute units required per relay.AddServicecommand to allow specifying compute units per relay.Bug Fixes
Tests