Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts Symbol transaction creation and announcement utilities to make transaction parameters safer and to optionally wait for on-chain confirmation, while tightening input validation in NFC-based token transfer routes.
Changes:
- Add configurable
fee(and mosaicduration) to mosaic definition / supply change transaction builders. - Add optional “wait for confirmation” polling to
SignAndAnnounce, and return an announce result containing the transaction hash. - Harden Amount validation in the NFC flow and align transfer message parameter naming with
CreateTransferTx.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Backend/Workspace/Tools/SupplyMosaic.js | Adds fee, validates supply delta, and clamps deadline hours when creating supply-change transactions. |
| Backend/Workspace/Tools/SignAndAnnounce.js | Adds confirmation polling + structured return value; adds signature buffer checks; adjusts logging. |
| Backend/Workspace/Tools/CreateTransferTx.js | Clamps deadline hours when creating transfer transactions. |
| Backend/Workspace/Tools/CreateMosaicTx.js | Adds duration + fee, clamps deadline hours, and updates mosaic definition transaction fields accordingly. |
| Backend/Workspace/Routes/SendTokenByNFC.js | Validates Amount more strictly and updates transfer message param / amount handling. |
| Backend/Workspace/Routes/CreateRoom.js | Waits for mosaic definition + supply change confirmation and logs resulting hashes. |
Comments suppressed due to low confidence (1)
Backend/Workspace/Tools/SignAndAnnounce.js:120
- The function returns from inside the
tryblock (and re-throws incatch), so the "Shutdown" log and trailingreturnafter the try/catch will never execute. Consider moving the shutdown logging into afinallyblock and removing the unreachable code at the end.
return {
hash,
announced: true,
confirmed: shouldWaitForConfirmation
};
} catch (error) {
console.error(`[${logOwner}] Error:`, error);
throw error; // ルーター側のcatchで捕まえられるようにエラーを再スローします
}
// Shutdown Log
console.log(`[${logOwner}] Shutdown!`);
return;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fee: BigInt(fee), | ||
| duration: BigInt(duration), | ||
| nonce: nonce, |
There was a problem hiding this comment.
fee and duration are coerced via BigInt(...) but not validated. Passing a negative value (or a non-integer number) can create an invalid transaction or throw unexpectedly at runtime. Add input validation to ensure both are non-negative integers before building the transaction.
| @@ -1,10 +1,44 @@ | |||
| import { PrivateKey } from 'symbol-sdk'; | |||
| import { SymbolFacade } from 'symbol-sdk/symbol'; | |||
There was a problem hiding this comment.
SymbolFacade is imported but not used in this module. Please remove the unused import to avoid confusion and keep the dependency surface minimal.
| import { SymbolFacade } from 'symbol-sdk/symbol'; |
| const confirmedRes = await fetch(`${nodeUrl}/transactions/confirmed/${hash}`); | ||
| if (confirmedRes.ok) { | ||
| return { confirmed: true }; | ||
| } | ||
|
|
||
| const statusRes = await fetch(`${nodeUrl}/transactionStatus/${hash}`); | ||
| if (statusRes.ok) { | ||
| const statusJson = await statusRes.json(); | ||
| const code = statusJson?.code; | ||
| if (code && code !== 'Success') { | ||
| throw new Error(`Transaction rejected: ${code}`); | ||
| } |
There was a problem hiding this comment.
waitForConfirmation will abort immediately if fetch(...) throws (e.g., transient network/DNS/TLS error), instead of continuing to poll until timeoutMs. Wrap the polling HTTP calls in a try/catch (or handle non-HTTP failures) so temporary node connectivity issues don’t cause a false failure.
| const confirmedRes = await fetch(`${nodeUrl}/transactions/confirmed/${hash}`); | |
| if (confirmedRes.ok) { | |
| return { confirmed: true }; | |
| } | |
| const statusRes = await fetch(`${nodeUrl}/transactionStatus/${hash}`); | |
| if (statusRes.ok) { | |
| const statusJson = await statusRes.json(); | |
| const code = statusJson?.code; | |
| if (code && code !== 'Success') { | |
| throw new Error(`Transaction rejected: ${code}`); | |
| } | |
| try { | |
| const confirmedRes = await fetch(`${nodeUrl}/transactions/confirmed/${hash}`); | |
| if (confirmedRes.ok) { | |
| return { confirmed: true }; | |
| } | |
| const statusRes = await fetch(`${nodeUrl}/transactionStatus/${hash}`); | |
| if (statusRes.ok) { | |
| const statusJson = await statusRes.json(); | |
| const code = statusJson?.code; | |
| if (code && code !== 'Success') { | |
| throw new Error(`Transaction rejected: ${code}`); | |
| } | |
| } | |
| } catch (error) { | |
| // Network / transport / parsing errors: log and continue polling until timeout | |
| console.warn(`waitForConfirmation: transient error while polling ${hash} at ${nodeUrl}:`, error); |
|
|
||
| // Deadline 作成 (v3のfromDatetimeはDateオブジェクトを受け取ります) | ||
| const deadline = facade.network.fromDatetime(new Date()).addHours(Number(deadlineHours)).timestamp; | ||
| const safeDeadlineHours = Math.min(Math.max(Number(deadlineHours) || 2, 1), 2); |
There was a problem hiding this comment.
safeDeadlineHours uses Number(deadlineHours) || 2, which treats a valid 0 as "unset" and silently clamps any value > 2 down to 2. If the intent is to enforce a valid deadline range, it’s safer to explicitly validate with Number.isFinite(...) and either clamp with clear semantics (using ?? instead of ||) or throw when the caller passes an out-of-range value.
| const safeDeadlineHours = Math.min(Math.max(Number(deadlineHours) || 2, 1), 2); | |
| let parsedDeadlineHours = Number(deadlineHours); | |
| if (!Number.isFinite(parsedDeadlineHours) || parsedDeadlineHours === 0) { | |
| parsedDeadlineHours = 2; | |
| } | |
| const safeDeadlineHours = Math.min(Math.max(parsedDeadlineHours, 1), 2); |
| const safeDeadlineHours = Math.min(Math.max(Number(deadlineHours) || 2, 1), 2); | ||
| const deadline = facade.network | ||
| .fromDatetime(new Date()) | ||
| .addHours(Number(deadlineHours)) | ||
| .addHours(safeDeadlineHours) | ||
| .timestamp; |
There was a problem hiding this comment.
safeDeadlineHours currently clamps deadlineHours to a maximum of 2 and also uses Number(deadlineHours) || 2 (so 0 becomes 2). Because callers in this repo pass deadlineHours: 24 (e.g. CreateRoom), this will silently override the caller’s request. Consider either throwing on out-of-range inputs or clearly documenting/renaming the parameter to reflect that it will be capped.
| const safeDeadlineHours = Math.min(Math.max(Number(deadlineHours) || 2, 1), 2); | ||
| const deadline = facade.network | ||
| .fromDatetime(new Date()) | ||
| .addHours(Number(deadlineHours)) | ||
| .addHours(safeDeadlineHours) | ||
| .timestamp; |
There was a problem hiding this comment.
safeDeadlineHours clamps deadlineHours to a maximum of 2 and uses Number(deadlineHours) || 2, which silently overrides caller-provided values (including treating 0 as default). Consider explicit validation (e.g., Number.isFinite) and either throwing for out-of-range values or making the capping behavior explicit to callers.
| networkType = 'testnet', | ||
| senderPrivateKey, | ||
| transferable = true, | ||
| duration = 86400n, |
There was a problem hiding this comment.
The default mosaic duration changed from effectively unlimited (0n) to 86400n. This is a behavior change that will make newly created mosaics expire after a fixed duration by default. If expiration is intended, consider requiring callers to pass duration explicitly (or at least documenting the units and rationale); otherwise consider keeping the previous default of 0n.
| duration = 86400n, | |
| duration = 0n, // 0 = unlimited duration (no expiration) |
No description provided.