Merged
Conversation
raul-oliveira
moved this from In Progress (Done)
to In Progress (WIP)
in Hathor Network
raul-oliveira
moved this from In Progress (WIP)
to In Progress (Done)
in Hathor Network
pedroferreira1
requested review from
pedroferreira1
and removed request for
r4mmer
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPre-build (signTx: false) nano-contract transactions are added to compute fees and include preparedTx in confirmation prompts; create-token supports an optional token version; RPC schemas, types, request builders, and tests updated; send/create nano flows handle caller updates, PIN signing, and return runFromMining results or tx hex. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC as RPC Handler
participant Wallet
participant Tx as Pre-built Tx
participant Prompt as Confirmation Prompt
Client->>RPC: sendNanoContractTx(params, network, options?)
RPC->>Wallet: getCurrentAddress() / getNetworkObject()
Wallet-->>RPC: temp caller / network info
RPC->>Wallet: createNanoContractTransaction(params, signTx: false, caller=tmp)
Wallet-->>Tx: { transaction, runFromMining }
RPC->>Tx: transaction.getFeeHeader()
Tx-->>RPC: { entries: [...] }
RPC->>Prompt: show confirmation (fee, contractPaysFees, preparedTx)
Prompt-->>RPC: user confirms (may change caller, provides pin)
alt caller changed
RPC->>Wallet: setNanoHeaderCaller(nanoHeader, newCaller)
Wallet-->>RPC: ack
end
RPC->>Wallet: wallet.signTx(transaction, storage, pin)
Wallet-->>RPC: signed transaction
RPC->>Tx: transaction.prepareToSend()
alt push_tx == true
RPC->>Tx: runFromMining(signedTx)
Tx-->>RPC: send result
else
RPC->>Tx: toHex()
Tx-->>RPC: tx hex
end
RPC-->>Client: response (result or tx hex)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/hathor-rpc-handler/package.json (1)
33-33:⚠️ Potential issue | 🟡 MinorVerify whether
@hathor/wallet-libshould be updated from 2.14.0 to 2.15.0.Version 2.14.0 does require Node 22.0.0 or higher, confirming the Node version upgrade is justified. However, version 2.15.0 is now available on npm and also requires Node 22.0.0 or higher. Given the PR description mentions waiting for the wallet-lib final version to be published, clarify whether 2.14.0 is intentional or if it should be updated to 2.15.0.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/package.json` at line 33, The package.json currently pins "@hathor/wallet-lib": "2.14.0" but the PR notes waiting for a final wallet-lib release; verify whether this was intentional or should move to 2.15.0 by checking upstream release notes and compatibility, then update the dependency entry "@hathor/wallet-lib" to "2.15.0" if appropriate (and regenerate lockfile), or add a short comment/PR note confirming why 2.14.0 must remain; after changing, run install and the test suite to ensure no regressions.
🧹 Nitpick comments (3)
packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts (1)
181-183: Consider a more descriptive error message.The error "Failed to create transaction" doesn't convey whether the issue was with pre-building, validation, or another step. A more specific message would aid debugging.
Proposed improvement
if (!preBuildResult.transaction) { - throw new Error('Failed to create transaction'); + throw new Error('Pre-build step did not produce a transaction object'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts` around lines 181 - 183, Replace the generic throw in the createNanoContractCreateTokenTx flow with a descriptive error that identifies the failing step (e.g., "pre-build step produced no transaction") and includes any available diagnostics from preBuildResult (such as preBuildResult.error, validation messages, or the preBuildResult object) so callers can see why preBuildResult.transaction was absent; update the check around preBuildResult.transaction in the createNanoContractCreateTokenTx function to construct that more informative Error message.packages/hathor-rpc-handler/src/types/rpcRequest.ts (1)
35-35: Makeversionnullable in the request type to match runtime parsing.The parser accepts
version: null(and defaults to it), but the request interface currently excludesnull. Aligning both avoids typed API drift.♻️ Suggested type alignment
export interface CreateTokenRpcRequest { method: RpcMethods.CreateToken, params: { @@ - version?: TokenVersion + version?: TokenVersion | null; @@ } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/types/rpcRequest.ts` at line 35, The request type's "version" property currently declared as optional TokenVersion (version?: TokenVersion) should allow null to match runtime parsing; update the interface/type that declares "version" (e.g., the RpcRequest type) to use version?: TokenVersion | null so callers and the parser agree on nullability.packages/hathor-rpc-handler/__tests__/rpcMethods/createToken.test.ts (1)
119-119: Add one explicit non-nullversionforwarding test.This assertion covers the default
nullpath. A second case withparams.versionset (e.g., V2) would directly lock in the new acceptance criterion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/createToken.test.ts` at line 119, Add a second test in createToken.test.ts that explicitly sets params.version to a non-null value (e.g., V2) and asserts that the handler forwards that value (tokenVersion) instead of defaulting to null; locate the existing test that sets tokenVersion: null and duplicate its structure but set params.version = TokenVersion.V2 (or equivalent enum) and update expectations to verify the created token call/response contains the provided version, referencing the test file and the tokenVersion/params.version fields to find where to add the case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts`:
- Around line 58-63: The mocked return value for createNanoContractTransaction
is missing methods used by the happy path (getNanoHeaders, prepareToSend,
runFromMining), so update the mock(s) for createNanoContractTransaction to
include a transaction object that implements getFeeHeader, getNanoHeaders
(returning expected headers array), prepareToSend (mockResolvedValue with
whatever prepared state your code expects), and runFromMining (mockResolvedValue
with mining result); apply the same expanded mock shape to the other
createNanoContractTransaction mock instance mentioned in the comment (the second
occurrence) so the test exercises the accepted-path fully.
In `@packages/hathor-rpc-handler/package.json`:
- Line 12: Update the `@types/node` dependency in package.json from 20.14.2 to a
22.x range to match the engine requirement "node": ">=22" and ensure type
coverage for Node.js 22 APIs; locate the dependency entry "@types/node":
"20.14.2" and change it to a compatible spec like ">=22.0.0" or a pinned "22.x"
version, then run your package manager (npm/yarn/pnpm) to refresh lockfiles and
verify TypeScript builds against Node 22 types.
In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts`:
- Around line 138-141: The pre-build options currently spread params.options and
set signTx: false but don't explicitly disable UTXO locking; update the options
object passed in the pre-build step (the object that spreads params.options
where signTx is set to false within sendNanoContractTx) to also set lockUtxos:
false (i.e., include lockUtxos: false alongside signTx: false) so UTXOs are not
locked by wallet-lib defaults during pre-build.
- Around line 48-49: sendNanoContractSchema accepts params.options (validated by
nanoTxOptionsSchema) but the public request type
SendNanoContractRpcRequest.params does not include options, preventing typed
clients from passing fields like max_fee or contract_pays_fees; update the RPC
request type to include an optional options?: NanoTxOptions (or equivalent
interface) on SendNanoContractRpcRequest.params to match sendNanoContractSchema,
ensure the NanoTxOptions type mirrors nanoTxOptionsSchema fields, and update any
exports/imports so typed clients can pass params.options without unsafe casts.
---
Outside diff comments:
In `@packages/hathor-rpc-handler/package.json`:
- Line 33: The package.json currently pins "@hathor/wallet-lib": "2.14.0" but
the PR notes waiting for a final wallet-lib release; verify whether this was
intentional or should move to 2.15.0 by checking upstream release notes and
compatibility, then update the dependency entry "@hathor/wallet-lib" to "2.15.0"
if appropriate (and regenerate lockfile), or add a short comment/PR note
confirming why 2.14.0 must remain; after changing, run install and the test
suite to ensure no regressions.
---
Nitpick comments:
In `@packages/hathor-rpc-handler/__tests__/rpcMethods/createToken.test.ts`:
- Line 119: Add a second test in createToken.test.ts that explicitly sets
params.version to a non-null value (e.g., V2) and asserts that the handler
forwards that value (tokenVersion) instead of defaulting to null; locate the
existing test that sets tokenVersion: null and duplicate its structure but set
params.version = TokenVersion.V2 (or equivalent enum) and update expectations to
verify the created token call/response contains the provided version,
referencing the test file and the tokenVersion/params.version fields to find
where to add the case.
In
`@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts`:
- Around line 181-183: Replace the generic throw in the
createNanoContractCreateTokenTx flow with a descriptive error that identifies
the failing step (e.g., "pre-build step produced no transaction") and includes
any available diagnostics from preBuildResult (such as preBuildResult.error,
validation messages, or the preBuildResult object) so callers can see why
preBuildResult.transaction was absent; update the check around
preBuildResult.transaction in the createNanoContractCreateTokenTx function to
construct that more informative Error message.
In `@packages/hathor-rpc-handler/src/types/rpcRequest.ts`:
- Line 35: The request type's "version" property currently declared as optional
TokenVersion (version?: TokenVersion) should allow null to match runtime
parsing; update the interface/type that declares "version" (e.g., the RpcRequest
type) to use version?: TokenVersion | null so callers and the parser agree on
nullability.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
packages/hathor-rpc-handler/__tests__/rpcMethods/createToken.test.tspackages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.tspackages/hathor-rpc-handler/package.jsonpackages/hathor-rpc-handler/src/rpcHandler/index.tspackages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.tspackages/hathor-rpc-handler/src/rpcMethods/createToken.tspackages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.tspackages/hathor-rpc-handler/src/schemas/createTokenSchema.tspackages/hathor-rpc-handler/src/types/prompt.tspackages/hathor-rpc-handler/src/types/rpcRequest.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts (2)
772-798:⚠️ Potential issue | 🟡 MinorDefault
push_txtest case is inconsistent with its setup.The test says “when not provided”, but
push_tx: trueis explicitly set in the request, so defaulting is not actually being tested.🔧 Proposed fix
it('should use default push_tx value when not provided', async () => { @@ actions: validActions as unknown as NanoContractAction[], args: [] as unknown[], - push_tx: true, + // push_tx intentionally omitted to validate default behavior }, } as SendNanoContractRpcRequest;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts` around lines 772 - 798, The test "should use default push_tx value when not provided" mistakenly sets push_tx: true, so remove the explicit push_tx property (or set it to undefined) from validRequest.params to actually test the default branch; keep the rest of the request the same and verify behavior via the existing assertions (e.g., that sendNanoContractTx calls mockWallet.createNanoContractTransaction and the subsequent runFromMining/signing flow), referencing sendNanoContractTx and mockWallet.createNanoContractTransaction to locate and update the test.
334-394:⚠️ Potential issue | 🔴 CriticalFailure-path test is mocking the wrong method and won't execute the actual error path.
The implementation at line 134-210 of
sendNanoContractTx.tsusescreateNanoContractTransactionfollowed byrunFromMining(), but line 367 of the test mockscreateAndSendNanoContractTransaction. This obsolete method is never called, so the rejection stub won't trigger and the test won't validate the real failure path.🔧 Proposed fix
- (wallet.createAndSendNanoContractTransaction as jest.Mock).mockRejectedValue(new Error('Transaction failed')); + const failingTx = { + transaction: { + getFeeHeader: jest.fn().mockReturnValue({ entries: [] }), + getNanoHeaders: jest.fn().mockReturnValue([{ address: null, seqnum: 0 }]), + prepareToSend: jest.fn(), + toHex: jest.fn().mockReturnValue('tx-hex'), + }, + runFromMining: jest.fn().mockRejectedValue(new Error('Transaction failed')), + }; + (wallet.createNanoContractTransaction as jest.Mock).mockResolvedValue(failingTx);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts` around lines 334 - 394, Test is stubbing the wrong wallet method; update the test to mock the actual call path used by sendNanoContractTx by stubbing wallet.createNanoContractTransaction to return an object whose runFromMining method rejects (e.g., return { runFromMining: jest.fn().mockRejectedValue(new Error('Transaction failed')) }), so the sendNanoContractTx error branch is exercised; keep references to sendNanoContractTx, createNanoContractTransaction, and runFromMining when locating the test change.
🧹 Nitpick comments (2)
packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts (2)
48-62: Add explicit test coverage for tokenversionforwarding.The new flow is validated for fees/signing, but it never sets or asserts
createTokenOptions.version. That leaves the PR acceptance criterion unguarded in this suite.♻️ Suggested test hardening
const createTokenOptions = { name: 'TestToken', symbol: 'TT', + version: 2, amount: '100', mintAddress: 'wallet1', changeAddress: 'wallet1', @@ expect(wallet.createNanoContractCreateTokenTransaction).toHaveBeenCalledWith( rpcRequest.params.method, rpcRequest.params.address, expect.objectContaining({ blueprintId: 'blueprint123', ncId: 'nc123', args: [], }), expect.objectContaining({ name: createTokenOptions.name, symbol: createTokenOptions.symbol, + version: createTokenOptions.version, contractPaysTokenDeposit: createTokenOptions.contractPaysTokenDeposit, }), expect.objectContaining({ signTx: false }) );Also applies to: 186-196, 247-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts` around lines 48 - 62, The test lacks coverage asserting that the token `version` is forwarded; update the createTokenOptions object in the test (variable createTokenOptions) to include an explicit version value (e.g., version: 1 and another case for a different version if desired) and add assertions after calling the function under test (createNanoContractCreateTokenTx or the RPC method invoked) to verify the constructed transaction/request includes that version field (assert on the payload or the mocked call that receives createTokenOptions.version). Repeat the same addition/assertion for the other two test blocks referenced (around the other createTokenOptions instances) so each test validates version forwarding.
473-510: Assert call order to match the test intent.Line 473’s test name says “before sending”, but it only checks that
prepareToSendwas called. It should also assert ordering relative torunFromMining.♻️ Suggested assertion update
await createNanoContractCreateTokenTx(rpcRequest, wallet, {}, promptHandler); expect(mockTransaction.prepareToSend).toHaveBeenCalled(); +expect(mockSendTxResult.runFromMining).toHaveBeenCalled(); +expect( + mockTransaction.prepareToSend.mock.invocationCallOrder[0] +).toBeLessThan( + mockSendTxResult.runFromMining.mock.invocationCallOrder[0] +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts` around lines 473 - 510, The test currently only checks that mockTransaction.prepareToSend was called; update it to assert call order against runFromMining so "prepareToSend" ran before sending. After calling createNanoContractCreateTokenTx, add an assertion comparing the mock invocation order numbers on mockTransaction.prepareToSend.mock.invocationCallOrder[0] and mockTransaction.runFromMining.mock.invocationCallOrder[0] (expect the prepareToSend order to be less than runFromMining). This uses the existing mocks (mockTransaction, mockSendTxResult) and the function under test createNanoContractCreateTokenTx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts`:
- Around line 772-798: The test "should use default push_tx value when not
provided" mistakenly sets push_tx: true, so remove the explicit push_tx property
(or set it to undefined) from validRequest.params to actually test the default
branch; keep the rest of the request the same and verify behavior via the
existing assertions (e.g., that sendNanoContractTx calls
mockWallet.createNanoContractTransaction and the subsequent
runFromMining/signing flow), referencing sendNanoContractTx and
mockWallet.createNanoContractTransaction to locate and update the test.
- Around line 334-394: Test is stubbing the wrong wallet method; update the test
to mock the actual call path used by sendNanoContractTx by stubbing
wallet.createNanoContractTransaction to return an object whose runFromMining
method rejects (e.g., return { runFromMining: jest.fn().mockRejectedValue(new
Error('Transaction failed')) }), so the sendNanoContractTx error branch is
exercised; keep references to sendNanoContractTx, createNanoContractTransaction,
and runFromMining when locating the test change.
---
Nitpick comments:
In
`@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts`:
- Around line 48-62: The test lacks coverage asserting that the token `version`
is forwarded; update the createTokenOptions object in the test (variable
createTokenOptions) to include an explicit version value (e.g., version: 1 and
another case for a different version if desired) and add assertions after
calling the function under test (createNanoContractCreateTokenTx or the RPC
method invoked) to verify the constructed transaction/request includes that
version field (assert on the payload or the mocked call that receives
createTokenOptions.version). Repeat the same addition/assertion for the other
two test blocks referenced (around the other createTokenOptions instances) so
each test validates version forwarding.
- Around line 473-510: The test currently only checks that
mockTransaction.prepareToSend was called; update it to assert call order against
runFromMining so "prepareToSend" ran before sending. After calling
createNanoContractCreateTokenTx, add an assertion comparing the mock invocation
order numbers on mockTransaction.prepareToSend.mock.invocationCallOrder[0] and
mockTransaction.runFromMining.mock.invocationCallOrder[0] (expect the
prepareToSend order to be less than runFromMining). This uses the existing mocks
(mockTransaction, mockSendTxResult) and the function under test
createNanoContractCreateTokenTx.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.tspackages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts
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 (3)
packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts (2)
175-207:⚠️ Potential issue | 🟠 MajorAlways emit loading-finished trigger via
finally.If signing/sending throws,
CreateNanoContractCreateTokenTxLoadingFinishedTriggeris never emitted, which can leave the client stuck in loading state.🔧 Suggested fix
const loadingTrigger: CreateNanoContractCreateTokenTxLoadingTrigger = { type: TriggerTypes.CreateNanoContractCreateTokenTxLoadingTrigger, }; - promptHandler(loadingTrigger, requestMetadata); + await promptHandler(loadingTrigger, requestMetadata); - // Sign the transaction - if (!preBuildResult.transaction) { - throw new Error('Failed to create transaction'); - } - await transactionUtils.signTransaction( - preBuildResult.transaction, - wallet.storage, - pinResponse.data.pinCode - ); - preBuildResult.transaction.prepareToSend(); - - // Send or return hex based on push_tx flag - let response: Transaction | string; - if (push_tx) { - // Send the transaction - response = await preBuildResult.runFromMining(); - } else { - // Convert to hex format for the response when not pushing to network - response = preBuildResult.transaction.toHex(); - } - - // Emit loading finished trigger - const loadingFinishedTrigger: CreateNanoContractCreateTokenTxLoadingFinishedTrigger = { - type: TriggerTypes.CreateNanoContractCreateTokenTxLoadingFinishedTrigger, - }; - promptHandler(loadingFinishedTrigger, requestMetadata); + let response: Transaction | string; + try { + if (!preBuildResult.transaction) { + throw new Error('Failed to create transaction'); + } + await transactionUtils.signTransaction( + preBuildResult.transaction, + wallet.storage, + pinResponse.data.pinCode + ); + preBuildResult.transaction.prepareToSend(); + response = push_tx + ? await preBuildResult.runFromMining() + : preBuildResult.transaction.toHex(); + } finally { + const loadingFinishedTrigger: CreateNanoContractCreateTokenTxLoadingFinishedTrigger = { + type: TriggerTypes.CreateNanoContractCreateTokenTxLoadingFinishedTrigger, + }; + await promptHandler(loadingFinishedTrigger, requestMetadata); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts` around lines 175 - 207, The loading-finished trigger is not emitted if signing/sending throws; wrap the sign/send logic (the block that calls transactionUtils.signTransaction, preBuildResult.transaction.prepareToSend, preBuildResult.runFromMining and the push_tx branch) in a try/finally and move the promptHandler call that emits CreateNanoContractCreateTokenTxLoadingFinishedTrigger into the finally so it always runs; keep the initial CreateNanoContractCreateTokenTxLoadingTrigger emission as-is and ensure you do not duplicate the finished trigger elsewhere.
84-99:⚠️ Potential issue | 🟠 MajorFail fast when
createTokenOptionsis missing.Defaulting to empty
name/symboland0ncan push invalid state into wallet-lib and fail later with less actionable errors. This endpoint should reject missing token options at validation time.🔧 Suggested fix
- const preBuildTokenOptions = { - name: createTokenOptions?.name ?? '', - symbol: createTokenOptions?.symbol ?? '', - amount: typeof createTokenOptions?.amount === 'string' ? BigInt(createTokenOptions.amount) : (createTokenOptions?.amount ?? 0n), - version: createTokenOptions?.version ?? null, - mintAddress: createTokenOptions?.mintAddress ?? address, - changeAddress: createTokenOptions?.changeAddress ?? null, - createMint: createTokenOptions?.createMint ?? true, - mintAuthorityAddress: createTokenOptions?.mintAuthorityAddress ?? null, - allowExternalMintAuthorityAddress: createTokenOptions?.allowExternalMintAuthorityAddress ?? false, - createMelt: createTokenOptions?.createMelt ?? true, - meltAuthorityAddress: createTokenOptions?.meltAuthorityAddress ?? null, - allowExternalMeltAuthorityAddress: createTokenOptions?.allowExternalMeltAuthorityAddress ?? false, - data: createTokenOptions?.data ?? null, - contractPaysTokenDeposit: createTokenOptions?.contractPaysTokenDeposit ?? false, - }; + if (!createTokenOptions) { + throw new InvalidParamsError('createTokenOptions is required'); + } + const preBuildTokenOptions = { + name: createTokenOptions.name, + symbol: createTokenOptions.symbol, + amount: typeof createTokenOptions.amount === 'string' ? BigInt(createTokenOptions.amount) : createTokenOptions.amount, + version: createTokenOptions.version ?? null, + mintAddress: createTokenOptions.mintAddress ?? address, + changeAddress: createTokenOptions.changeAddress ?? null, + createMint: createTokenOptions.createMint ?? true, + mintAuthorityAddress: createTokenOptions.mintAuthorityAddress ?? null, + allowExternalMintAuthorityAddress: createTokenOptions.allowExternalMintAuthorityAddress ?? false, + createMelt: createTokenOptions.createMelt ?? true, + meltAuthorityAddress: createTokenOptions.meltAuthorityAddress ?? null, + allowExternalMeltAuthorityAddress: createTokenOptions.allowExternalMeltAuthorityAddress ?? false, + data: createTokenOptions.data ?? null, + contractPaysTokenDeposit: createTokenOptions.contractPaysTokenDeposit ?? false, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts` around lines 84 - 99, Reject requests when createTokenOptions is absent or null instead of silently defaulting values: at the start of the handler that constructs preBuildTokenOptions (refer to createTokenOptions and preBuildTokenOptions) validate that createTokenOptions is provided and contains required fields (at minimum name, symbol, amount or other required keys) and throw or return a validation error; remove or guard the fallback defaults so invalid/missing input does not propagate into wallet-lib and surface a clear error message to the caller.packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts (1)
189-221:⚠️ Potential issue | 🟠 MajorEmit loading-finished trigger in a
finallyblock.If signing/mining fails, the finished trigger is skipped and the UI may remain in loading state.
🔧 Suggested fix
const sendNanoContractLoadingTrigger: SendNanoContractTxLoadingTrigger = { type: TriggerTypes.SendNanoContractTxLoadingTrigger, }; - triggerHandler(sendNanoContractLoadingTrigger, requestMetadata); + await triggerHandler(sendNanoContractLoadingTrigger, requestMetadata); - - let response: Transaction | string; + let response: Transaction | string; + try { // If caller changed, update the pre-built transaction if (confirmedCaller !== tempCallerResult.address) { const nanoHeaders = preBuildSendTx.transaction.getNanoHeaders(); if (nanoHeaders && nanoHeaders.length > 0) { @@ if (params.pushTx) { response = await preBuildSendTx.runFromMining(); } else { // Convert the transaction object to hex format for the response response = preBuildSendTx.transaction.toHex(); } - - const sendNanoContractLoadingFinishedTrigger: SendNanoContractTxLoadingFinishedTrigger = { - type: TriggerTypes.SendNanoContractTxLoadingFinishedTrigger, - }; - triggerHandler(sendNanoContractLoadingFinishedTrigger, requestMetadata); + } finally { + const sendNanoContractLoadingFinishedTrigger: SendNanoContractTxLoadingFinishedTrigger = { + type: TriggerTypes.SendNanoContractTxLoadingFinishedTrigger, + }; + await triggerHandler(sendNanoContractLoadingFinishedTrigger, requestMetadata); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts` around lines 189 - 221, The loading-finished trigger (SendNanoContractTxLoadingFinishedTrigger) must be emitted in a finally block so it always fires even if signing or pushing fails; wrap the signing/prepare/runFromMining/toHex logic (the calls to transactionUtils.signTransaction, preBuildSendTx.transaction.prepareToSend, and preBuildSendTx.runFromMining / preBuildSendTx.transaction.toHex) in a try/finally and call triggerHandler(sendNanoContractLoadingFinishedTrigger, requestMetadata) in the finally; keep the initial triggerHandler(sendNanoContractLoadingTrigger, requestMetadata) as-is and ensure any thrown error is rethrown after the finally so existing error handling upstream still runs.
♻️ Duplicate comments (1)
packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts (1)
138-141:⚠️ Potential issue | 🟠 MajorPre-build still does not explicitly disable UTXO locking.
This is the same concern raised earlier and still appears unresolved in the current hunk. Please include
lockUtxos: falseexplicitly in pre-build options.🔧 Suggested fix
const preBuildSendTx = await wallet.createNanoContractTransaction( params.method, tempCallerResult.address, preBuildTxData, { ...params.options, signTx: false, + lockUtxos: false, }, );In `@hathor/wallet-lib`, for createNanoContractTransaction pre-build usage, is lockUtxos disabled automatically when signTx is false, or must lockUtxos: false be set explicitly?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts` around lines 138 - 141, The pre-build call in sendNanoContractTx.ts spreads params.options and sets signTx: false but does not explicitly disable UTXO locking; update the options passed to the createNanoContractTransaction pre-build to include lockUtxos: false alongside signTx: false (i.e., ensure the merged options object contains both signTx: false and lockUtxos: false) so the UTXO lock is explicitly disabled even if the upstream behavior changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts`:
- Line 126: The code currently force-casts wallet.getCurrentAddress() to {
address: string } (tempCallerResult) which can mask null/invalid returns; change
sendNanoContractTx to call wallet.getCurrentAddress(), verify the result is
non-null and has a non-empty string address (e.g. typeof result === 'object' &&
typeof result.address === 'string' && result.address.trim() !== ''), and if not
throw a clear domain error (or return an appropriate RPC error) instead of
proceeding with the casted value; update any downstream usage of
tempCallerResult to rely on the validated value.
---
Outside diff comments:
In
`@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts`:
- Around line 175-207: The loading-finished trigger is not emitted if
signing/sending throws; wrap the sign/send logic (the block that calls
transactionUtils.signTransaction, preBuildResult.transaction.prepareToSend,
preBuildResult.runFromMining and the push_tx branch) in a try/finally and move
the promptHandler call that emits
CreateNanoContractCreateTokenTxLoadingFinishedTrigger into the finally so it
always runs; keep the initial CreateNanoContractCreateTokenTxLoadingTrigger
emission as-is and ensure you do not duplicate the finished trigger elsewhere.
- Around line 84-99: Reject requests when createTokenOptions is absent or null
instead of silently defaulting values: at the start of the handler that
constructs preBuildTokenOptions (refer to createTokenOptions and
preBuildTokenOptions) validate that createTokenOptions is provided and contains
required fields (at minimum name, symbol, amount or other required keys) and
throw or return a validation error; remove or guard the fallback defaults so
invalid/missing input does not propagate into wallet-lib and surface a clear
error message to the caller.
In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts`:
- Around line 189-221: The loading-finished trigger
(SendNanoContractTxLoadingFinishedTrigger) must be emitted in a finally block so
it always fires even if signing or pushing fails; wrap the
signing/prepare/runFromMining/toHex logic (the calls to
transactionUtils.signTransaction, preBuildSendTx.transaction.prepareToSend, and
preBuildSendTx.runFromMining / preBuildSendTx.transaction.toHex) in a
try/finally and call triggerHandler(sendNanoContractLoadingFinishedTrigger,
requestMetadata) in the finally; keep the initial
triggerHandler(sendNanoContractLoadingTrigger, requestMetadata) as-is and ensure
any thrown error is rethrown after the finally so existing error handling
upstream still runs.
---
Duplicate comments:
In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts`:
- Around line 138-141: The pre-build call in sendNanoContractTx.ts spreads
params.options and sets signTx: false but does not explicitly disable UTXO
locking; update the options passed to the createNanoContractTransaction
pre-build to include lockUtxos: false alongside signTx: false (i.e., ensure the
merged options object contains both signTx: false and lockUtxos: false) so the
UTXO lock is explicitly disabled even if the upstream behavior changes.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 713abcf9-91c2-4719-8ac9-5ced4638e68c
📒 Files selected for processing (3)
packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.tspackages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.tspackages/hathor-rpc-handler/src/types/prompt.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts (1)
127-133:⚠️ Potential issue | 🟡 MinorValidate
getCurrentAddress()shape before calling.trim().The force-cast can still throw a runtime
TypeErrorwhenaddressis missing/non-string, bypassing your domain error handling.🛠️ Suggested fix
- const tempCallerResult = await wallet.getCurrentAddress() as { address: string }; - if (!tempCallerResult || tempCallerResult.address.trim() === '') { + const tempCallerResult = await wallet.getCurrentAddress(); + const tempCallerAddress = ( + tempCallerResult && + typeof tempCallerResult === 'object' && + typeof (tempCallerResult as { address?: unknown }).address === 'string' + ) ? (tempCallerResult as { address: string }).address.trim() : ''; + + if (tempCallerAddress === '') { throw new SendNanoContractTxError('Unable to get current wallet address'); } - - const tempCallerAddress = tempCallerResult.address;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts` around lines 127 - 133, The code force-casts getCurrentAddress() to { address: string } then calls .trim(), which can still throw if address is missing or not a string; update the logic in sendNanoContractTx (the tempCallerResult/tempCallerAddress handling) to avoid the cast and explicitly validate the shape by checking that tempCallerResult exists and typeof tempCallerResult.address === 'string' and tempCallerResult.address.trim() !== '' before using .trim(); if validation fails, throw SendNanoContractTxError('Unable to get current wallet address'); finally set tempCallerAddress to the trimmed string.
🧹 Nitpick comments (4)
packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts (1)
140-147: Add an explicit inline note that pre-build keeps UTXO locking enabled by design.A short comment here will prevent future regressions that reintroduce
lockUtxos: false.Based on learnings: pre-build
createNanoContractTransaction(..., { signTx: false })is intentionally allowed to lock UTXOs from the pre-build phase onward.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts` around lines 140 - 147, Add a concise inline comment above the createNanoContractTransaction call explaining that preBuildSendTx (from createNanoContractTransaction(..., { signTx: false })) intentionally keeps UTXO locking enabled by design and therefore should not set lockUtxos: false; reference the call site (createNanoContractTransaction) and the preBuildSendTx variable so future changes do not disable UTXO locking during the pre-build phase.packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts (1)
132-157: Consider extracting repeated confirmation/PIN mock setup into helpers.The same response scaffolding is repeated many times, which makes future schema updates harder to maintain.
♻️ Optional helper extraction sketch
+const mockAcceptedPrompts = ( + promptHandler: jest.Mock, + nanoData: unknown, + tokenData: unknown, + pinCode = '1234', +) => { + promptHandler + .mockResolvedValueOnce({ + type: TriggerResponseTypes.CreateNanoContractCreateTokenTxConfirmationResponse, + data: { accepted: true, nano: nanoData, token: tokenData }, + }) + .mockResolvedValueOnce({ + type: TriggerResponseTypes.PinRequestResponse, + data: { accepted: true, pinCode }, + }); +};Also applies to: 205-230, 277-300, 439-464, 482-507, 521-546, 562-587, 614-636
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts` around lines 132 - 157, The test repeats identical promptHandler.mockResolvedValueOnce scaffolding for CreateNanoContractCreateTokenTxConfirmationResponse and PinRequestResponse across multiple blocks (e.g., where promptHandler.mockResolvedValueOnce is used with TriggerResponseTypes.CreateNanoContractCreateTokenTxConfirmationResponse and TriggerResponseTypes.PinRequestResponse, referencing rpcRequest.params, createTokenOptions and pinCode); extract a small helper function (e.g., makeConfirmationMock and makePinMock) that returns the resolved value objects and use it in tests to replace the repeated mocks so updates to the response shape are centralized.packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts (1)
67-73: Consider centralizing repeated transaction mocks into a shared factory.This would cut duplication and reduce drift when transaction shape evolves again.
Also applies to: 105-114, 212-223, 287-299, 350-360, 524-535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts` around lines 67 - 73, Multiple tests in sendNanoContractTx.test.ts duplicate the same mockTransaction shape; extract a shared factory (e.g., createMockTransaction) into a test util and use it across all spots (lines referencing mockTransaction at the shown blocks and the other occurrences) to avoid duplication and drift. Implement createMockTransaction to return the object with getFeeHeader, getNanoHeaders, prepareToSend, toHex (and allow optional overrides), replace each inline mockTransaction with a call to this factory in the tests, and import the factory in the test file so future shape changes are made in one place.packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts (1)
109-115: Add an explicit comment that pre-build intentionally does not disable UTXO locking.This will help preserve the intended behavior in future refactors.
Based on learnings: pre-build calls with
signTx: falseshould keep default UTXO locking and should not addlockUtxos: false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts` around lines 109 - 115, The pre-build call to wallet.createNanoContractCreateTokenTransaction (the invocation that sets { ...options, signTx: false }) intentionally must not disable UTXO locking; add a clear inline comment above this call stating that because signTx is false for pre-building we deliberately keep default UTXO locking (do not add lockUtxos: false or modify options) so future refactors won’t accidentally disable UTXO locks; reference the preBuildResult variable and the createNanoContractCreateTokenTransaction call in the comment for clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts`:
- Around line 117-119: Replace the generic throw in
createNanoContractCreateTokenTx where it checks preBuildResult.transaction with
a domain-specific error type used by the RPC layer (e.g., RpcError or
DomainError) so clients can handle it consistently; locate the check for
preBuildResult.transaction and throw the appropriate error class (including a
clear error message and any required error code/metadata) instead of new
Error('Unable to create transaction object').
---
Duplicate comments:
In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts`:
- Around line 127-133: The code force-casts getCurrentAddress() to { address:
string } then calls .trim(), which can still throw if address is missing or not
a string; update the logic in sendNanoContractTx (the
tempCallerResult/tempCallerAddress handling) to avoid the cast and explicitly
validate the shape by checking that tempCallerResult exists and typeof
tempCallerResult.address === 'string' and tempCallerResult.address.trim() !== ''
before using .trim(); if validation fails, throw SendNanoContractTxError('Unable
to get current wallet address'); finally set tempCallerAddress to the trimmed
string.
---
Nitpick comments:
In
`@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts`:
- Around line 132-157: The test repeats identical
promptHandler.mockResolvedValueOnce scaffolding for
CreateNanoContractCreateTokenTxConfirmationResponse and PinRequestResponse
across multiple blocks (e.g., where promptHandler.mockResolvedValueOnce is used
with TriggerResponseTypes.CreateNanoContractCreateTokenTxConfirmationResponse
and TriggerResponseTypes.PinRequestResponse, referencing rpcRequest.params,
createTokenOptions and pinCode); extract a small helper function (e.g.,
makeConfirmationMock and makePinMock) that returns the resolved value objects
and use it in tests to replace the repeated mocks so updates to the response
shape are centralized.
In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts`:
- Around line 67-73: Multiple tests in sendNanoContractTx.test.ts duplicate the
same mockTransaction shape; extract a shared factory (e.g.,
createMockTransaction) into a test util and use it across all spots (lines
referencing mockTransaction at the shown blocks and the other occurrences) to
avoid duplication and drift. Implement createMockTransaction to return the
object with getFeeHeader, getNanoHeaders, prepareToSend, toHex (and allow
optional overrides), replace each inline mockTransaction with a call to this
factory in the tests, and import the factory in the test file so future shape
changes are made in one place.
In
`@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts`:
- Around line 109-115: The pre-build call to
wallet.createNanoContractCreateTokenTransaction (the invocation that sets {
...options, signTx: false }) intentionally must not disable UTXO locking; add a
clear inline comment above this call stating that because signTx is false for
pre-building we deliberately keep default UTXO locking (do not add lockUtxos:
false or modify options) so future refactors won’t accidentally disable UTXO
locks; reference the preBuildResult variable and the
createNanoContractCreateTokenTransaction call in the comment for clarity.
In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts`:
- Around line 140-147: Add a concise inline comment above the
createNanoContractTransaction call explaining that preBuildSendTx (from
createNanoContractTransaction(..., { signTx: false })) intentionally keeps UTXO
locking enabled by design and therefore should not set lockUtxos: false;
reference the call site (createNanoContractTransaction) and the preBuildSendTx
variable so future changes do not disable UTXO locking during the pre-build
phase.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed3f91b7-4712-43c8-a81d-fda0b21abb53
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.tspackages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.tspackages/hathor-rpc-handler/package.jsonpackages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.tspackages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.tspackages/hathor-rpc-handler/src/rpcRequest/sendNanoContractTx.tspackages/hathor-rpc-handler/src/types/prompt.tspackages/hathor-rpc-handler/src/types/rpcRequest.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/hathor-rpc-handler/src/types/prompt.ts
- packages/hathor-rpc-handler/package.json
raul-oliveira
force-pushed
the
raul-oliveira/feat-fee-token-creation
branch
from
88ee7fb to
7fddf08
Compare
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts (1)
248-263: Add an explicit assertion for tokenversionforwarding.The PR objective includes forwarding token version, but this call assertion doesn’t currently validate that behavior (explicit version or default
null).Suggested assertion tweak
expect.objectContaining({ name: createTokenOptions.name, symbol: createTokenOptions.symbol, + version: null, contractPaysTokenDeposit: createTokenOptions.contractPaysTokenDeposit, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts` around lines 248 - 263, The test currently verifies many forwarded fields but omits asserting the token version; update the call assertion for wallet.createNanoContractCreateTokenTransaction in the createNanoContractCreateTokenTx test so it explicitly checks that the forwarded options include a version field matching the input (e.g., forward createTokenOptions.version when provided, or null when absent), by adding an expect.objectContaining({ version: createTokenOptions.version ?? null }) (or equivalent) to the options argument check against rpcRequest.packages/hathor-rpc-handler/src/types/prompt.ts (1)
10-10: Use a type-only import forTokenVersionandTransaction.Both symbols are only used in type positions (lines 191, 199, 321); using value import syntax is unnecessary and hides the type-only intent.
Proposed change
-import { TokenVersion, Transaction } from '@hathor/wallet-lib'; +import type { TokenVersion, Transaction } from '@hathor/wallet-lib';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/types/prompt.ts` at line 10, Replace the runtime import of TokenVersion and Transaction with a type-only import: change the import that currently brings in TokenVersion and Transaction from '@hathor/wallet-lib' to an "import type" form so these symbols are imported only for types (they are used as types in the Prompt-related interfaces/props including the declarations referenced for TokenVersion and Transaction); update the import statement that also imports other non-type symbols if present to keep runtime imports separate and ensure no runtime usage remains of TokenVersion/Transaction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts`:
- Around line 372-377: The mock implementation for promptHandler
(promptHandler.mockImplementation) returns an object missing the required
TriggerResponse "type" field; update the mocked returns (both the branch on
TriggerTypes.CreateNanoContractCreateTokenTxConfirmationPrompt at lines shown
and the other branch at 403-407) to return a fully shaped TriggerResponse by
including the appropriate response "type" value alongside data (e.g., the
corresponding response enum value for
CreateNanoContractCreateTokenTxConfirmationPrompt) and keep data: { accepted:
false }; ensure the mocked object matches the TriggerResponse contract so
type-guards and dispatch logic exercise correctly.
In `@packages/hathor-rpc-handler/src/types/prompt.ts`:
- Around line 189-191: NanoContractParams is forcing preparedTx into response
payloads because it's reused by SendNanoContractTxConfirmationResponse and
CreateNanoContractCreateTokenTxConfirmationResponse; make preparedTx not
required to avoid forcing clients to echo Transaction objects. Fix by changing
the shared NanoContractParams type so preparedTx is optional (preparedTx?:
Transaction) or split the type into two (e.g., NanoContractParams and
NanoContractPreparedParams) and use the prepared variant only where an actual
prepared Transaction is returned, updating references to NanoContractParams,
SendNanoContractTxConfirmationResponse, and
CreateNanoContractCreateTokenTxConfirmationResponse accordingly.
---
Nitpick comments:
In
`@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts`:
- Around line 248-263: The test currently verifies many forwarded fields but
omits asserting the token version; update the call assertion for
wallet.createNanoContractCreateTokenTransaction in the
createNanoContractCreateTokenTx test so it explicitly checks that the forwarded
options include a version field matching the input (e.g., forward
createTokenOptions.version when provided, or null when absent), by adding an
expect.objectContaining({ version: createTokenOptions.version ?? null }) (or
equivalent) to the options argument check against rpcRequest.
In `@packages/hathor-rpc-handler/src/types/prompt.ts`:
- Line 10: Replace the runtime import of TokenVersion and Transaction with a
type-only import: change the import that currently brings in TokenVersion and
Transaction from '@hathor/wallet-lib' to an "import type" form so these symbols
are imported only for types (they are used as types in the Prompt-related
interfaces/props including the declarations referenced for TokenVersion and
Transaction); update the import statement that also imports other non-type
symbols if present to keep runtime imports separate and ensure no runtime usage
remains of TokenVersion/Transaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b75bcd08-723f-495c-b2d2-900991b1a4e3
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.tspackages/hathor-rpc-handler/package.jsonpackages/hathor-rpc-handler/src/types/prompt.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hathor-rpc-handler/package.json
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)
packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts (1)
163-189:⚠️ Potential issue | 🟡 MinorGuard
callerbefore updating nano header.At Line 164/183, if
nano.calleris missing,undefined !== addressis true andsetNanoHeaderCallermay receive an invalid caller value.🛠️ Suggested fix
- const { nano } = confirmationResponse.data; - const confirmedCaller = nano.caller; + const { nano } = confirmationResponse.data; + const confirmedCaller = nano.caller ?? address; @@ - if (confirmedCaller !== address) { + if (confirmedCaller !== address) { const nanoHeaders = preBuildResult.transaction.getNanoHeaders(); if (!nanoHeaders || nanoHeaders.length === 0) { throw new SendNanoContractTxError('No nano headers found in the transaction'); } await wallet.setNanoHeaderCaller(nanoHeaders[0], confirmedCaller); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts` around lines 163 - 189, The code compares confirmedCaller (from confirmationResponse.data.nano.caller) to address and may call wallet.setNanoHeaderCaller with an undefined caller; guard against missing caller by checking that confirmedCaller is a non-empty string (or otherwise valid) before performing the !== address check and before calling setNanoHeaderCaller; if the caller is absent, either skip the update or throw a clear SendNanoContractTxError mentioning "missing nano caller", and ensure you still validate nanoHeaders via preBuildResult.transaction.getNanoHeaders() before calling setNanoHeaderCaller.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts`:
- Around line 191-199: The code currently calls wallet.signTx(...) and then
transactionUtils.signTransaction(...) on the same transaction in
createNanoContractCreateTokenTx (while sendNanoContractTx only calls
wallet.signTx), so either consolidate to a single signing path or explicitly
document why both are required; to fix, decide whether token creation truly
needs two distinct signing steps—if not, remove the redundant call to
transactionUtils.signTransaction and update tests to expect only wallet.signTx,
otherwise add a clear comment above both calls in
createNanoContractCreateTokenTx explaining the different responsibilities (e.g.,
which key/storage each function signs with) and keep both to preserve test
behavior. Ensure references to wallet.signTx, transactionUtils.signTransaction,
and sendNanoContractTx are used to locate the code.
---
Outside diff comments:
In
`@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts`:
- Around line 163-189: The code compares confirmedCaller (from
confirmationResponse.data.nano.caller) to address and may call
wallet.setNanoHeaderCaller with an undefined caller; guard against missing
caller by checking that confirmedCaller is a non-empty string (or otherwise
valid) before performing the !== address check and before calling
setNanoHeaderCaller; if the caller is absent, either skip the update or throw a
clear SendNanoContractTxError mentioning "missing nano caller", and ensure you
still validate nanoHeaders via preBuildResult.transaction.getNanoHeaders()
before calling setNanoHeaderCaller.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e494517c-8f2a-4131-b9cf-b5c2aee3d9b6
📒 Files selected for processing (1)
packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts (1)
373-377:⚠️ Potential issue | 🟡 MinorReturn fully shaped
TriggerResponseobjects in rejection branches.Line [376] and Line [407] still return
{ data: { accepted: false } }withouttype, which weakens contract fidelity and can hide response-dispatch regressions.Suggested fix
- return { data: { accepted: false } }; // Reject to end early + return { + type: TriggerResponseTypes.CreateNanoContractCreateTokenTxConfirmationResponse, + data: { accepted: false }, + }; // Reject to end early- return { data: { accepted: false } }; // Reject to end early + return { + type: TriggerResponseTypes.CreateNanoContractCreateTokenTxConfirmationResponse, + data: { accepted: false }, + }; // Reject to end earlyAlso applies to: 404-408
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts` around lines 373 - 377, The mock implementation for promptHandler.mockImplementation used for CreateNanoContractCreateTokenTxConfirmationPrompt is returning a partial object ({ data: { accepted: false } }) in rejection branches; update these returns to full TriggerResponse-shaped objects including the correct type field (use TriggerTypes.CreateNanoContractCreateTokenTxConfirmationPrompt or the appropriate TriggerTypes value) alongside data so the mock respects the TriggerResponse contract and prevents response-dispatch regressions—apply the same fix to both the branch around capturedPrompt and the similar branch at lines ~404-408.
🧹 Nitpick comments (2)
packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts (1)
189-199: Add explicit assertions thatcreateTokenOptions.versionis forwarded.Current expectations can still pass if
versionis dropped in the call path, which weakens coverage for this PR’s token-version forwarding objective.Suggested test assertion hardening
expect(wallet.createNanoContractCreateTokenTransaction).toHaveBeenCalledWith( rpcRequest.params.method, rpcRequest.params.address, expect.objectContaining({ blueprintId: 'blueprint123', ncId: 'nc123', args: [], }), - expect.anything(), + expect.objectContaining({ + version: createTokenOptions.version, + }), expect.objectContaining({ signTx: false }) );expect.objectContaining({ name: createTokenOptions.name, symbol: createTokenOptions.symbol, + version: createTokenOptions.version, contractPaysTokenDeposit: createTokenOptions.contractPaysTokenDeposit, }),Also applies to: 250-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts` around lines 189 - 199, Update the test assertions to explicitly assert that createTokenOptions.version is forwarded to wallet.createNanoContractCreateTokenTransaction: in the expect for wallet.createNanoContractCreateTokenTransaction (the call that currently uses expect.objectContaining with blueprintId, ncId, args), include version: '<expectedVersion>' inside that objectContaining (i.e. expect.objectContaining({ blueprintId: 'blueprint123', ncId: 'nc123', args: [], version: 'vX' })), and mirror the same explicit assertion in the second assertion block around lines 250-264 so the test fails if version is dropped.packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts (1)
455-455: Use specific error assertions instead of generictoThrow().Line [455] and Line [504] currently pass for any thrown error, which can hide unrelated failures.
Suggested tightening
-import { SendNanoContractTxError, InvalidParamsError } from '../../src/errors'; +import { SendNanoContractTxError, InvalidParamsError, PromptRejectedError } from '../../src/errors';- await expect(sendNanoContractTx(rpcRequest, wallet, {}, promptHandler)).rejects.toThrow(); + await expect(sendNanoContractTx(rpcRequest, wallet, {}, promptHandler)).rejects.toThrow(PromptRejectedError);- await expect(sendNanoContractTx(rpcRequest, wallet, {}, promptHandler)).rejects.toThrow(); + await expect(sendNanoContractTx(rpcRequest, wallet, {}, promptHandler)).rejects.toThrow(PromptRejectedError);Also applies to: 504-504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts` at line 455, The test uses a generic await expect(sendNanoContractTx(rpcRequest, wallet, {}, promptHandler)).rejects.toThrow() which hides unrelated failures; update both occurrences to assert a specific error type or message (e.g., .rejects.toThrowError(/expected message/) or .rejects.toBeInstanceOf(ExpectedErrorClass) ) that matches the error sendNanoContractTx is supposed to raise for the given rpcRequest/wallet/promptHandler inputs; replace the generic toThrow() with these specific assertions to ensure the test fails only for the intended error cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts`:
- Around line 433-437: The mock implementation for promptHandler in the
sendNanoContractTx test returns an untyped object on rejection, breaking the
TriggerResponse contract; update the branch in promptHandler.mockImplementation
that handles SendNanoContractTxConfirmationPrompt to return a properly typed
response object including type:
TriggerTypes.SendNanoContractTxConfirmationResponse and the data payload {
accepted: false } (i.e., mirror the TriggerResponse shape instead of returning a
bare object) so the mock satisfies the TriggerResponse<...> contract for the
SendNanoContractTx confirmation flow.
---
Duplicate comments:
In
`@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts`:
- Around line 373-377: The mock implementation for
promptHandler.mockImplementation used for
CreateNanoContractCreateTokenTxConfirmationPrompt is returning a partial object
({ data: { accepted: false } }) in rejection branches; update these returns to
full TriggerResponse-shaped objects including the correct type field (use
TriggerTypes.CreateNanoContractCreateTokenTxConfirmationPrompt or the
appropriate TriggerTypes value) alongside data so the mock respects the
TriggerResponse contract and prevents response-dispatch regressions—apply the
same fix to both the branch around capturedPrompt and the similar branch at
lines ~404-408.
---
Nitpick comments:
In
`@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts`:
- Around line 189-199: Update the test assertions to explicitly assert that
createTokenOptions.version is forwarded to
wallet.createNanoContractCreateTokenTransaction: in the expect for
wallet.createNanoContractCreateTokenTransaction (the call that currently uses
expect.objectContaining with blueprintId, ncId, args), include version:
'<expectedVersion>' inside that objectContaining (i.e. expect.objectContaining({
blueprintId: 'blueprint123', ncId: 'nc123', args: [], version: 'vX' })), and
mirror the same explicit assertion in the second assertion block around lines
250-264 so the test fails if version is dropped.
In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts`:
- Line 455: The test uses a generic await expect(sendNanoContractTx(rpcRequest,
wallet, {}, promptHandler)).rejects.toThrow() which hides unrelated failures;
update both occurrences to assert a specific error type or message (e.g.,
.rejects.toThrowError(/expected message/) or
.rejects.toBeInstanceOf(ExpectedErrorClass) ) that matches the error
sendNanoContractTx is supposed to raise for the given
rpcRequest/wallet/promptHandler inputs; replace the generic toThrow() with these
specific assertions to ensure the test fails only for the intended error cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a452a937-185b-4dae-ba34-09fccbd0175b
📒 Files selected for processing (3)
packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.tspackages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.tspackages/hathor-rpc-handler/src/types/prompt.ts
raul-oliveira
commented
Mar 6, 2026
| network: string, | ||
| method: string, | ||
| blueprintId: string, | ||
| blueprintId: string | null, |
Contributor
Author
There was a problem hiding this comment.
This update was made to match the current implementation where blueprint isn't always defined.
raul-oliveira
commented
Mar 6, 2026
| method: string; | ||
| blueprint_id: string; | ||
| nc_id: string | null; | ||
| blueprint_id?: string | null; |
Contributor
Author
There was a problem hiding this comment.
Same here. The interface was missing some stuff that we are already using, so I updated it to match the usage.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts (1)
361-366:⚠️ Potential issue | 🟡 MinorReturn fully shaped trigger responses in rejection mocks.
These mocked branches should include
typeto stay faithful toTriggerResponseand avoid masking dispatch/type-guard regressions.🔧 Proposed fix
promptHandler.mockImplementation((prompt: CreateNanoContractCreateTokenTxConfirmationPrompt) => { if (prompt.type === TriggerTypes.CreateNanoContractCreateTokenTxConfirmationPrompt) { capturedPrompt = prompt; - return { data: { accepted: false } }; // Reject to end early + return { + type: TriggerResponseTypes.CreateNanoContractCreateTokenTxConfirmationResponse, + data: { accepted: false }, + }; // Reject to end early } });Also applies to: 392-396
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts` around lines 361 - 366, The mock implementation for promptHandler (the branch handling CreateNanoContractCreateTokenTxConfirmationPrompt) returns an object missing the required TriggerResponse shape; update the rejection mocks returned from promptHandler.mockImplementation to include the prompt `type` field and any other required TriggerResponse properties so they match the TriggerResponse contract (for the CreateNanoContractCreateTokenTxConfirmationPrompt branch that sets capturedPrompt) and do the same for the other mocked branch around lines 392-396 to avoid masking dispatch/type-guard regressions.packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts (1)
127-130:⚠️ Potential issue | 🟠 MajorStrengthen caller address validation before using
.trim().The current cast can still crash on malformed responses (
addressmissing/non-string), bypassing your domain error path.🔧 Proposed fix
- const tempCallerResult = await wallet.getCurrentAddress() as { address: string }; - if (!tempCallerResult || tempCallerResult.address.trim() === '') { + const tempCallerResult = await wallet.getCurrentAddress(); + if ( + !tempCallerResult || + typeof tempCallerResult !== 'object' || + typeof (tempCallerResult as { address?: unknown }).address !== 'string' || + (tempCallerResult as { address: string }).address.trim() === '' + ) { throw new SendNanoContractTxError('Unable to get current wallet address'); } - const tempCallerAddress = tempCallerResult.address; + const tempCallerAddress = (tempCallerResult as { address: string }).address;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts` around lines 127 - 130, The code casts wallet.getCurrentAddress() to { address: string } and then calls .trim(), which can throw if the response is malformed; update the validation around tempCallerResult in sendNanoContractTx.ts to explicitly check that tempCallerResult is an object and that typeof tempCallerResult.address === 'string' before calling .trim(), and throw SendNanoContractTxError('Unable to get current wallet address') if the address is missing, not a string, or empty after trimming (referencing wallet.getCurrentAddress, tempCallerResult, and SendNanoContractTxError).
🧹 Nitpick comments (3)
packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts (1)
40-40: Add an explicit assertion thatversionis forwarded to pre-build token options.The test data sets
version, but current expectations don’t verify it is passed tocreateNanoContractCreateTokenTransaction.Also applies to: 176-187, 237-252
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts` at line 40, Add an assertion that the provided version value is forwarded into the pre-build token options passed to createNanoContractCreateTokenTransaction: after invoking the RPC test, inspect the mock call arguments of createNanoContractCreateTokenTransaction and assert that the token options include version === 1 (or the test's version value). Repeat the same explicit assertion for the other test cases covering the same flow so each verifies the mock was called with tokenOptions.version set to the input version.packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts (1)
100-101: Add an explicit comment about intentional UTXO locking in pre-build.A brief note here will prevent future regressions where someone reintroduces
lockUtxos: falseby mistake.Based on learnings: pre-build in RPC nano methods is intentionally allowed to lock UTXOs and should not override this behavior with
lockUtxos: false.Also applies to: 108-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts` around lines 100 - 101, Add a short inline comment near the pre-build configuration indicating that UTXO locking is intentional for RPC nano pre-builds so it must not be overridden with lockUtxos: false; update the comment around the preBuildData object (in createNanoContractCreateTokenTx) and the similar block at the later pre-build (lines referenced 108-114) to explicitly state "intentional UTXO locking for pre-build — do not set lockUtxos: false" so future maintainers do not remove or negate this behavior.packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts (1)
126-127: Document that pre-build may intentionally lock UTXOs.Given the project decision, add a short inline comment near the pre-build call clarifying that
signTx: falsepre-build is expected to lock UTXOs and must not forcelockUtxos: false.Based on learnings: the pre-build step is intentionally allowed to lock UTXOs from this phase onward and should not override it with
lockUtxos: false.Also applies to: 140-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts` around lines 126 - 127, Add a short inline comment adjacent to the pre-build invocation (near tempCallerResult / the call that passes signTx: false) documenting that this pre-build intentionally locks UTXOs and must not be overridden with lockUtxos: false; repeat the same explanatory comment for the other pre-build use in the later block (the second pre-build call around the 140-147 region) so readers know signTx: false pre-builds are allowed to lock UTXOs from this phase onward.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts`:
- Around line 163-168: The code calls confirmedCaller.trim() without ensuring
confirmedCaller is a string, which can cause an unhandled runtime error for
malformed confirmation payloads; update the guard in the
createNanoContractCreateTokenTx flow to first check that typeof confirmedCaller
=== 'string' (or coerce safely) before calling .trim(), and if it's missing or
not a string or is an empty string after trimming, throw SendNanoContractTxError
with the existing message; locate the check around nano.caller / confirmedCaller
and replace the current falsy/trim check with the type-aware validation to
ensure only strings are trimmed and all other cases produce the controlled
SendNanoContractTxError.
---
Duplicate comments:
In
`@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts`:
- Around line 361-366: The mock implementation for promptHandler (the branch
handling CreateNanoContractCreateTokenTxConfirmationPrompt) returns an object
missing the required TriggerResponse shape; update the rejection mocks returned
from promptHandler.mockImplementation to include the prompt `type` field and any
other required TriggerResponse properties so they match the TriggerResponse
contract (for the CreateNanoContractCreateTokenTxConfirmationPrompt branch that
sets capturedPrompt) and do the same for the other mocked branch around lines
392-396 to avoid masking dispatch/type-guard regressions.
In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts`:
- Around line 127-130: The code casts wallet.getCurrentAddress() to { address:
string } and then calls .trim(), which can throw if the response is malformed;
update the validation around tempCallerResult in sendNanoContractTx.ts to
explicitly check that tempCallerResult is an object and that typeof
tempCallerResult.address === 'string' before calling .trim(), and throw
SendNanoContractTxError('Unable to get current wallet address') if the address
is missing, not a string, or empty after trimming (referencing
wallet.getCurrentAddress, tempCallerResult, and SendNanoContractTxError).
---
Nitpick comments:
In
`@packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.ts`:
- Line 40: Add an assertion that the provided version value is forwarded into
the pre-build token options passed to createNanoContractCreateTokenTransaction:
after invoking the RPC test, inspect the mock call arguments of
createNanoContractCreateTokenTransaction and assert that the token options
include version === 1 (or the test's version value). Repeat the same explicit
assertion for the other test cases covering the same flow so each verifies the
mock was called with tokenOptions.version set to the input version.
In
`@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts`:
- Around line 100-101: Add a short inline comment near the pre-build
configuration indicating that UTXO locking is intentional for RPC nano
pre-builds so it must not be overridden with lockUtxos: false; update the
comment around the preBuildData object (in createNanoContractCreateTokenTx) and
the similar block at the later pre-build (lines referenced 108-114) to
explicitly state "intentional UTXO locking for pre-build — do not set lockUtxos:
false" so future maintainers do not remove or negate this behavior.
In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts`:
- Around line 126-127: Add a short inline comment adjacent to the pre-build
invocation (near tempCallerResult / the call that passes signTx: false)
documenting that this pre-build intentionally locks UTXOs and must not be
overridden with lockUtxos: false; repeat the same explanatory comment for the
other pre-build use in the later block (the second pre-build call around the
140-147 region) so readers know signTx: false pre-builds are allowed to lock
UTXOs from this phase onward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c348a41-6f08-4345-a2ee-4a737490ea26
📒 Files selected for processing (3)
packages/hathor-rpc-handler/__tests__/rpcMethods/createNanoContractCreateTokenTx.test.tspackages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.tspackages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts
pedroferreira1
requested changes
Mar 6, 2026
github-project-automation
bot
moved this from In Progress (WIP)
to In Review (WIP)
in Hathor Network
raul-oliveira
commented
Mar 6, 2026
| // Ensure mintAddress has a value (required by wallet-lib's CreateTokenOptionsInput) | ||
| const tokenOptions: CreateTokenOptionsInput = { | ||
| ...token, | ||
| mintAddress: token.mintAddress ?? address, |
Contributor
Author
There was a problem hiding this comment.
@pedroferreira1 here is the answer for this comment. It was using the address by default in the previous implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts (1)
433-437:⚠️ Potential issue | 🟡 MinorReturn a typed confirmation response in this rejection mock branch.
The current branch omits
type, which weakens TriggerResponse contract fidelity for this path.🧪 Proposed fix
- return { data: { accepted: false } }; // Reject to end early + return { + type: TriggerResponseTypes.SendNanoContractTxConfirmationResponse, + data: { accepted: false }, + }; // Reject to end early🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts` around lines 433 - 437, The rejection mock in promptHandler.mockImplementation for SendNanoContractTxConfirmationPrompt returns an untyped object; update the returned TriggerResponse to include the correct type field so it conforms to the TriggerResponse contract (e.g., return an object with type: TriggerTypes.SendNanoContractTxConfirmationPrompt and data: { accepted: false }). Locate the mock implementation referencing SendNanoContractTxConfirmationPrompt and TriggerTypes.SendNanoContractTxConfirmationPrompt and change the rejection branch to return the typed confirmation response.packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts (1)
163-169:⚠️ Potential issue | 🟠 MajorGuard
confirmationResponse.data.nano.callertype before trimming.This branch can still throw a raw runtime error if
nanois malformed orcalleris not a string.🔧 Proposed fix
- const { nano } = confirmationResponse.data; - const confirmedCaller = nano.caller; + const confirmedCaller = confirmationResponse.data?.nano?.caller; // Validate caller is present (defensive check against malformed responses) - if (!confirmedCaller || confirmedCaller.trim() === '') { + if (typeof confirmedCaller !== 'string' || confirmedCaller.trim() === '') { throw new SendNanoContractTxError('Missing or empty nano caller in confirmation response'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts` around lines 163 - 169, The code currently assumes confirmationResponse.data.nano.caller is a string and calls trim on it; first guard the type by verifying that confirmationResponse?.data?.nano?.caller is a string (e.g., typeof confirmedCaller === 'string') before calling trim, and if it's missing or not a string throw the existing SendNanoContractTxError with the same message; update the check around confirmedCaller (and the assignment from confirmationResponse.data.nano.caller) to perform the type-safe validation to avoid raw runtime errors when nano or caller is malformed.
🧹 Nitpick comments (1)
packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts (1)
121-121: Document the intentional UTXO-locking behavior in pre-build options.A short inline note near
signTx: falsewould reduce future regressions that try to addlockUtxos: false.Based on learnings: In
packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts, the pre-build call withsignTx: falseis intentionally allowed to lock UTXOs, and addinglockUtxos: falseis not desired.Also applies to: 137-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts` at line 121, Add an inline comment documenting that the pre-build call in sendNanoContractTx intentionally uses signTx: false and therefore should allow UTXO locking (do not add lockUtxos: false); update the comment immediately next to the signTx: false option in the pre-build options block inside sendNanoContractTx and replicate the same explanatory note for the second pre-build call around lines 137-141 so future changes don't disable UTXO locking by accident.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts`:
- Around line 183-189: Guard extraction of the confirmation caller before
calling .trim(): ensure sendNanoContractTxResponse and its nested .data.nc exist
and that confirmedCaller is a string (use typeof confirmedCaller === 'string' or
similar) before invoking .trim(), and if those checks fail throw the existing
SendNanoContractTxError('Missing or empty nano caller in confirmation
response'); update the block around sendNanoContractTxResponse, confirmedCaller
to perform these null/type checks so raw runtime errors cannot bypass the domain
error path.
---
Duplicate comments:
In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.ts`:
- Around line 433-437: The rejection mock in promptHandler.mockImplementation
for SendNanoContractTxConfirmationPrompt returns an untyped object; update the
returned TriggerResponse to include the correct type field so it conforms to the
TriggerResponse contract (e.g., return an object with type:
TriggerTypes.SendNanoContractTxConfirmationPrompt and data: { accepted: false
}). Locate the mock implementation referencing
SendNanoContractTxConfirmationPrompt and
TriggerTypes.SendNanoContractTxConfirmationPrompt and change the rejection
branch to return the typed confirmation response.
In
`@packages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.ts`:
- Around line 163-169: The code currently assumes
confirmationResponse.data.nano.caller is a string and calls trim on it; first
guard the type by verifying that confirmationResponse?.data?.nano?.caller is a
string (e.g., typeof confirmedCaller === 'string') before calling trim, and if
it's missing or not a string throw the existing SendNanoContractTxError with the
same message; update the check around confirmedCaller (and the assignment from
confirmationResponse.data.nano.caller) to perform the type-safe validation to
avoid raw runtime errors when nano or caller is malformed.
---
Nitpick comments:
In `@packages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.ts`:
- Line 121: Add an inline comment documenting that the pre-build call in
sendNanoContractTx intentionally uses signTx: false and therefore should allow
UTXO locking (do not add lockUtxos: false); update the comment immediately next
to the signTx: false option in the pre-build options block inside
sendNanoContractTx and replicate the same explanatory note for the second
pre-build call around lines 137-141 so future changes don't disable UTXO locking
by accident.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a13994b1-d426-4450-b6d8-ab5504696d33
📒 Files selected for processing (5)
packages/hathor-rpc-handler/__tests__/rpcMethods/sendNanoContractTx.test.tspackages/hathor-rpc-handler/src/rpcMethods/createNanoContractCreateTokenTx.tspackages/hathor-rpc-handler/src/rpcMethods/sendNanoContractTx.tspackages/hathor-rpc-handler/src/rpcRequest/sendNanoContractTx.tspackages/hathor-rpc-handler/src/types/rpcRequest.ts
| mintAddress: token.mintAddress ?? address, | ||
| }; | ||
| // Validate caller is present (defensive check against malformed responses) | ||
| if (!confirmedCaller || confirmedCaller.trim() === '') { |
Collaborator
There was a problem hiding this comment.
We're using zod up there, why don't we add this validation there?
| "@types/eslint__js": "8.42.3", | ||
| "@types/jest": "29.5.12", | ||
| "@types/node": "20.14.2", | ||
| "@types/node": "^22.0.0", |
Collaborator
There was a problem hiding this comment.
Where? Did you forget to commit?
pedroferreira1
previously approved these changes
Mar 6, 2026
Comment on lines
+136
to
+184
| const { | ||
| caller, | ||
| blueprintId: confirmedBluePrintId, | ||
| actions: confirmedActions, | ||
| args: confirmedArgs, | ||
| } = sendNanoContractTxResponse.data.nc; | ||
| // Validate nano response has valid caller | ||
| const nanoValidation = nanoContractResponseWithCallerSchema.safeParse(sendNanoContractTxResponse.data.nc); |
Collaborator
There was a problem hiding this comment.
The entire sendNanoContractTxResponse object comes from the client, why validate only data.nc and not the entire response??
Contributor
Author
There was a problem hiding this comment.
At a first glance I was looking only for the data we need in the method. However, your point is good.
It's done in dd0f3af
raul-oliveira
force-pushed
the
raul-oliveira/feat-fee-token-creation
branch
from
70ec120 to
1c7b4af
Compare
pedroferreira1
approved these changes
Mar 6, 2026
andreabadesso
approved these changes
Mar 6, 2026
fix: wallet-lib new version props feat: createNanoContactTransaction and createNanoContractCreateTokenTransaction tests: nano contract without sign feat: contract pays fees chore: adds yalc to gitignore review changes: change fee return and signTx method review changes: code rabbit review test: check for preparedTx in the prompt fix: remove yalc references to wallet-lib review changes: change error objetc fix: remove prepared tx from confirmation responses and tests for ensure default values fix: remove duplicated sign call feat: add extra guard to safe caller fix: removes unecessary filter review changes: remove options prop from rpc interface feat: fee for create token tx refact: removed unecessary object spread operator review changes: add caller validation with zod schema review changes: use fixed version review changes: validating entire user response fix: zod schemas to validate the confirmation response
raul-oliveira
force-pushed
the
raul-oliveira/feat-fee-token-creation
branch
from
1c7b4af to
63937cf
Compare
github-project-automation
bot
moved this from In Review (WIP)
to Waiting to be deployed
in Hathor Network
3 tasks
3 tasks
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
3 participants
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.
Important
This pr should be merged only after the
@hathor/wallet-libfee is published.Motivation
Introduces the fee based token creation.
Acceptance Criteria
Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets mergedSummary by CodeRabbit
New Features
Tests
Chores