test: add setupDashClient unit tests and mnemonic validation#61
Merged
test: add setupDashClient unit tests and mnemonic validation#61
Conversation
Replace hardcoded DIP-13 path construction in deriveKeysFromMnemonic with the exported dip13KeyPath() helper, ensuring a single source of truth for path derivation. Add test cases for missing setupDashClient() permutations including undefined manager guards, requireIdentity flag, and identityIndex forwarding. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Catch invalid BIP39 mnemonics early with a clear error message instead of letting them propagate to the WASM SDK as opaque WasmSdkError objects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verify key-role correctness (keyId per getSigner method), signer depth (keyCount for getMaster/getFullSigner), createClient success branches (testnet/mainnet isConnected), findNextIndex loop logic with stubbed occupied indices, createForNewIdentity nonzero auto-index, and AddressKeyManager getInfo/getInfoAt with stubbed SDK. Fix stale comment and remove weak clientConfig direct-usage tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Don't run setup in test:all
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds runtime validation for mnemonic inputs in setupDashClient and introduces a comprehensive test suite for identity and address key management operations, with supporting dependency updates to enable the new testing infrastructure. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
package.json (1)
12-13:test:alldoesn’t include the new setup suite.If your CI treats
test:allas the full gate,test/setupDashClient.test.mjswon’t run. Consider chainingtest:setup(or renamingtest:allto reflect current scope).♻️ Suggested script update
- "test:all": "node --test --test-timeout=300000 --test-concurrency=1 test/read-only.test.mjs test/read-write.test.mjs", + "test:all": "node --test --test-timeout=300000 --test-concurrency=1 test/read-only.test.mjs test/read-write.test.mjs && npm run test:setup",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 12 - 13, The test:all npm script currently runs only read-only and read-write suites and omits the setup suite (script names "test:all" and "test:setup"), so update package.json to include the setup test in the test:all command or rename test:all to reflect scope; specifically, modify the "test:all" script to also invoke test/setupDashClient.test.mjs (or chain the "test:setup" script within it) so CI runs the setup suite as part of the full test gate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/setupDashClient.test.mjs`:
- Around line 77-84: The test suite bootstraps a real SDK client in the before
hook (createClient, network) which couples the IdentityKeyManager tests to live
network calls; refactor tests to stub or mock network interactions for unit
tests by replacing createClient/network-dependent calls with a mocked client or
injected fake implementation (e.g., a mocked SDK exposing the same methods used
by IdentityKeyManager), and move the real createClient usage behind an explicit
integration flag or separate describe block named "integration" so live-network
tests run only when opted-in.
- Around line 450-461: The test uses a real SDK and a mutable TEST_MNEMONIC to
assert "no on-chain identity", which is flaky; replace the dependency with a
deterministic fake/stub SDK that returns null for identity lookups (e.g.,
implement a minimal sdk mock where byPublicKeyHash or the identity resolution
method returns null) and pass that mock into IdentityKeyManager.create({ sdk,
mnemonic: TEST_MNEMONIC }) so the failure path is deterministic; apply the same
change to the similar test at lines 513-516 to stub the identity resolver rather
than relying on chain state.
- Around line 744-772: The test is skipped but documents a real crash when
clientConfig.mnemonic is null; fix by adding an explicit guard in
setupDashClient() that throws a clear Error when mnemonic is missing and ensure
returned objects (keyManager, addressKeyManager) are either valid helpers or
replaced with stubs that throw a clear Error for getAuth()/getSigner; update
setupDashClient() to check clientConfig.mnemonic early and throw a descriptive
Error (e.g., "mnemonic required to create keyManager") so the test can be
unskipped and assertions on keyManager.getAuth and addressKeyManager.getSigner
will receive an instanceOf Error with no raw TypeError.
---
Nitpick comments:
In `@package.json`:
- Around line 12-13: The test:all npm script currently runs only read-only and
read-write suites and omits the setup suite (script names "test:all" and
"test:setup"), so update package.json to include the setup test in the test:all
command or rename test:all to reflect scope; specifically, modify the "test:all"
script to also invoke test/setupDashClient.test.mjs (or chain the "test:setup"
script within it) so CI runs the setup suite as part of the full test gate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21c0f9e0-0aee-4413-a8f7-4933a3d4afe0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonsetupDashClient.mjstest/setupDashClient.test.mjs
Tutorials depend on dotenv to load .env files, so it should be a regular dependency to ensure it is available after `npm install`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fork PRs don't have access to repository secrets, so identity-retrieve fails in CI. Skip tests marked with requiresMnemonic when the env var is absent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
setupDashClientcovering client creation, key derivation, DIP-13 path logic, identity handling, and error paths (~920 lines, 138 assertions)setupDashClientbefore key derivation, throwing a clear error for invalid BIP39 mnemonicstest:setupscript for running setupDashClient tests via MochaSummary by CodeRabbit
Release Notes
Chores
Bug Fixes
Tests