-
Notifications
You must be signed in to change notification settings - Fork 8
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
[Testing] add account balance coverage to account settlement #670
Conversation
WalkthroughThe recent changes introduce a 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 Configration File (
|
// Assert that `suppliertypes.ModuleName` account module balance is *unchanged* | ||
// Assert that `supplierAddress` account balance has *increased* by the appropriate amount | ||
// Assert that `supplierAddress` staked balance is *unchanged* | ||
// Assert that `apptypes.ModuleName` account module balance is *unchanged* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Reviewer, this assertion seems to be incorrect to me. My understanding is that the account module balance MUST decrease as a result of the burn. I assumed this is copy-pasta and have the test making the modified assertion.
988dc63
to
1ee53df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- testutil/keeper/tokenomics.go (2 hunks)
- x/tokenomics/keeper/settle_session_accounting_test.go (4 hunks)
- x/tokenomics/types/expected_keepers.go (3 hunks)
Additional comments not posted (15)
x/tokenomics/types/expected_keepers.go (3)
9-9
: Import Statement AddedThe import statement for
banktypes
has been added correctly and is necessary for the new method in theBankKeeper
interface.
58-62
: New SupplierKeeper InterfaceThe
SupplierKeeper
interface has been introduced withGetSupplier
andSetSupplier
methods. This addition appears correct and aligns with the PR's objective to add coverage for supplier account balances.
30-30
: New Method in BankKeeper InterfaceThe
Balance
method has been added to theBankKeeper
interface. Ensure the implementation of this method aligns with the expected functionality and handles errors appropriately.testutil/keeper/tokenomics.go (2)
60-60
: SupplierKeeper Added to TokenomicsModuleKeepersThe
SupplierKeeper
field is correctly added to theTokenomicsModuleKeepers
struct.
344-344
: SupplierKeeper InitializationThe
SupplierKeeper
is correctly initialized in theNewTokenomicsModuleKeepers
function.x/tokenomics/keeper/settle_session_accounting_test.go (10)
5-12
: Import Statements AddedThe new import statements for
context
,authtypes
, andbanktypes
are necessary for the updated test logic and are correctly added.
24-24
: Import Statement for suppliertypesThe import statement for
suppliertypes
is necessary for the updated test logic and is correctly added.
29-29
: TODO_MAINNET: Add CoverageThe TODO comment indicates that test coverage for handling app debt scenarios needs to be added. This is a reminder and does not affect the current functionality.
34-47
: Adding Application and Supplier for TestThe code correctly sets up a new application and supplier with their respective stakes for the test scenario.
70-109
: Test for Valid AccountingThe test function correctly sets up the context and initial conditions for testing valid accounting scenarios. The balance queries before accounting are appropriately done.
110-124
: Claim Setup for TestThe code correctly sets up a claim with the necessary session header and root hash for the test scenario.
126-127
: Error Handling in TestThe test correctly checks for no errors in the
SettleSessionAccounting
function call.
130-156
: Assertions for Valid AccountingThe assertions in the test function correctly verify the expected balances and stakes after the accounting process.
160-248
: Test for Low App StakeThe test function correctly sets up the context and initial conditions for testing scenarios where the application stake is too low. The balance queries and assertions are appropriately done.
456-474
: New getBalance FunctionThe
getBalance
function is introduced to retrieve account balances for testing. Ensure that this function is used consistently across all relevant test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments but this reflects well how it works.
|
||
// Add a new application | ||
appStake := cosmostypes.NewCoin("upokt", math.NewInt(1000000)) | ||
expectedAppEndStakeAmount := cosmostypes.NewCoin("upokt", math.NewInt(420)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we calculate this value or explain how we came up with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just an arbitrary amount which the expectedAppBurn
is defined in terms of. The idea is to ensure a non-zero stake balance that can be asserted at the end of the test. Defining this value first lets us define expectedAppBurn
in terms of it and appStake
, which I figured would be the simplest way to set things up.
require.True(t, appIsFound) | ||
require.Equal(t, &expectedAppEndStakeAmount, app.GetStake()) | ||
|
||
// Assert that `apptypes.ModuleName` account module balance is *decreased* by the appropriate amount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a comment on why app module balance decreases and not supplier module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Assert that `apptypes.ModuleName` account module balance is *decreased* by the appropriate amount
// NB: The application module account burns the amount of uPOKT that was held in escrow
// on behalf of the applications which were serviced in a given session.
// Assert that `suppliertypes.ModuleName` account module balance is *unchanged*
// NB: Supplier rewards are minted to the supplier module account but then immediately
// distributed to the supplier accounts which provided service in a given session.
…ent-balances * 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- x/tokenomics/keeper/settle_session_accounting_test.go (4 hunks)
Additional comments not posted (7)
x/tokenomics/keeper/settle_session_accounting_test.go (7)
29-47
: LGTM!The setup and assertions for the test look correct.
70-161
: LGTM!The setup and assertions for the test look correct. The function comprehensively tests the valid accounting scenarios.
165-253
: LGTM!The setup and assertions for the test look correct. The function comprehensively tests the scenario where the application stake is too low.
Line range hint
255-267
:
LGTM!The setup and assertions for the test look correct. The function comprehensively tests the scenario where the application is not found.
Line range hint
269-331
:
LGTM!The setup and assertions for the test look correct. The function comprehensively tests various scenarios with invalid root hashes.
Line range hint
333-413
:
LGTM!The setup and assertions for the test look correct. The function comprehensively tests various scenarios with invalid claims.
462-479
: LGTM!The helper function looks correct and is useful for the test cases.
tokenomicstypes "github.com/pokt-network/poktroll/x/tokenomics/types" | ||
) | ||
|
||
func TestSettleSessionAccounting_HandleAppGoingIntoDebt(t *testing.T) { | ||
t.Skip("TODO_MAINNET: Add coverage of the design choice made for how to handle this scenario.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder: Add coverage for the design choice.
The test is currently skipped with a TODO comment. Ensure to add the necessary test coverage for the design choice made for handling this scenario.
Do you want me to help with adding the test coverage or open a GitHub issue to track this task?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
…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)
…-root * 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)
Summary
Add coverage for the following balances to the account settlement tests:
Issue
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_test
make test_e2e
PR Testing (only if making code changes)
devnet-test-e2e
label to the PR.make trigger_ci
if you want to re-trigger tests without any code changesSanity Checklist
Summary by CodeRabbit
New Features
SupplierKeeper
to manage supplier interactions within the tokenomics module.Tests
getBalance
for retrieving account balances in tests.