feat(js): add TypeScript SDK package (packages/js)#4
Conversation
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces a new ChangesSafHandle JS SDK
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Example as register.ts
participant Wallet as DirectSecp256k1HdWallet
participant SigningClient as SafHandleSigningClient
participant Validation as normalizeName
participant Chain as SigningCosmWasmClient
Example->>Wallet: fromMnemonic(SAFHANDLE_MNEMONIC)
Example->>SigningClient: connectWithSigner(rpcEndpoint, signer, contractAddress)
SigningClient->>Chain: SigningCosmWasmClient.connectWithSigner
Example->>SigningClient: registerName(name)
SigningClient->>Validation: normalizeName(name)
SigningClient->>SigningClient: nameFee() via getConfig
SigningClient->>Chain: execute(register_name, fee)
Chain-->>SigningClient: ExecuteResult (transactionHash)
SigningClient-->>Example: ExecuteResult
sequenceDiagram
participant Example as resolve.ts
participant Client as SafHandleClient
participant Chain as CosmWasmClient
Example->>Client: SafHandleClient.connect(rpcEndpoint, contractAddress)
Client->>Chain: CosmWasmClient.connect
Example->>Client: lookup(input)
Client->>Chain: queryContractSmart(get_address)
alt not found
Chain-->>Client: error
Client-->>Example: null
else found
Chain-->>Client: address
Client->>Chain: getAddress(input)
Chain-->>Client: record details
Client-->>Example: address + record
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/js/src/constants.ts (1)
13-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid duplicating the gas price literal.
SAFROCHAIN_TESTNET.gasPriceandDEFAULT_GAS_PRICEboth hardcode"0.025usaf"independently; if one changes, the other can silently drift out of sync.♻️ Proposed fix
-export const SAFROCHAIN_TESTNET: NetworkConfig = { - chainId: "safro-testnet-1", - rpcEndpoint: "https://rpc.testnet.safrochain.com", - addressPrefix: "addr_safro", - denom: "usaf", - gasPrice: "0.025usaf", - // SafHandle v1 (name-only) contract on safro-testnet-1, code_id 157. - // Source of truth: safhandle-contract/config/testnet.json. - contractAddress: "addr_safro1j6n2q333gy80pmpd6avss32y4nhd8deayv8m9x4uazt6zkdczk9sxjfun6", -}; - /** Default gas price used when constructing a signing client. */ export const DEFAULT_GAS_PRICE = "0.025usaf"; + +export const SAFROCHAIN_TESTNET: NetworkConfig = { + chainId: "safro-testnet-1", + rpcEndpoint: "https://rpc.testnet.safrochain.com", + addressPrefix: "addr_safro", + denom: "usaf", + gasPrice: DEFAULT_GAS_PRICE, + // SafHandle v1 (name-only) contract on safro-testnet-1, code_id 157. + // Source of truth: safhandle-contract/config/testnet.json. + contractAddress: "addr_safro1j6n2q333gy80pmpd6avss32y4nhd8deayv8m9x4uazt6zkdczk9sxjfun6", +};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/js/src/constants.ts` around lines 13 - 25, The gas price literal is duplicated between SAFROCHAIN_TESTNET.gasPrice and DEFAULT_GAS_PRICE, which can drift out of sync. Update the constants in constants.ts so there is a single source of truth for the gas price, and have both SAFROCHAIN_TESTNET and DEFAULT_GAS_PRICE reference that shared value instead of hardcoding the string twice. Use the SAFROCHAIN_TESTNET and DEFAULT_GAS_PRICE symbols to keep the change localized and easy to follow.packages/js/test/signing-client.test.ts (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnit tests import from
dist/instead ofsrc/, coupling them to a manual build step.Importing
SafHandleSigningClientfrom../dist/index.jsmeans these tests only work afternpm run build(pretestcoversnpm test, but nottest:watch), so watch-mode runs can silently execute against a stale build ifsrcchanges without a rebuild. Since vitest can run TypeScript directly, consider importing from../src/index.tsfor unit/mocked tests like this one, reserving the dist-based import for the integration suite that intentionally exercises the built package.The empty
beforeEachat Line 40-42 is also a no-op (mocks are created fresh inmakeClient()per test) and can be removed.Also applies to: 40-42
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/js/test/signing-client.test.ts` at line 5, The unit test in signing-client.test.ts is coupled to the built output by importing SafHandleSigningClient from the dist entry instead of the source entry, which can leave watch-mode tests running against stale compiled code. Update the test to import from the src index so vitest executes the latest TypeScript directly, and keep the dist-based import only for integration coverage if needed. Also remove the empty beforeEach in this test file since makeClient() already creates fresh mocks per test and the hook is a no-op.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/js/examples/register.ts`:
- Around line 10-12: The register example currently falls back to a placeholder
name when the CLI argument is missing, which can accidentally broadcast a real
transaction with the wrong value. Update the argument handling in register.ts so
the name is required and the script fails fast with a clear error if
process.argv[2] is absent, similar to the existing SAFHANDLE_MNEMONIC check; use
the existing name and mnemonic setup in the example to locate the change.
In `@packages/js/examples/resolve.ts`:
- Around line 5-7: The CONTRACT_ADDRESS fallback in resolve.ts still accepts an
empty SAFHANDLE_CONTRACT value, which can let connect() proceed with an invalid
address. Update the CONTRACT_ADDRESS assignment to trim and validate the
environment variable before using it, and fall back to
SAFROCHAIN_TESTNET.contractAddress only when the env var is missing or blank;
use the existing resolve.ts setup around CONTRACT_ADDRESS and connect() to keep
the example from passing a bad address downstream.
---
Nitpick comments:
In `@packages/js/src/constants.ts`:
- Around line 13-25: The gas price literal is duplicated between
SAFROCHAIN_TESTNET.gasPrice and DEFAULT_GAS_PRICE, which can drift out of sync.
Update the constants in constants.ts so there is a single source of truth for
the gas price, and have both SAFROCHAIN_TESTNET and DEFAULT_GAS_PRICE reference
that shared value instead of hardcoding the string twice. Use the
SAFROCHAIN_TESTNET and DEFAULT_GAS_PRICE symbols to keep the change localized
and easy to follow.
In `@packages/js/test/signing-client.test.ts`:
- Line 5: The unit test in signing-client.test.ts is coupled to the built output
by importing SafHandleSigningClient from the dist entry instead of the source
entry, which can leave watch-mode tests running against stale compiled code.
Update the test to import from the src index so vitest executes the latest
TypeScript directly, and keep the dist-based import only for integration
coverage if needed. Also remove the empty beforeEach in this test file since
makeClient() already creates fresh mocks per test and the hook is a no-op.
🪄 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 Plus
Run ID: b6c714e6-1e06-450a-a32e-6192d67998b3
⛔ Files ignored due to path filters (1)
packages/js/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
packages/js/LICENSEpackages/js/README.mdpackages/js/examples/register.tspackages/js/examples/resolve.tspackages/js/package.jsonpackages/js/scripts/write-cjs-package.mjspackages/js/src/client.tspackages/js/src/constants.tspackages/js/src/errors.tspackages/js/src/index.tspackages/js/src/signing-client.tspackages/js/src/types.tspackages/js/src/validation.tspackages/js/test/errors.test.tspackages/js/test/integration.test.tspackages/js/test/signing-client.test.tspackages/js/test/validation.test.tspackages/js/tsconfig.cjs.jsonpackages/js/tsconfig.json
| const name = process.argv[2] ?? "my-name"; | ||
| const mnemonic = process.env.SAFHANDLE_MNEMONIC; | ||
| if (!mnemonic) throw new Error("Set SAFHANDLE_MNEMONIC to a funded testnet account."); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Require an explicit name argument.
Defaulting to "my-name" makes a copied example capable of broadcasting a real register transaction for a placeholder name when the arg is omitted. Fail fast instead.
Suggested fix
- const name = process.argv[2] ?? "my-name";
+ const name = process.argv[2];
+ if (!name) {
+ throw new Error("Pass the name to register as the first CLI argument.");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const name = process.argv[2] ?? "my-name"; | |
| const mnemonic = process.env.SAFHANDLE_MNEMONIC; | |
| if (!mnemonic) throw new Error("Set SAFHANDLE_MNEMONIC to a funded testnet account."); | |
| const name = process.argv[2]; | |
| if (!name) { | |
| throw new Error("Pass the name to register as the first CLI argument."); | |
| } | |
| const mnemonic = process.env.SAFHANDLE_MNEMONIC; | |
| if (!mnemonic) throw new Error("Set SAFHANDLE_MNEMONIC to a funded testnet account."); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/js/examples/register.ts` around lines 10 - 12, The register example
currently falls back to a placeholder name when the CLI argument is missing,
which can accidentally broadcast a real transaction with the wrong value. Update
the argument handling in register.ts so the name is required and the script
fails fast with a clear error if process.argv[2] is absent, similar to the
existing SAFHANDLE_MNEMONIC check; use the existing name and mnemonic setup in
the example to locate the change.
| const CONTRACT_ADDRESS = | ||
| process.env.SAFHANDLE_CONTRACT ?? SAFROCHAIN_TESTNET.contractAddress!; | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Normalize the contract-address fallback.
?? will not fall back when SAFHANDLE_CONTRACT is set to an empty string, so the example can still reach connect() with a bad address and fail later with a less clear error. Trim/validate the env var before using it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/js/examples/resolve.ts` around lines 5 - 7, The CONTRACT_ADDRESS
fallback in resolve.ts still accepts an empty SAFHANDLE_CONTRACT value, which
can let connect() proceed with an invalid address. Update the CONTRACT_ADDRESS
assignment to trim and validate the environment variable before using it, and
fall back to SAFROCHAIN_TESTNET.contractAddress only when the env var is missing
or blank; use the existing resolve.ts setup around CONTRACT_ADDRESS and
connect() to keep the example from passing a bad address downstream.
2 participants
Summary
Adds the JavaScript/TypeScript SDK as
packages/js, the first concretepackage in the repository. It provides the SafHandle client for resolving
and registering handles on Safrochain.
What's included
src/— client, signing client, validation, errors, types, constantstest/— unit tests (validation, errors, signing client) + gatedintegration suite
examples/—resolve.ts,register.tstsconfig.json/tsconfig.cjs.jsonpackage.jsonpublished as@safrochain/safhandleBuild artifacts (
dist/,node_modules/) are excluded and covered by theexisting
.gitignore.Verification
npm install && npm run build— ESM + CJS build OKnpm test— 41 unit tests pass, 6 integration tests skipped(require
SAFHANDLE_INTEGRATION=1)npm run verifystill passes (docs/format/audit gate unaffected)Notes / follow-ups
wired as an npm workspace, so the root documentation-verification CI stays
dependency-free — wiring it as a workspace hoists dev deps and breaks the
audit --audit-level=highgate.SafHandleClient,SafHandleError) does not yetmatch the "Phase 2" API described in
docs/(SafHandle,getAddress,registerName, …). Aligning code and docs is a separate follow-up.Test plan