chore: remove legacy CJS files, fix env defaults, and improve type annotations#63
Merged
chore: remove legacy CJS files, fix env defaults, and improve type annotations#63
Conversation
?? only falls back on null/undefined, so an empty env var (e.g. DATA_CONTRACT_ID='') would be used as-is instead of the intended default.
📝 WalkthroughWalkthroughThe PR deletes three JavaScript identity example scripts, replaces multiple occurrences of the nullish coalescing operator ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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: 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 `@1-Identities-and-Names/identity-update-disable-key.mjs`:
- Line 8: The constant DISABLE_KEY_ID currently uses || which treats "0" as
falsy; change it to use the nullish coalescing operator (??) so a user-provided
"0" is preserved. Update the DISABLE_KEY_ID initialization to coalesce only when
process.env.DISABLE_KEY_ID is null/undefined (e.g., use
process.env.DISABLE_KEY_ID ?? fallback) and then convert to Number, ensuring
DISABLE_KEY_ID preserves 0 instead of defaulting to 99.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef9a3d01-dce6-4584-b949-2623d019b776
📒 Files selected for processing (18)
1-Identities-and-Names/identity-retrieve-account-ids.js1-Identities-and-Names/identity-transfer-credits.mjs1-Identities-and-Names/identity-update-add-key.js1-Identities-and-Names/identity-update-disable-key.js1-Identities-and-Names/identity-update-disable-key.mjs1-Identities-and-Names/identity-withdraw-credits.mjs1-Identities-and-Names/name-register.mjs2-Contracts-and-Documents/contract-retrieve-history.mjs2-Contracts-and-Documents/contract-retrieve.mjs2-Contracts-and-Documents/contract-update-history.mjs2-Contracts-and-Documents/contract-update-minimal.mjs2-Contracts-and-Documents/document-delete.mjs2-Contracts-and-Documents/document-retrieve.mjs2-Contracts-and-Documents/document-submit.mjs2-Contracts-and-Documents/document-update.mjsREADME.mdpackage.jsontest/setupDashClient.test.mjs
💤 Files with no reviewable changes (3)
- 1-Identities-and-Names/identity-update-disable-key.js
- 1-Identities-and-Names/identity-update-add-key.js
- 1-Identities-and-Names/identity-retrieve-account-ids.js
#64) * chore: fix tsconfig to check setupDashClient with correct ESM settings Update tsconfig.json to use module/moduleResolution node16 for proper .mjs ESM support, scope include to setupDashClient.mjs only, and set maxNodeModuleJsDepth to 0. Add @types/node and @types/mocha devDeps. Add JSDoc type annotations to setupDashClient.mjs to pass strict tsc check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: replace any types with precise SDK types in setupDashClient Add JSDoc typedef imports for Identity, IdentityPublicKey, PlatformAddress, PlatformAddressInfo, and NetworkLike. Define DerivedKeyEntry and AddressEntry typedefs. Replace all any params/returns with real SDK types. Add missing @returns to create(), convenience signer methods, and exported functions. Add identity-not-found guard in getSigner() with corresponding test. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
thephez
changed the title
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 (1)
setupDashClient.mjs (1)
380-404:⚠️ Potential issue | 🟡 MinorFail fast when the expected identity public key is absent.
At Line 401,
identity.getPublicKeyById(key.keyId)can returnundefined, but execution continues and returns a signer anyway. This pushes errors downstream with poorer diagnostics.💡 Suggested fix
const identityKey = identity.getPublicKeyById(key.keyId); + if (!identityKey) { + throw new Error( + `Identity "${this.id}" does not contain keyId=${key.keyId} (${keyName}).`, + ); + } const signer = new IdentitySigner(); signer.addKeyFromWif(key.privateKeyWif); return { identity, identityKey, signer };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setupDashClient.mjs` around lines 380 - 404, In getSigner, check the result of identity.getPublicKeyById(key.keyId) and throw a clear error if it is undefined so we fail fast; locate getSigner, the identity variable from this.sdk.identities.fetch(this.id), and the key entry (keyName/key.keyId) and replace the current logic that returns a signer when identityKey is missing with a throw including identity id, keyName and keyId to provide actionable diagnostics before creating the IdentitySigner.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tsconfig.json`:
- Line 74: The tsconfig's "include" was narrowed to only "setupDashClient.mjs",
which limits type-checking and checkJs/strict coverage to that single file;
restore a broader include pattern (e.g., revert to the previous globs or add
back the tutorial/source directories and file extensions such as "src/**/*.ts",
"tutorials/**/*.js", "*.ts", "*.js", or the original patterns) so TypeScript's
checkJs/strict rules run across the full codebase; update the "include" array
(the "include" property and the "setupDashClient.mjs" entry) to cover all
tutorial and source files instead of a single file.
---
Outside diff comments:
In `@setupDashClient.mjs`:
- Around line 380-404: In getSigner, check the result of
identity.getPublicKeyById(key.keyId) and throw a clear error if it is undefined
so we fail fast; locate getSigner, the identity variable from
this.sdk.identities.fetch(this.id), and the key entry (keyName/key.keyId) and
replace the current logic that returns a signer when identityKey is missing with
a throw including identity id, keyName and keyId to provide actionable
diagnostics before creating the IdentitySigner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1550b207-a06e-4994-9516-025caca70191
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsetupDashClient.mjstest/setupDashClient.test.mjstsconfig.json
🚧 Files skipped from review as they are similar to previous changes (2)
- test/setupDashClient.test.mjs
- package.json
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
.js) tutorial files that used the oldrequire()API (identity-retrieve-account-ids.js,identity-update-add-key.js,identity-update-disable-key.js). All tutorials now use ESM (.mjs) exclusively.??with||forprocess.envdefaults across all tutorial scripts. The nullish coalescing operator (??) does not handle empty strings (""), whichprocess.envreturns for unset-but-defined variables. Using||correctly falls back to the default in those cases.setupDashClient.mjs— added@typedef,@param, and@returnsannotations toIdentityKeyManager,AddressKeyManager,createClient(),setupDashClient(), and related helpers. Added a null-check afteridentities.fetch()with a corresponding test.tsconfig.jsonto usemodule: "node16"/moduleResolution: "node16"so that.mjsfiles are type-checked correctly. NarrowedincludetosetupDashClient.mjsand setmaxNodeModuleJsDepth: 0to avoid checkingnode_modules.package.json: fixedmainentry (connect.js→connect.mjs), upgraded formatter toprettier@3.8.1, added@types/mochaand@types/nodedev dependencies.README.mdand test file to fix line lengths and brace style.Summary by CodeRabbit
Chores
Refactor
Removed
Tests
Documentation