feat: create nano contract tx without signing#1023
Merged
raul-oliveira merged 14 commits intomasterfrom Mar 5, 2026
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence DiagramsequenceDiagram
participant Client
participant Wallet
participant Builder as NanoContractTransactionBuilder
participant Signer
participant Blockchain
rect rgba(100, 150, 200, 0.5)
Note over Client,Wallet: Build unsigned (signTx: false)
Client->>Wallet: createNanoContractTransaction(signTx: false)
Wallet->>Builder: build()
Builder-->>Wallet: Transaction (unsigned)
Wallet-->>Client: SendTransaction (unsigned, no PIN)
end
rect rgba(150, 100, 200, 0.5)
Note over Client,Signer: Edit caller and sign
Client->>Client: Edit nano header (caller, seqnum)
Client->>Wallet: request signing (provide PIN)
Wallet->>Signer: prepareToSend/sign()
Signer-->>Wallet: Signed transaction
Wallet-->>Client: Prepared SendTransaction (signed)
end
rect rgba(100, 200, 150, 0.5)
Note over Client,Blockchain: Submit
Client->>Blockchain: send signed transaction
Blockchain-->>Client: confirmation / inclusion
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/wallet/wallet.ts (1)
2926-2927: AlignpinCodetyping with deferred-sign runtime behavior.Line 2926 requires
pinCode: string, but Line 2849 passes an optional value (pin!) that is intentionally absent whensignTxis false. Making this parameter nullable/optional removes the non-null assertion and keeps the contract accurate.Proposed type-safe adjustment
- async prepareNanoSendTransactionWalletService( + async prepareNanoSendTransactionWalletService( tx: Transaction, address: string, - pinCode: string, + pinCode?: string | null, options: { signTx?: boolean } = { signTx: true } ): Promise<SendTransactionWalletService> { @@ - const sendTransaction = new SendTransactionWalletService(this, { - transaction: tx, - pin: pinCode, - }); + const sendTransaction = new SendTransactionWalletService(this, { + transaction: tx, + pin: pinCode ?? undefined, + });Also applies to: 2849-2851
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.ts` around lines 2926 - 2927, The function currently declares pinCode: string but callers sometimes pass pin! only when options.signTx is true; change the parameter to be optional or nullable (pinCode?: string or pinCode: string | undefined) and update the function body (the code paths in the function that reference pinCode) to only require a non-null pin when options.signTx is true (throw or assert if signTx true and pinCode is missing); also update any internal usages to guard against undefined and remove the non-null assertion at the caller site (the call around lines 2849–2851) so the runtime matches the deferred-sign behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/wallet.ts`:
- Around line 2849-2851: The code is calling tx.prepareToSend() and passing a
non-null asserted pin (pin!) even for deferred-sign flows (signTx === false),
which finalizes the tx too early and violates the helper's non-null pin
contract; update the logic around prepareNanoSendTransactionWalletService and
tx.prepareToSend(): only call tx.prepareToSend() after signatures are injected
(i.e., when signTx is true or after external signing completes) and remove the
unconditional prepareToSend() from the deferred path (also apply the same change
in src/new/wallet.ts where tx.prepareToSend() is called at line ~3101); fix the
pin contract by changing the call to prepareNanoSendTransactionWalletService to
pass a nullable pin (pass pin or undefined/null) or update
prepareNanoSendTransactionWalletService's signature to accept pin?: string |
null so you don't use pin! when signTx is false. Ensure all branches that
perform external signing re-finalize the transaction after signature injection.
---
Nitpick comments:
In `@src/wallet/wallet.ts`:
- Around line 2926-2927: The function currently declares pinCode: string but
callers sometimes pass pin! only when options.signTx is true; change the
parameter to be optional or nullable (pinCode?: string or pinCode: string |
undefined) and update the function body (the code paths in the function that
reference pinCode) to only require a non-null pin when options.signTx is true
(throw or assert if signTx true and pinCode is missing); also update any
internal usages to guard against undefined and remove the non-null assertion at
the caller site (the call around lines 2849–2851) so the runtime matches the
deferred-sign behavior.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
__tests__/integration/template/transaction/fee.test.tssrc/nano_contracts/types.tssrc/new/wallet.tssrc/wallet/types.tssrc/wallet/wallet.ts
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1023 +/- ##
===========================================
+ Coverage 66.68% 87.82% +21.13%
===========================================
Files 114 114
Lines 8791 8818 +27
Branches 1994 2004 +10
===========================================
+ Hits 5862 7744 +1882
+ Misses 2901 1046 -1855
Partials 28 28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as spam.
This comment was marked as spam.
r4mmer
previously approved these changes
Feb 27, 2026
tuliomir
reviewed
Feb 28, 2026
raul-oliveira
force-pushed
the
feat/build-tx-without-sign-and-lock
branch
from
a9b8260 to
e5dc76f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/wallet/wallet.ts (1)
2849-2851:⚠️ Potential issue | 🟠 MajorType contract mismatch:
pin!may be null whensignTxis false.When
signTxis false, the PIN validation is skipped (lines 2808-2810), sopincan benull. However,prepareNanoSendTransactionWalletServicedeclarespinCode: string(non-nullable at line 2926). The non-null assertionpin!masks a runtime type violation.Either update
prepareNanoSendTransactionWalletServiceto acceptpinCode: string | null, or pass a safe fallback value when signing is deferred.🔧 Suggested fix: Update helper signature to accept nullable pin
At line 2926:
async prepareNanoSendTransactionWalletService( tx: Transaction, address: string, - pinCode: string, + pinCode: string | null, options: { signTx?: boolean } = { signTx: true } ): Promise<SendTransactionWalletService> {Then at line 2942, guard before using:
if (options.signTx) { + if (!pinCode) { + throw new PinRequiredError('Pin is required for signing.'); + } await transaction.signTransaction(tx, storageProxy.createProxy(), pinCode);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.ts` around lines 2849 - 2851, The call currently passes pin! into prepareNanoSendTransactionWalletService but pin may be null when newOptions.signTx is false; update prepareNanoSendTransactionWalletService's signature to accept pinCode: string | null (or undefined) and then guard uses of pinCode inside prepareNanoSendTransactionWalletService (e.g., where signing or PIN validation occurs) to only access it when non-null, or alternatively pass a safe fallback when calling from the caller that knows signTx is false; reference prepareNanoSendTransactionWalletService, the pin variable at the caller, and newOptions.signTx to implement the chosen fix.
🧹 Nitpick comments (1)
src/wallet/wallet.ts (1)
2807-2810: Consider usingsignTx !== falsepattern for consistency.The current check
newOptions.signTx === truediffers from the pattern used insrc/new/wallet.ts. Using!== falseguards against invalid inputs likenullor0and maintains consistency across wallet implementations.♻️ Suggested change
- if (newOptions.signTx === true && !pin) { + if (newOptions.signTx !== false && !pin) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.ts` around lines 2807 - 2810, Update the conditional that gates requiring a PIN in wallet.ts to use the same pattern as other implementations: replace the check `newOptions.signTx === true` with `newOptions.signTx !== false` so the PIN is required for any truthy or unspecified values and only skipped when explicitly false; update the `if` that throws `PinRequiredError('Pin is required.')` accordingly (target the block referencing newOptions.signTx and PinRequiredError).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wallet/wallet.ts`:
- Around line 2864-2873: The IHathorWallet interface's
createAndSendNanoContractTransaction signature doesn't match the implementation
in createAndSendNanoContractTransaction (method: string, address: string, data:
CreateNanoTxData, options: Omit<CreateNanoTxOptions, 'signTx'>):
Promise<Transaction>; — update the IHathorWallet declaration to use the same
optional options type (options?: Omit<CreateNanoTxOptions, 'signTx'>) and return
type Promise<Transaction>, and ensure relevant types (CreateNanoTxOptions,
CreateNanoTxData, Transaction) are imported/available in the interface file so
the signature matches the implementation.
---
Duplicate comments:
In `@src/wallet/wallet.ts`:
- Around line 2849-2851: The call currently passes pin! into
prepareNanoSendTransactionWalletService but pin may be null when
newOptions.signTx is false; update prepareNanoSendTransactionWalletService's
signature to accept pinCode: string | null (or undefined) and then guard uses of
pinCode inside prepareNanoSendTransactionWalletService (e.g., where signing or
PIN validation occurs) to only access it when non-null, or alternatively pass a
safe fallback when calling from the caller that knows signTx is false; reference
prepareNanoSendTransactionWalletService, the pin variable at the caller, and
newOptions.signTx to implement the chosen fix.
---
Nitpick comments:
In `@src/wallet/wallet.ts`:
- Around line 2807-2810: Update the conditional that gates requiring a PIN in
wallet.ts to use the same pattern as other implementations: replace the check
`newOptions.signTx === true` with `newOptions.signTx !== false` so the PIN is
required for any truthy or unspecified values and only skipped when explicitly
false; update the `if` that throws `PinRequiredError('Pin is required.')`
accordingly (target the block referencing newOptions.signTx and
PinRequiredError).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/new/wallet.tssrc/wallet/wallet.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/new/wallet.ts
Comment on lines
2864
to
+2873
| async createAndSendNanoContractTransaction( | ||
| method: string, | ||
| address: string, | ||
| data: CreateNanoTxData, | ||
| options: CreateNanoTxOptions = {} | ||
| options: Omit<CreateNanoTxOptions, 'signTx'> = {} | ||
| ): Promise<Transaction> { | ||
| const sendTransaction = await this.createNanoContractTransaction( | ||
| method, | ||
| address, | ||
| data, | ||
| options | ||
| ); | ||
| const sendTransaction = await this.createNanoContractTransaction(method, address, data, { | ||
| ...options, | ||
| signTx: true, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts -A5 'createAndSendNanoContractTransaction' src/wallet/types.tsRepository: HathorNetwork/hathor-wallet-lib
Length of output: 269
Interface mismatch with IHathorWallet.createAndSendNanoContractTransaction.
The implementation declares options: Omit<CreateNanoTxOptions, 'signTx'> but the IHathorWallet interface in src/wallet/types.ts (line 432) declares options?: { pinCode?: string }. Update the interface to match:
createAndSendNanoContractTransaction(
method: string,
address: string,
data: CreateNanoTxData,
options?: Omit<CreateNanoTxOptions, 'signTx'>
): Promise<Transaction>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wallet/wallet.ts` around lines 2864 - 2873, The IHathorWallet interface's
createAndSendNanoContractTransaction signature doesn't match the implementation
in createAndSendNanoContractTransaction (method: string, address: string, data:
CreateNanoTxData, options: Omit<CreateNanoTxOptions, 'signTx'>):
Promise<Transaction>; — update the IHathorWallet declaration to use the same
optional options type (options?: Omit<CreateNanoTxOptions, 'signTx'>) and return
type Promise<Transaction>, and ensure relevant types (CreateNanoTxOptions,
CreateNanoTxData, Transaction) are imported/available in the interface file so
the signature matches the implementation.
Contributor
There was a problem hiding this comment.
@raul-oliveira : this is important, and is being tracked by the breaking changes described in HathorNetwork/rfcs#100
I think it doesn't demand any action here , but it's good to keep in mind anyway.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/wallet/wallet.ts (1)
2844-2846:⚠️ Potential issue | 🟠 MajorRemove the unsafe
pin!contract in deferred-sign flows.Line 2808 allows unsigned creation without a PIN, but Line 2844 still forces
pin!into a method that requiresstringat Line 2921. This masks a nullable path and weakens the runtime/type contract.🔧 Proposed fix
- return this.prepareNanoSendTransactionWalletService(tx, address, pin!, { + return this.prepareNanoSendTransactionWalletService(tx, address, pin, { signTx: newOptions.signTx, }); } async prepareNanoSendTransactionWalletService( tx: Transaction, address: string, - pinCode: string, + pinCode: string | null, options: { signTx?: boolean } = { signTx: true } ): Promise<SendTransactionWalletService> { @@ - if (options.signTx) { + if (options.signTx) { + if (!pinCode) { + throw new PinRequiredError('Pin is required.'); + } await transaction.signTransaction(tx, storageProxy.createProxy(), pinCode); // Finalize the transaction tx.prepareToSend(); } const sendTransaction = new SendTransactionWalletService(this, { transaction: tx, - pin: pinCode, + pin: pinCode ?? undefined, });#!/bin/bash set -euo pipefail echo "== Deferred-sign PIN flow and non-null assertions ==" rg -n --type=ts -C3 'signTx|prepareNanoSendTransactionWalletService|pin!' src/wallet/wallet.ts src/new/wallet.ts echo "== SendTransactionWalletService PIN typing/usage ==" rg -n --type=ts -C4 'class SendTransactionWalletService|constructor\\(|pin\\??:' src/wallet/sendTransactionWalletService.tsAlso applies to: 2919-2923
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/wallet.ts` around lines 2844 - 2846, The call to prepareNanoSendTransactionWalletService at prepareNanoSendTransactionWalletService(tx, address, pin!, { signTx: newOptions.signTx }) unsafely force-unwraps pin; remove the non-null assertion and thread the nullable PIN correctly through the deferred-sign flow: change the call to pass pin (nullable) and update prepareNanoSendTransactionWalletService / SendTransactionWalletService constructors and the method that requires a string PIN (referenced around the pin usage at lines ~2921 and the class SendTransactionWalletService) to accept pin?: string and handle the unsigned/signTx case (throw or skip signing) so no nullable path is masked by a non-null assertion.
🧹 Nitpick comments (1)
__tests__/integration/hathorwallet_prepare_without_sign.test.ts (1)
75-79: Avoid stopping the same wallet twice in teardown.
generateWalletHelpertracks created wallets, andstopAllWallets()already stops them. CallinghWallet.stop()first can create noisy double-stop behavior.♻️ Suggested cleanup
afterAll(async () => { - await hWallet.stop(); await stopAllWallets(); await GenesisWalletHelper.clearListeners(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/hathorwallet_prepare_without_sign.test.ts` around lines 75 - 79, The teardown calls both hWallet.stop() and stopAllWallets(), causing duplicate stops for wallets tracked by generateWalletHelper; remove the explicit await hWallet.stop() from the afterAll block (leave await stopAllWallets() and await GenesisWalletHelper.clearListeners()) so wallets are only stopped once, or alternatively register hWallet with generateWalletHelper instead of calling hWallet.stop() directly; update the afterAll in __tests__/integration/hathorwallet_prepare_without_sign.test.ts accordingly to reference hWallet.stop and stopAllWallets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/wallet/wallet.ts`:
- Around line 2844-2846: The call to prepareNanoSendTransactionWalletService at
prepareNanoSendTransactionWalletService(tx, address, pin!, { signTx:
newOptions.signTx }) unsafely force-unwraps pin; remove the non-null assertion
and thread the nullable PIN correctly through the deferred-sign flow: change the
call to pass pin (nullable) and update prepareNanoSendTransactionWalletService /
SendTransactionWalletService constructors and the method that requires a string
PIN (referenced around the pin usage at lines ~2921 and the class
SendTransactionWalletService) to accept pin?: string and handle the
unsigned/signTx case (throw or skip signing) so no nullable path is masked by a
non-null assertion.
---
Nitpick comments:
In `@__tests__/integration/hathorwallet_prepare_without_sign.test.ts`:
- Around line 75-79: The teardown calls both hWallet.stop() and
stopAllWallets(), causing duplicate stops for wallets tracked by
generateWalletHelper; remove the explicit await hWallet.stop() from the afterAll
block (leave await stopAllWallets() and await
GenesisWalletHelper.clearListeners()) so wallets are only stopped once, or
alternatively register hWallet with generateWalletHelper instead of calling
hWallet.stop() directly; update the afterAll in
__tests__/integration/hathorwallet_prepare_without_sign.test.ts accordingly to
reference hWallet.stop and stopAllWallets.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
__tests__/integration/hathorwallet_prepare_without_sign.test.tssrc/new/wallet.tssrc/wallet/wallet.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/new/wallet.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/new/wallet.ts`:
- Around line 3107-3114: createAndSendNanoContractTransaction must not accept
signTx: false because createNanoContractTransaction can return an unsigned
SendTransaction while createAndSendNanoContractTransaction always calls
runFromMining(); update createAndSendNanoContractTransaction to validate
newOptions.signTx and either reject/throw when signTx === false or coerce it to
true before calling createNanoContractTransaction so the returned object is
always signed, and add a clear error message referencing
createAndSendNanoContractTransaction, createNanoContractTransaction,
runFromMining, and prepareNanoSendTransaction/SendTransaction to make the
contract explicit.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/new/wallet.tssrc/wallet/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/wallet/types.ts
pedroferreira1
previously approved these changes
Mar 3, 2026
pedroferreira1
requested changes
Mar 4, 2026
| ) | ||
| ).rejects.toThrow(/exceeds maximum fee/); | ||
| }); | ||
| it('should create a fee token and deposit htr into the contract', async () => { |
Member
There was a problem hiding this comment.
This test is not related with the template test file
Contributor
Author
There was a problem hiding this comment.
I'll open another pr to address it. we have some tests that aren't using the template but are related to fees.
raul-oliveira
force-pushed
the
feat/build-tx-without-sign-and-lock
branch
5 times, most recently
from
ce94a20 to
c0001db
Compare
tuliomir
reviewed
Mar 5, 2026
raul-oliveira
force-pushed
the
feat/build-tx-without-sign-and-lock
branch
from
c0001db to
6044a14
Compare
tuliomir
previously approved these changes
Mar 5, 2026
pedroferreira1
requested changes
Mar 5, 2026
| return addressInfo.seqnum + 1; | ||
| } | ||
|
|
||
| async setNanoHeaderCaller(nanoHeader: NanoContractHeader, address: string): Promise<void> { |
src/new/wallet.ts
Outdated
| * @param tx - The transaction to be signed | ||
| * @param options - Options for signing | ||
| * @param options.pinCode - PIN to decrypt the private key. Optional but required if not set in this | ||
| * @param pinCode - PIN to decrypt the private key. |
Member
There was a problem hiding this comment.
Why? The pinCode is inside the options parameter
Comment on lines
+1620
to
+1630
| async setNanoHeaderCaller(nanoHeader: NanoContractHeader, address: string): Promise<void> { | ||
| const newAddress = new Address(address, { network: this.network }); | ||
|
|
||
| newAddress.validateAddress(); | ||
|
|
||
| const newCallerSeqnum = await this.getNanoHeaderSeqnum(address); | ||
| // eslint-disable-next-line no-param-reassign | ||
| nanoHeader.address = newAddress; | ||
| // eslint-disable-next-line no-param-reassign | ||
| nanoHeader.seqnum = newCallerSeqnum; | ||
| } |
Member
There was a problem hiding this comment.
Both methods are exactly the same. We should create a util method with this that just receives the nanoHeader, address, wallet. That way we can remove the duplicated code
Comment on lines
+3081
to
+3087
| const storageProxy = new WalletServiceStorageProxy(this, this.storage); | ||
| const signedTx = await transaction.signTransaction( | ||
| tx, | ||
| storageProxy.createProxy(), | ||
| options.pinCode | ||
| ); | ||
| signedTx.prepareToSend(); |
Member
There was a problem hiding this comment.
This part was inspired by the prepareNanoSendTransactionWalletService method of this same file. I think we could go to this method and just call this.signTx, removing the duplicated code
pedroferreira1
approved these changes
Mar 5, 2026
tuliomir
approved these changes
Mar 5, 2026
github-project-automation
bot
moved this from In Review (Done)
to Waiting to be deployed
in Hathor Network
This was referenced Mar 15, 2026
1 task
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
4 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.
Motivation
Rpc-lib handler needs to provide the fee header to the wallet-mobile, this will show the total fee amount to the user before accepting the tx. To avoid building the tx twice, for rpc-prompt and rpc-request, we'll do the following:
Acceptance Criteria
Security Checklist
Summary by CodeRabbit
New Features
Behavior
Tests