YNU-268: update manual rpc.Client test according to NitroRPCv0.4#380
YNU-268: update manual rpc.Client test according to NitroRPCv0.4#380
Conversation
Summary of ChangesHello @philanton, 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 significantly updates the manual RPC client test suite to align with the latest NitroRPCv0.4 specification. It involves migrating the test environment to a new UAT WebSocket RPC endpoint, refining existing v0.2 test cases, and introducing a suite of new tests to thoroughly validate the v0.4 protocol's session lifecycle, including various state submission intents (operate, deposit, withdraw) and their corresponding impact on asset allocations and balances. 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
|
|
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. WalkthroughReworks the manual client test to cover Nitro RPC v0.2 and v0.4 flows: updates WS endpoint and asset symbol, adds per-test allocation variants, expands table-driven cases (create/submit/close), and adds explicit payload hashing/signing plus per-step balance assertions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test Suite
participant C as RPC Client
participant R as Nitro RPC Server
participant B as Balance/Acct Service
rect rgba(220,235,255,0.35)
note over T,C: CreateAppSession (v0_2 / v0_4)
T->>T: build create payload, hash, sign
T->>C: CreateAppSession(request, versionFlag)
C->>R: RPC CreateAppSession
R-->>C: CreateAck (sessionID)
C-->>T: sessionID
T->>B: getBalances(pre / post-create)
end
rect rgba(220,255,220,0.35)
note over T,C: SubmitAppState (operate / deposit / withdraw)
T->>T: build state payload, hash, sign
T->>C: SubmitAppState(sessionID, state, allocations)
C->>R: RPC SubmitAppState
R-->>C: SubmitAck
C-->>T: ack
T->>B: getBalances(check deltas per action)
end
rect rgba(255,240,220,0.35)
note over T,C: CloseAppSession
T->>T: build close payload, hash, sign
T->>C: CloseAppSession(sessionID)
C->>R: RPC CloseAppSession
R-->>C: CloseAck
C-->>T: ack
T->>B: getBalances(final assertion)
end
note over T,B: Assertions per scenario: no-op / decrease / increase as expected
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Code Review
This pull request updates the manual RPC client test to support NitroRPC v0.4. It correctly renames existing tests to specify v0.2 and adds new sub-tests for v0.4 features like deposit, withdraw, and operate intents. The balance assertions have also been improved by moving them inside each sub-test. The changes are logical and well-structured. I've added a few suggestions to improve naming and code clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
clearnode/pkg/rpc/client_manual_test.go (2)
44-47: Avoid blocking the client's error path: makehandleErrornon‑blocking or just logCurrent code can deadlock if multiple errors occur; the buffered chan (size 1) is never drained.
Apply one of these minimal fixes:
Option A — log directly (simplest):
- errCh := make(chan error, 1) - handleError := func(err error) { - errCh <- err - } + handleError := func(err error) { + t.Logf("client error: %v", err) +}Option B — keep the chan but prevent blocking:
- errCh := make(chan error, 1) - handleError := func(err error) { - errCh <- err - } + errCh := make(chan error, 16) + handleError := func(err error) { + select { + case errCh <- err: + default: + t.Logf("client error (dropped, buffer full): %v", err) + } + } + go func() { + for err := range errCh { + t.Logf("client error: %v", err) + } + }()
169-181: Compile-time bug:uuid.New().ID()does not exist; generateNoncefrom UUID bytes
github.com/google/uuidexposes noID()method. Use the first 8 bytes of a v4 UUID.Apply the following diffs:
CreateAppSession v0_2:
- Nonce: uint64(uuid.New().ID()), + Nonce: binary.BigEndian.Uint64(uuid.New()[:8]),CreateAppSession v0_4:
- Nonce: uint64(uuid.New().ID()), + Nonce: binary.BigEndian.Uint64(uuid.New()[:8]),Also add the import:
// add to the import block import ( // ... "encoding/binary" // ... )Also applies to: 293-306
🧹 Nitpick comments (2)
clearnode/pkg/rpc/client_manual_test.go (2)
94-96: Wait for first balance update to avoid flaky readsReads of
currentBalancecan occur before any event arrives after reconnect.Apply this after Start:
err = client.Start(ctx, uatWsRpcUrl, handleError) require.NoError(t, err) + + // Wait until we get the first balance update for the tracked asset + require.Eventually(t, func() bool { + currentBalanceMu.RLock() + ok := currentBalance.Cmp(decimal.Zero) >= 0 + currentBalanceMu.RUnlock() + return ok + }, 10*time.Second, 200*time.Millisecond, "no balance update received for %s", assetSymbol)Add import:
import ( // ... "time" )
20-22: Optional: make WS URL and asset configurable via env for different envsHardcoded values limit reuse across environments.
Example:
-const ( - uatWsRpcUrl = "wss://canarynet.yellow.com/ws" -) +var uatWsRpcUrl = getenvDefault("TEST_WS_URL", "wss://canarynet.yellow.com/ws") + +func getenvDefault(k, def string) string { + if v := os.Getenv(k); v != "" { + return v + } + return def +}Similarly, allow overriding:
- assetSymbol := "ytest.usd" + if v := os.Getenv("TEST_ASSET_SYMBOL"); v != "" { + assetSymbol = v + } else { + assetSymbol = "ytest.usd" + }Also applies to: 78-78
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clearnode/pkg/rpc/client_manual_test.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
clearnode/pkg/rpc/client_manual_test.go (4)
clearnode/pkg/rpc/api.go (13)
AppAllocation(582-589)SubmitAppStateRequest(388-401)CloseAppSessionRequest(407-414)CreateAppSessionRequest(375-382)AppDefinition(538-551)VersionNitroRPCv0_4(33-33)CreateAppSessionMethod(102-102)AppSessionIntentOperate(596-596)Version(27-27)SubmitAppStateMethod(104-104)AppSessionIntentDeposit(598-598)AppSessionIntentWithdraw(600-600)CloseAppSessionMethod(106-106)clearnode/pkg/sign/eth_signer.go (1)
NewEthereumAddress(24-26)clearnode/nitrolite/signature.go (1)
Sign(57-78)clearnode/pkg/rpc/message.go (1)
NewRequest(42-47)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
clearnode/pkg/rpc/client_manual_test.go (1)
98-118: Consider consolidating identical allocation sets.
appAllocationsV0_2andappAllocationsV0_4_Originalare identical, which may cause confusion. Consider one of these approaches:
- Use a single shared allocation variable if the protocol version doesn't affect initial allocation structure
- Add comments explaining why separate variables are needed for different protocol versions
Example consolidation:
- appAllocationsV0_2 := []rpc.AppAllocation{ - { - ParticipantWallet: walletSigner.PublicKey().Address().String(), - AssetSymbol: assetSymbol, - Amount: decimal.NewFromInt(1), - }, - } - appAllocationsV0_4_Original := []rpc.AppAllocation{ + // Initial allocation for both v0.2 and v0.4 session creation + appAllocationsInitial := []rpc.AppAllocation{ { ParticipantWallet: walletSigner.PublicKey().Address().String(), AssetSymbol: assetSymbol, Amount: decimal.NewFromInt(1), }, } + // Increased allocation for v0.4 deposit testing appAllocationsV0_4_Deposited := []rpc.AppAllocation{ { ParticipantWallet: walletSigner.PublicKey().Address().String(), AssetSymbol: assetSymbol, Amount: decimal.NewFromInt(2), }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
clearnode/pkg/rpc/client_manual_test.go(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
clearnode/pkg/rpc/client_manual_test.go (5)
clearnode/pkg/rpc/api.go (13)
AppAllocation(582-589)SubmitAppStateRequest(388-401)CloseAppSessionRequest(407-414)CreateAppSessionRequest(375-382)AppDefinition(538-551)VersionNitroRPCv0_4(33-33)CreateAppSessionMethod(102-102)AppSessionIntentOperate(596-596)Version(27-27)SubmitAppStateMethod(104-104)AppSessionIntentDeposit(598-598)AppSessionIntentWithdraw(600-600)CloseAppSessionMethod(106-106)clearnode/pkg/sign/sign.go (2)
PublicKey(22-25)Address(28-33)clearnode/pkg/sign/eth_signer.go (1)
NewEthereumAddress(24-26)clearnode/nitrolite/signature.go (1)
Sign(57-78)clearnode/pkg/rpc/message.go (1)
NewRequest(42-47)
🔇 Additional comments (7)
clearnode/pkg/rpc/client_manual_test.go (7)
21-21: LGTM! Environment and test asset updates are appropriate.The WebSocket URL change to canary and the test asset symbol update are correct for UAT testing.
Also applies to: 78-78
163-284: Verify balance update timing in tests.The balance assertions rely on
currentBalancebeing updated via the async event handler (lines 82-92) before the assertion runs. If the balance update event is delayed, tests could produce false positives or negatives.Consider adding explicit synchronization or polling with timeout:
// Helper function to wait for balance update func waitForBalanceChange(t *testing.T, mu *sync.RWMutex, current *decimal.Decimal, expected int64, timeout time.Duration) { deadline := time.Now().Add(timeout) for time.Now().Before(deadline) { mu.RLock() actual := current.IntPart() mu.RUnlock() if actual == expected { return } time.Sleep(10 * time.Millisecond) } t.Fatalf("balance did not update to expected value %d within timeout", expected) }Then use it in tests:
createAppRes, _, err := client.CreateAppSession(ctx, &createAppFullReq) require.NoError(t, err) fmt.Printf("App Session Created: %+v\n", createAppRes.AppSessionID) appSessionID = createAppRes.AppSessionID -currentBalanceMu.RLock() -balanceAfter := currentBalance.IntPart() -currentBalanceMu.RUnlock() - -balanceDiff := balanceBefore - balanceAfter -assert.Equal(t, int64(1), balanceDiff, "balance should decrease by 1 unit") +expectedBalance := balanceBefore - 1 +waitForBalanceChange(t, ¤tBalanceMu, ¤tBalance, expectedBalance, 5*time.Second)
287-332: LGTM! v0.4 session creation is correctly implemented.The test properly uses
VersionNitroRPCv0_4protocol and the balance tracking logic is consistent with the expected behavior (1 unit locked in the session).
335-373: LGTM! v0.4 operate intent is correctly implemented.The test properly includes the required v0.4 fields (
Intent,Version) and correctly expects no balance change for an operate action.
376-412: LGTM! v0.4 deposit flow is correctly implemented.The test properly demonstrates deposit behavior: allocation increases from 1 to 2 units, and the ledger balance decreases by 1 unit accordingly.
415-451: LGTM! v0.4 withdraw flow is correctly implemented.The test properly demonstrates withdraw behavior: allocation decreases from 2 to 1 units, and the ledger balance increases by 1 unit accordingly.
454-488: LGTM! Test case naming issue resolved.The test case name is now correct (
CloseAppSession_v0_4), addressing the previous review feedback. The implementation properly closes the v0.4 session with the expected balance increase.
Summary by CodeRabbit