refactor: split setupDashClient into browser-safe core and Node wrapper#68
Merged
refactor: split setupDashClient into browser-safe core and Node wrapper#68
Conversation
Extract createClient, IdentityKeyManager, AddressKeyManager, dip13KeyPath, and KEY_SPECS into setupDashClient-core.mjs so browser apps can import them without pulling in dotenv or process.env. setupDashClient.mjs keeps the Node-only convenience layer and re-exports the core API so existing tutorial imports continue to work unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a browser-safe core module Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client
participant IDM as IdentityKeyManager
participant KD as KeyDerivation (hexToBytes/dip13)
participant SDK as EvoSDK (on-chain)
Caller->>IDM: create / createForNewIdentity(mnemonic, network)
IDM->>KD: derive keys, call dip13KeyPath / hexToBytes
KD-->>IDM: derived pubkeys / bytes (or throw on invalid hex)
IDM->>SDK: query identity existence (findNextIndex / lookup)
SDK-->>IDM: identity found? / identity data
IDM-->>Caller: IdentityKeyManager instance / signer / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@setupDashClient-core.mjs`:
- Around line 64-77: The hexToBytes helper currently uses parseInt and substr
which can accept invalid inputs; update hexToBytes to strictly validate each
two-character chunk before conversion (e.g., ensure each chunk matches
/^[0-9A-Fa-f]{2}$/) and reject on any non-hex or odd-length input, then convert
validated pairs to bytes; also replace legacy substr usage with slice when
extracting chunks. Ensure errors reference hexToBytes so callers get clear
failure context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc818023-f6c7-413e-8b74-1dd48f0b1adf
📒 Files selected for processing (2)
setupDashClient-core.mjssetupDashClient.mjs
parseInt(chunk, 16) silently accepted inputs like "1z" as 1, so the
NaN guard let non-hex characters through and produced wrong bytes.
Validate each 2-char chunk against /^[0-9A-Fa-f]{2}$/ before
conversion and replace the deprecated substr with slice. Adds a
regression test that poisons publicKey with "1z" and asserts the
hexToBytes error references the bad offset.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/setupDashClient.test.mjs (1)
415-474: Regression test covers the intended path, but the 4 valid-key blocks are copy-pasted.The assertion (
/hexToBytes.*offset 2/) correctly targets the master key (first inKEY_SPECS) with its poisoned'1z'at byte offset 2, so the test is meaningful. The fourauthHigh/auth/transfer/encryptionentries duplicate the sameBuffer.from(PrivateKey.fromWIF(...).getPublicKey().toBytes()).toString('hex')boilerplate; sincegetKeysInCreationthrows on the first bad key, the other fourpublicKeyfields are never actually read — you could omit them entirely or use a tiny helper to keep the test focused. Optional.♻️ Optional simplification
- const km = new IdentityKeyManager( - sdk, - null, - { - master: { ...base.keys.master, publicKey: poisonedHex }, - authHigh: { - ...base.keys.authHigh, - publicKey: Buffer.from( - PrivateKey.fromWIF(base.keys.authHigh.privateKeyWif) - .getPublicKey() - .toBytes(), - ).toString('hex'), - }, - auth: { - ...base.keys.auth, - publicKey: Buffer.from( - PrivateKey.fromWIF(base.keys.auth.privateKeyWif) - .getPublicKey() - .toBytes(), - ).toString('hex'), - }, - transfer: { - ...base.keys.transfer, - publicKey: Buffer.from( - PrivateKey.fromWIF(base.keys.transfer.privateKeyWif) - .getPublicKey() - .toBytes(), - ).toString('hex'), - }, - encryption: { - ...base.keys.encryption, - publicKey: Buffer.from( - PrivateKey.fromWIF(base.keys.encryption.privateKeyWif) - .getPublicKey() - .toBytes(), - ).toString('hex'), - }, - }, - 0, - ); + const pubHex = (wif) => + Buffer.from(PrivateKey.fromWIF(wif).getPublicKey().toBytes()).toString( + 'hex', + ); + const km = new IdentityKeyManager( + sdk, + null, + { + master: { ...base.keys.master, publicKey: poisonedHex }, + authHigh: { ...base.keys.authHigh, publicKey: pubHex(base.keys.authHigh.privateKeyWif) }, + auth: { ...base.keys.auth, publicKey: pubHex(base.keys.auth.privateKeyWif) }, + transfer: { ...base.keys.transfer, publicKey: pubHex(base.keys.transfer.privateKeyWif) }, + encryption: { ...base.keys.encryption, publicKey: pubHex(base.keys.encryption.privateKeyWif) }, + }, + 0, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/setupDashClient.test.mjs` around lines 415 - 474, The test duplicates identical publicKey construction for authHigh/auth/transfer/encryption even though getKeysInCreation() will fail on the poisoned master key; simplify by removing those four heavy inline Buffer.from(PrivateKey.fromWIF(...).getPublicKey().toBytes()).toString('hex') blocks or replace them with a small helper/variable that computes a single valid publicKeyHex (e.g., validHex) and reuse it for authHigh/auth/transfer/encryption, keeping the poisonedHex only for master so the assertion against getKeysInCreation() and the /hexToBytes.*offset 2/ message stays focused and the test is easier to read.setupDashClient-core.mjs (2)
277-293:findNextIndexis an unboundedfor (;;)loop.If
sdk.identities.byPublicKeyHashever returned a truthy value for every index (e.g., a buggy SDK or exotic test stub), this would loop forever and block the event loop betweenawaits. Not a real-world hazard for tutorials against testnet, but a cheap safety net (e.g., cap at a few hundred indices and throw) would make the helper more robust as it’s now exposed from the browser-safe core too. Leaving as-is is also reasonable for an educational module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setupDashClient-core.mjs` around lines 277 - 293, The helper findNextIndex contains an unbounded for(;;) loop; add a safe iteration cap (e.g., MAX_INDEX = 1000 or configurable) and after that many attempts throw a clear Error indicating no free index found to avoid infinite looping; update the loop in findNextIndex to break/throw when i >= MAX_INDEX and include the MAX_INDEX constant and an explanatory error message referencing sdk.identities.byPublicKeyHash and dip13KeyPath so callers can detect the failure.
364-381: Minor: O(n²) lookup insidegetKeysInCreation.
Object.values(this.keys).find(...)runs on everyKEY_SPECSiteration. With 5 keys this is negligible, but sincethis.keysis already keyed by role (master/authHigh/auth/transfer/encryption) and the correspondingkeyIds are fixed, you could build aMap<keyId, entry>once, or keythis.keysbykeyIddirectly, and drop the.find. Purely a readability nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setupDashClient-core.mjs` around lines 364 - 381, getKeysInCreation performs an O(n²) lookup by calling Object.values(this.keys).find(...) for each KEY_SPECS entry; refactor to build a single lookup by keyId once and reuse it: create a Map or object keyed by key.keyId from this.keys before iterating KEY_SPECS (referencing getKeysInCreation, KEY_SPECS, this.keys, and keyId), then inside the KEY_SPECS.map replace the find with a direct lookup from that Map and keep the rest of the creation logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@setupDashClient-core.mjs`:
- Around line 277-293: The helper findNextIndex contains an unbounded for(;;)
loop; add a safe iteration cap (e.g., MAX_INDEX = 1000 or configurable) and
after that many attempts throw a clear Error indicating no free index found to
avoid infinite looping; update the loop in findNextIndex to break/throw when i
>= MAX_INDEX and include the MAX_INDEX constant and an explanatory error message
referencing sdk.identities.byPublicKeyHash and dip13KeyPath so callers can
detect the failure.
- Around line 364-381: getKeysInCreation performs an O(n²) lookup by calling
Object.values(this.keys).find(...) for each KEY_SPECS entry; refactor to build a
single lookup by keyId once and reuse it: create a Map or object keyed by
key.keyId from this.keys before iterating KEY_SPECS (referencing
getKeysInCreation, KEY_SPECS, this.keys, and keyId), then inside the
KEY_SPECS.map replace the find with a direct lookup from that Map and keep the
rest of the creation logic unchanged.
In `@test/setupDashClient.test.mjs`:
- Around line 415-474: The test duplicates identical publicKey construction for
authHigh/auth/transfer/encryption even though getKeysInCreation() will fail on
the poisoned master key; simplify by removing those four heavy inline
Buffer.from(PrivateKey.fromWIF(...).getPublicKey().toBytes()).toString('hex')
blocks or replace them with a small helper/variable that computes a single valid
publicKeyHex (e.g., validHex) and reuse it for
authHigh/auth/transfer/encryption, keeping the poisonedHex only for master so
the assertion against getKeysInCreation() and the /hexToBytes.*offset 2/ message
stays focused and the test is easier to read.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7f593c7-e8bb-439f-bb31-81b410d4d58b
📒 Files selected for processing (2)
setupDashClient-core.mjstest/setupDashClient.test.mjs
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.
setupDashClient.mjs—createClient,IdentityKeyManager,AddressKeyManager,dip13KeyPath, andKEY_SPECS— into a newsetupDashClient-core.mjsthat has no Node-only dependencies.setupDashClient.mjsas the Node convenience wrapper: it loads.envviadotenv, readsPLATFORM_MNEMONIC/NETWORKfromprocess.env, and exposes thesetupDashClient()one-call helper used by tutorials.setupDashClient.mjsso existing tutorial imports (import { IdentityKeyManager } from './setupDashClient.mjs') keep working with no changes.Buffer.from(hex, 'hex')with a smallhexToByteshelper in the core so browser apps can consumeIdentityKeyManagerwithout a Buffer polyfill.No behavior change — this is a pure reorganization to let browser apps (
docs/interactive runner, future in-browser tutorials) share the same key manager code as the Node tutorials instead of copy-pasting it.Summary by CodeRabbit
New Features
Bug Fixes
Tests