YNU-457: skip empty assets in app allocations validation#416
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds multiple tests covering allocations referencing withdrawn or never-deposited assets and updates allocation validation to treat zero-balance assets specially: if an asset's app-session balance is zero, a non-zero allocation for that asset triggers an error; otherwise validation for that asset is skipped. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC
participant verifyAllocations as verifyAllocations()
Note over RPC,verifyAllocations: SubmitAppState -> verifyAllocations
Client->>RPC: SubmitAppState(with allocations)
RPC->>verifyAllocations: iterate assets in appSessionBalance
alt asset balance == 0
verifyAllocations->>verifyAllocations: check allocationSum[asset]
alt allocationSum[asset] != 0
verifyAllocations-->>RPC: return error "asset not deposited"
else allocationSum[asset] == 0
verifyAllocations-->>RPC: continue (skip validation)
end
else asset balance != 0
verifyAllocations->>verifyAllocations: require allocationSum[asset] == allocated amount
alt mismatch
verifyAllocations-->>RPC: return redistribution error
else match
verifyAllocations-->>RPC: ok
end
end
RPC-->>Client: response (success or error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-11-04T19:14:48.928ZApplied to files:
🧬 Code graph analysis (2)clearnode/app_session_service_test.go (3)
clearnode/rpc_router_private.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @dimast-x, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where the application's state validation incorrectly enforced allocation requirements for assets that had been fully withdrawn from a session. By introducing a check to skip zero-balance assets during the allocation verification process, the system now behaves as expected, allowing operations on remaining assets without being hindered by historical, zero-balance assets. This enhances the flexibility and correctness of app state management. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Summary of ChangesHello @dimast-x, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where the application's allocation validation incorrectly required allocations for assets that had been fully withdrawn and held a zero balance. The change introduces a modification to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where operations on an app session would incorrectly fail if an asset with a zero balance was not included in the allocations. The change in verifyAllocations correctly skips validation for such assets. A new test case, Operate_WithdrawnAsset_NotRequired, has been added to verify this fix, which is a great addition. My review includes a couple of suggestions to improve the robustness of the new test by handling potential errors when fetching balances.
There was a problem hiding this comment.
Code Review
This pull request addresses a bug in the application session state validation. Previously, the validation logic incorrectly required allocations for assets with a zero balance, which caused failures for assets that had been fully withdrawn from a session. The fix correctly skips zero-balance assets during validation. A new test case has been added to confirm that operations on assets with positive balances can proceed without needing to specify allocations for zero-balance assets. The change is logical, well-targeted, and properly tested.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clearnode/app_session_service_test.go(1 hunks)clearnode/rpc_router_private.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
clearnode/app_session_service_test.go (3)
clearnode/pkg/rpc/api.go (4)
VersionNitroRPCv0_4(33-33)AppSessionIntentOperate(640-640)Version(27-27)AppAllocation(626-633)clearnode/ledger.go (2)
NewAccountID(44-50)GetWalletLedger(56-58)clearnode/rpc_router_private.go (2)
SubmitAppStateParams(48-54)AppAllocation(62-66)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
clearnode/app_session_service_test.go (1)
486-532: Test coverage: add case for zero-balance asset with non-zero allocation.The test validates that zero-balance assets can be omitted from allocations (happy path). However, it doesn't cover the edge case where a zero-balance asset is included with a non-zero allocation, which should fail validation.
Consider adding a test case like:
t.Run("Operate_WithdrawnAsset_NonZeroAllocation_Error", func(t *testing.T) { db, cleanup := setupTestDB(t) defer cleanup() service := createTestAppSessionService(db, nil) session := createTestAppSession(t, db, "test-session-invalid-alloc", rpc.VersionNitroRPCv0_4, []string{userAddressA.Hex(), userAddressB.Hex()}, []int64{1, 1}, 2) sessionAccountID := NewAccountID(session.SessionID) // USDC deposited then fully withdrawn (balance = 0) setupAppSessionBalances(t, db, sessionAccountID, map[common.Address]map[string]int{ userAddressA: {"usdc": 100}, }) require.NoError(t, GetWalletLedger(db, userAddressA).Record(sessionAccountID, "usdc", decimal.NewFromInt(-100), nil)) // Try to operate with non-zero USDC allocation despite zero balance params := &SubmitAppStateParams{ AppSessionID: session.SessionID, Intent: rpc.AppSessionIntentOperate, Version: 2, Allocations: []AppAllocation{ {Participant: userAddressA.Hex(), AssetSymbol: "usdc", Amount: decimal.NewFromInt(50)}, {Participant: userAddressB.Hex(), AssetSymbol: "usdc", Amount: decimal.NewFromInt(50)}, }, } _, err := service.SubmitAppState(context.Background(), params, rpcSigners(userAddressA, userAddressB), rpcSigners(userAddressA, userAddressB)) require.Error(t, err) assert.Contains(t, err.Error(), "zero balance but non-zero allocation") })This will verify that the validation gap (flagged in
rpc_router_private.go) is properly addressed.
dimast-x
changed the title
dimast-x
changed the title
4 participants
Summary by CodeRabbit