[TODOs] add EventApplicationOverserviced event
#669
Conversation
bryanchriswhite
added
on-chain
WalkthroughThe changes introduce a new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
x/tokenomics/types/event.pb.gois excluded by!**/*.pb.go
Files selected for processing (4)
- api/poktroll/tokenomics/event.pulsar.go (6 hunks)
- proto/poktroll/tokenomics/event.proto (2 hunks)
- x/tokenomics/keeper/settle_session_accounting.go (10 hunks)
- x/tokenomics/types/errors.go (1 hunks)
Files not summarized due to errors (1)
- api/poktroll/tokenomics/event.pulsar.go: Error: Message exceeds token limit
Additional comments not posted (21)
proto/poktroll/tokenomics/event.proto (2)
7-7: Import Addition: Ensure the necessity and usage.The import for
cosmos/base/v1beta1/coin.protoseems necessary for the new message. Verify that all fields using this import are correctly defined.
39-45: New Message:EventApplicationOverservicedThe new message
EventApplicationOverservicedis correctly defined and includes fields forapplication_addr,expected_burn, andeffective_burn. Ensure that all new fields are correctly populated and used in the codebase.x/tokenomics/types/errors.go (1)
28-28: New Error:ErrTokenomicsApplicationOverservicedThe new error
ErrTokenomicsApplicationOverservicedis correctly defined with code 1121. Ensure that this error is properly used in the codebase.x/tokenomics/keeper/settle_session_accounting.go (16)
7-8: Import Alias Renaming: Ensure clarity and consistency.The import alias renaming from
sdktocosmostypesandtokenomics/typestotokenomicstypesimproves clarity. Ensure that all references are updated accordingly.
32-32: Initialization ofsettlementAmt: Ensure correctness.The initialization of
settlementAmtusingcosmostypes.NewCoinis appropriate. Ensure that this variable is correctly used throughout the function.
44-44: Error Handling: Nil ClaimThe error handling for a nil claim is appropriate. Ensure that this error is correctly propagated and handled by the caller.
51-51: Error Handling: Nil Session HeaderThe error handling for a nil session header is appropriate. Ensure that this error is correctly propagated and handled by the caller.
55-55: Error Handling: Invalid Session HeaderThe error handling for an invalid session header is appropriate. Ensure that this error is correctly propagated and handled by the caller.
58-60: Error Handling: Invalid Supplier AddressThe error handling for an invalid supplier address is appropriate. Ensure that this error is correctly propagated and handled by the caller.
63-65: Error Handling: Invalid Application AddressThe error handling for an invalid application address is appropriate. Ensure that this error is correctly propagated and handled by the caller.
75-75: Error Handling: Invalid Root HashThe error handling for an invalid root hash is appropriate. Ensure that this error is correctly propagated and handled by the caller.
93-93: Error Handling: Application Not FoundThe error handling for an application not found is appropriate. Ensure that this error is correctly propagated and handled by the caller.
100-100: Initialization ofsettlementAmtuPOKT: Ensure correctness.The initialization of
settlementAmtuPOKTusingcosmostypes.NewCoinsis appropriate. Ensure that this variable is correctly used throughout the function.
119-119: Error Handling: Mint Coins FailureThe error handling for a failure to mint coins is appropriate. Ensure that this error is correctly propagated and handled by the caller.
132-132: Error Handling: Send Coins FailureThe error handling for a failure to send coins is appropriate. Ensure that this error is correctly propagated and handled by the caller.
Line range hint
142-168:
New Logic: Handle Application OverservicedThe new logic to handle the case when an application is overserviced is appropriate. Ensure that the
EventApplicationOverservicedevent is correctly emitted and the error is correctly propagated and handled by the caller.
176-176: Error Handling: Burn Coins FailureThe error handling for a failure to burn coins is appropriate. Ensure that this error is correctly propagated and handled by the caller.
183-183: Error Handling: Negative Application StakeThe error handling for a negative application stake is appropriate. Ensure that this error is correctly propagated and handled by the caller.
193-198: Function Signature Change: Ensure CorrectnessThe function signature change of
getCoinFromComputeUnitsto returncosmostypes.Coinis appropriate. Ensure that this change is correctly reflected in all function calls.api/poktroll/tokenomics/event.pulsar.go (2)
5-5: Import looks good!The new import statement for
cosmossdk.io/api/cosmos/base/v1beta1is necessary for the newEventApplicationOverservicedmessage.
2547-2557: New messageEventApplicationOverservicedlooks good!The new message
EventApplicationOverservicedwith fieldsapplication_addr,expected_burn, andeffective_burnis correctly defined.
bryanchriswhite
added
push-image
|
The image is going to be pushed after the next commit. You can use If you also want to run E2E tests, please add |
|
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 669) |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/tokenomics/keeper/settle_session_accounting.go (10 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/tokenomics/keeper/settle_session_accounting.go
…ation-overserviced * pokt/main: [TODOs] refactor: proof path calculation (#659) [Dependencies] bump go-getter and ibc-go (#691) [Relayminer] refactor: `relayerSessionsManager#waitForBlock()` (#648) [Observables] chore: add `ReplayObservable#SubscribeFromLatestBufferedOffset()` (#647) [Observability] Add claim relays counter (#644) [Code Health] chore: log unused error when updating relay mining difficulty (#683) [Testing] chore: uncomment proof CLI query tests (#668) build(deps): bump ws from 7.5.9 to 7.5.10 in /docusaurus (#686) build(deps): bump webpack-dev-middleware from 5.3.3 to 5.3.4 in /docusaurus (#688) build(deps): bump express from 4.18.2 to 4.19.2 in /docusaurus (#687) build(deps): bump follow-redirects from 1.15.3 to 1.15.6 in /docusaurus (#685) build(deps): bump braces from 3.0.2 to 3.0.3 in /docusaurus (#689) [CosmosSDK] Bump to v0.50.7 (#682)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/tokenomics/keeper/settle_session_accounting.go (10 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/tokenomics/keeper/settle_session_accounting.go
bryanchriswhite
removed
push-image
…ation-overserviced * pokt/main: [TODO] Blockers @okdas (#674) [TODO_BLOCKER] chore: Remove stale TODO_BLOCKERs (#694) [TODO] refactor: query expiring claims w/ index (#671) [TODOs] `grace_period_end_offset_blocks` validation & cleanup (#667) [Testing] add account balance coverage to account settlement (#670) [Application] Auto-undelegation of unstaked gateways (#692) [RelayMiner] Fix LeanClient mutiple suppliers support (#662)
bryanchriswhite
added
push-image
|
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 669) |
|
The image is going to be pushed after the next commit. You can use If you also want to run E2E tests, please add |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- api/poktroll/tokenomics/event.pulsar.go (6 hunks)
- x/tokenomics/keeper/settle_session_accounting.go (6 hunks)
- x/tokenomics/keeper/settle_session_accounting_test.go (2 hunks)
Files not summarized due to errors (1)
- api/poktroll/tokenomics/event.pulsar.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (1)
- x/tokenomics/keeper/settle_session_accounting.go
Additional comments not posted (16)
x/tokenomics/keeper/settle_session_accounting_test.go (2)
10-10: Added JSON package import.The addition of the
jsonpackage is presumably for handling JSON operations in the tests, particularly for unmarshalling event attributes. Ensure that this package is used appropriately and securely, especially in handling untrusted input, although in a test environment this is usually controlled.
256-273: Review of event attribute checks and coin unmarshalling inTestSettleSessionAccounting_AppStakeTooLow.
Context Unwrapping: Line 256 unwraps the SDK context correctly which is a common pattern in Cosmos SDK tests.
Event Retrieval: Lines 257-261 handle the retrieval of events and their attributes. It's important to validate that these attributes are expected to be present in the events to avoid potential runtime errors from missing attributes.
Attribute Unmarshalling: Lines 267-270 handle the unmarshalling of the
expectedBurnandeffectiveBurnattributes intoCointypes. This is crucial for ensuring that the events contain valid and correctly formatted data. The use of JSON for serialization/deserialization in events is standard, but ensure that the data format matches the expected schema.Assertions: Lines 263, 272-273 use assertions to check the lengths of attributes and compare the coins. These assertions are well-placed to ensure the integrity of the test conditions.
Overall, the changes are well-integrated and appear to follow best practices for handling events in Cosmos SDK tests. However, ensure that the event attributes are always set as expected to avoid potential issues during runtime.
Would you like to add more robust error handling or fallbacks for missing event attributes?
Verification successful
Event attribute checks and coin unmarshalling in
TestSettleSessionAccounting_AppStakeTooLoware valid.The attributes
application_addr,expected_burn, andeffective_burnare defined and used correctly in the codebase, particularly in theEventApplicationOverservicedevent. This confirms that the attributes are set during the operations leading to the test, addressing the concern about potential runtime errors from missing attributes.
x/tokenomics/types/event.pb.go: Attributes are defined in theEventApplicationOverservicedevent.x/tokenomics/keeper/settle_session_accounting_test.go: Attributes are used in the test.Scripts executed
The following scripts were executed for the analysis:
Script:
# Check if the event attributes are always set during the operations leading to this test. rg --type go -A 5 $'events.EmitEvent'Length of output: 36
Script:
#!/bin/bash # Searching for event emission patterns in the codebase rg --type go 'EventManager().EmitEvent' -A 5Length of output: 46
Script:
#!/bin/bash # Searching for any event emission methods in the codebase rg --type go 'Emit' -A 5Length of output: 11197
Script:
#!/bin/bash # Searching for specific event attributes in the codebase rg --type go 'application_addr' -A 5 rg --type go 'expected_burn' -A 5 rg --type go 'effective_burn' -A 5Length of output: 38140
api/poktroll/tokenomics/event.pulsar.go (14)
1772-1785: Initialization ofEventApplicationOverservicedThe initialization function for
EventApplicationOverservicedis correctly setting up the message descriptor and field descriptors. Ensure that the message and field names match the protobuf definitions exactly.
1787-1787: Review ofProtoMessageandInterfacemethodsThe
ProtoMessageandInterfacemethods are standard for protobuf message types in Go. They are implemented correctly here.Also applies to: 1840-1844
1846-1870: Field Iteration Logic inRangeMethodThe
Rangemethod iterates over fields correctly and uses the provided function to handle each field. This is a standard pattern in Go protobuf implementations.
1872-1897: Field Presence Check inHasMethodThe
Hasmethod correctly checks for field presence based on the field's full name. It handles nullable and non-nullable fields appropriately, which is crucial for correct serialization and deserialization behavior.
1899-1919: Clear Method ImplementationThe
Clearmethod is implemented to reset fields to their zero values. This method is crucial for reusing or resetting protobuf messages in long-lived applications to avoid stale data.
1921-1944: Field Retrieval inGetMethodThe
Getmethod is implemented to retrieve field values safely. It handles both populated and unpopulated fields correctly, returning a default value for unpopulated fields. This is a critical method for robust data handling in applications.
1946-1970: Field Setting inSetMethodThe
Setmethod correctly assigns values to fields, including handling oneof fields and extensions. It's implemented to ensure thread safety, which is essential in concurrent environments.
1972-2002: Mutable Reference Retrieval inMutableMethodThe
Mutablemethod is correctly implemented to provide mutable references to composite fields. It ensures that a new value is allocated if the field is unpopulated, which is crucial for modifying repeated fields or nested messages.
2004-2023: Field Creation inNewFieldMethodThe
NewFieldmethod is implemented to create new instances of fields, ensuring that the correct type is instantiated for complex fields like messages. This method is essential for dynamic message construction.
2025-2034: Oneof Handling inWhichOneofMethodThe
WhichOneofmethod is implemented to identify which field in a oneof group is populated. This method is crucial for correctly interpreting messages that use oneof to represent optional or mutually exclusive fields.
2036-2052: Unknown Field Handling inSetUnknownMethodThe
SetUnknownmethod is implemented to manage unknown fields in a message, allowing for forward compatibility by preserving unknown fields during deserialization. This is a best practice in protocol buffer implementations.
2054-2062: Message Validity Check inIsValidMethodThe
IsValidmethod is implemented to check if the message is a valid, non-nil instance. This is crucial for preventing runtime errors when working with potentially nil messages.
2066-2348: Protobuf Method Implementations inProtoMethodsThe
ProtoMethodsprovides custom implementations for serialization, deserialization, and size calculation. These are optimized for performance and correctness, ensuring that the message handles data efficiently and correctly across different protobuf versions.
5-5: Ensure correct import forv1beta1The import alias
v1beta1forcosmossdk.io/api/cosmos/base/v1beta1is used in the file. Confirm that this is the correct version and alias as per project standards.Verification successful
Verified: Correct import alias for
v1beta1The import alias
v1beta1forcosmossdk.io/api/cosmos/base/v1beta1is used consistently across multiple files in the project, indicating that it is the standard alias. Therefore, the import inevent.pulsar.gois correct.
api/poktroll/tokenomics/event.pulsar.goline 5Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the correct version and alias are used for the cosmos SDK. grep -r "cosmossdk.io/api/cosmos/base/v1beta1" .Length of output: 1170
| @@ -29,40 +29,40 @@ func (k Keeper) SettleSessionAccounting( | |||
| ) error { | |||
| logger := k.Logger().With("method", "SettleSessionAccounting") | |||
|
|
|||
| settlementAmt := sdk.NewCoin("upokt", math.NewInt(0)) | |||
| settlementCoin := cosmostypes.NewCoin("upokt", math.NewInt(0)) | |||
There was a problem hiding this comment.
Thoughts on pluralizing this to Coins?
There was a problem hiding this comment.
settlementCoins is used elsewhere and is a []cosmostypes.Coin type.
Co-authored-by: Daniel Olshansky <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/tokenomics/keeper/settle_session_accounting.go (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/tokenomics/keeper/settle_session_accounting.go
Summary
Add
EventApplicationOverservicedwhen application stake is less than the settlement amount so that we can monitor occurrences of this scenario and compute the effective inflation.Issue
While the TODO asks to "unstake" the application, such an action isn't precisely defined. When using the CLI, "unstaking" has the effect of moving staked tokens to the application address and then removing the application from the state. In the current context, there is no stake to move, leaving "unstake" to mean to remove the application from the state store.
Doing this automatically as the result of an action other than explicitly unstaking seems likely to me to introduce confusion. For example, imagine an application querying the network to check their staked balance. If we remove the application at the end of a session where the application was overserviced, such queries would return an app not found error. This also has the effect of wiping out information about the application's on-chain service configuration, which may be useful as an intent signal.
Once #612 is implemented, unintentional application overservicing will be mitigated. An attack vector remains which is not trivially addressed; "unstaking" overserviced application has a negligible impact on this while coming with the aforementioned trade-offs.
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
EventApplicationOverservicedmessage to track application over-service events, including expected and effective burns.Refactor
Tests