Conversation
f3d8f17 to
666e692
Compare
666e692 to
934e046
Compare
| export interface MoneroPrivateKeys { | ||
| moneroKey: string | ||
| moneroSpendKeyPrivate: string | ||
| moneroSpendKeyPublic: string |
There was a problem hiding this comment.
This matches what we had in the previous Monero repo, so that's good. Zcash and Piratechain store the birthday height in the keys, and Zano does too (embedded in the key), but Monero has not done so historically.
Edit: Ah, I see that we are adding the height as non-optional. This is bad. We should make the height ?: number in the type definition, so the type system forces us to handle the legacy undefined case explicitly. The asOptional(asNumber, 0) is a bit dangerous.
src/monero/MoneroEngine.ts
Outdated
|
|
||
| let birthdayHeight: number | ||
| try { | ||
| if (daemonAddress === EDGE_MONERO_LWS_SERVER) { |
There was a problem hiding this comment.
Should EDGE_MONERO_LWS_SERVER be placed in the neworkInfo, so we aren't comparing with this magic constant?
src/monero/MoneroEngine.ts
Outdated
| }, | ||
|
|
||
| onError: error => { | ||
| this.log.error('Wallet lifecycle error:', String(error)) |
There was a problem hiding this comment.
Could we make these strings include the word "Monero", like "Monero lifecycle error"? That would improve debugging friendliness.
| async resyncBlockchain(): Promise<void> { | ||
| await this.killEngine() | ||
| await this.clearBlockchainCache() | ||
| await this.tools.cppBridge.deleteWallet( |
There was a problem hiding this comment.
I popped over to the server side, and indeed: The deleteWallet method performs cppBridge.closeWallet functionality prior to doing the file removal. This means we are free from race conditions in this particular case. 👍
src/monero/MoneroEngine.ts
Outdated
| this.currentWalletSettings = newSettings | ||
| await this.killEngine() | ||
| await this.clearBlockchainCache() | ||
| await this.startEngine() |
There was a problem hiding this comment.
This will leave the existing backend as a leftover file on disk. We'd need to call this.tools.cppBridge.deleteWallet if we wanted to avoid that, but I suppose leaving the other file is harmless, and would make the cost of toggling back & forth a lot cheaper. Optional to fix, depending on whether we prefer easy switching or tidiness more.
There was a problem hiding this comment.
Leaving the other file was intentional for easy switching
934e046 to
6790578
Compare
paullinator
left a comment
There was a problem hiding this comment.
Additional Findings
- critical
package.json: devDependency edge-core-js pinned to beta version '2.42.1-beta.0'. Beta dependencies should not ship to production. The master branch uses '^2.42.0' (stable).- Upgrade to a stable edge-core-js release (>=2.42.1) or revert to '^2.42.0' before merging. If the beta has required API changes (walletSettings), wait for its stable release.
- critical
package.json: devDependency react-native-monero-lwsf is '0.1.0-beta.0' while peerDependency declares 'v0.1.0'. The 'v' prefix in the peerDependency semver string is non-standard and may cause npm/yarn resolution warnings. The dev version (beta) does not satisfy the peer version (stable).- Remove the 'v' prefix from the peerDependency version. Align both devDependency and peerDependency to the same stable release before merging.
- warning: PR has 14 commits including 4 unsquashed 'fixup!' commits and a typo ('new wallet's to save'). PR description says 'none' and the CHANGELOG checkbox is unchecked. This is a significant feature addition that warrants a proper description and CHANGELOG entry.
- Squash fixup commits, add a meaningful PR description, and add a CHANGELOG entry for Monero support.
- suggestion: No Monero fixtures in test/plugin/ or test/engine/ test suites. The 1423 lines of new code (MoneroEngine, MoneroTools, moneroTypes) have zero dedicated test coverage. Key logic (processTransaction, queryTransactionsAsc/Desc, resolveBirthdayHeight, makeSpend, translateFee) is untested.
- Add Monero fixtures to the plugin and engine test suites. If the native CppBridge cannot run in Node.js, create a mock. At minimum test processTransaction, translateFee, and the cleaner types which have no native dependency.
src/monero/MoneroEngine.ts
Outdated
| if (isReceive) { | ||
| nativeAmount = tx.amount.toString() | ||
| } else { | ||
| nativeAmount = `-${(tx.amount + tx.fee).toString()}` |
There was a problem hiding this comment.
Issue: Precision loss: (tx.amount + tx.fee).toString() uses JavaScript number addition on piconero values that can exceed Number.MAX_SAFE_INTEGER (~9 XMR). The rest of the codebase uses biggystring for amount arithmetic. This line silently produces wrong amounts for large transactions.
Recommendation: Use nativeAmount = -${add(tx.amount.toString(), tx.fee.toString())}`` with the already-imported add from biggystring.
| const unsubscribeWalletEvent = this.tools.moneroIo.on( | ||
| 'walletEvent', | ||
| event => { | ||
| if (event.walletId !== base64UrlWalletId) return | ||
| this.log(`Wallet event: ${event.eventName} data=${event.data}`) | ||
| this.queryTransactions(base64UrlWalletId).catch(err => | ||
| this.log.error( | ||
| `Event-triggered queryTransactions error: ${String(err)}` | ||
| ) | ||
| ) | ||
| } | ||
| ) |
There was a problem hiding this comment.
Warning: Every wallet event triggers a full queryTransactions call regardless of event type. Events like sync-progress or block-connected do not need transaction re-queries. High-frequency events during sync cause a flood of overlapping queryTransactions calls that race on otherData fields (mostRecentTxid, processedTransactionCount).
Recommendation: Filter events by eventName to only trigger on transaction-relevant events. Debounce or serialize with a mutex to prevent concurrent queryTransactions calls racing on shared state.
| const keysPromise = new Promise<MoneroPrivateKeys>(resolve => { | ||
| this.sendKeysToNative = resolve | ||
| }) |
There was a problem hiding this comment.
Warning: keysPromise is a one-shot promise resolved by syncNetwork. If killEngine is called before syncNetwork ever runs (e.g., rapid changeUserSettings), nativeWalletId.stop() blocks on the lifecycle manager's onStart which is blocked on the unresolved keysPromise, causing a deadlock.
Recommendation: Add a rejection path for keysPromise in killEngine, or add a timeout in onStart so the lifecycle manager can abort cleanly if keys never arrive.
| async changeUserSettings(userSettings: JsonObject): Promise<void> { | ||
| const newSettings = asMaybe(asMoneroUserSettings)(userSettings) | ||
| if (newSettings == null || matchJson(this.currentSettings, newSettings)) { | ||
| return | ||
| } | ||
|
|
||
| this.currentSettings = newSettings | ||
| await this.killEngine() | ||
| await this.startEngine() | ||
| } | ||
|
|
||
| async changeWalletSettings(walletSettings: JsonObject): Promise<void> { | ||
| const newSettings = asMaybe(asMoneroWalletSettings)(walletSettings) | ||
| if ( | ||
| newSettings == null || | ||
| matchJson(this.currentWalletSettings, newSettings) | ||
| ) { | ||
| return | ||
| } | ||
|
|
||
| this.currentWalletSettings = newSettings | ||
| await this.killEngine() | ||
| await this.clearBlockchainCache() | ||
| await this.startEngine() | ||
| } |
There was a problem hiding this comment.
Warning: changeUserSettings and changeWalletSettings call killEngine then startEngine without guarding against concurrent invocations. Rapid settings changes can interleave, with the second killEngine running while the first startEngine is still initializing.
Recommendation: Serialize settings changes with a mutex or guard flag to prevent overlapping killEngine/startEngine cycles.
| }) | ||
| } | ||
|
|
||
| const isReceive = tx.direction === 0 |
There was a problem hiding this comment.
Warning: Magic number tx.direction === 0 to determine receive vs send. If the native library changes the TransactionDirection enum ordering, this silently breaks.
Recommendation: Import and use named enum values from react-native-monero-lwsf (e.g., TransactionDirection.IN) instead of comparing against 0.
| const moneroIo = nativeIo.monero as MoneroIo | ||
| if (moneroIo == null) throw new Error('Need monero native IO') |
There was a problem hiding this comment.
Suggestion: Type assertion nativeIo.monero as MoneroIo bypasses type safety. Missing methods would surface later at runtime.
Recommendation: Use a cleaner/validator for the MoneroIo interface at this boundary to fail fast with a clear error.
| export function translateFee(fee?: string): TransactionPriority { | ||
| if (fee === 'low') return 1 | ||
| if (fee === 'high') return 3 | ||
| return 2 // Default to medium | ||
| } |
There was a problem hiding this comment.
Suggestion: translateFee maps strings to magic numbers (1, 2, 3) without documentation of what Monero priority levels they correspond to.
Recommendation: Add inline comments: 1=Low, 2=Normal/Default, 3=Elevated.
| } | ||
|
|
||
| try { | ||
| const status = await this.tools.cppBridge.getWalletStatus(nativeWalletId) |
There was a problem hiding this comment.
Suggestion: syncNetwork uses magic numbers for poll intervals (1000, 20000, 5000).
Recommendation: Extract into named constants: SYNC_POLL_MS, SYNCED_POLL_MS, ERROR_POLL_MS.
| @@ -22,7 +22,9 @@ export interface ZanoSyncTracker extends SyncTracker { | |||
| /** | |||
There was a problem hiding this comment.
Suggestion: Incomplete JSDoc: 'Creates a Sync' is truncated.
Recommendation: Complete the JSDoc description.
| async resyncBlockchain(): Promise<void> { | ||
| await this.killEngine() | ||
| await this.clearBlockchainCache() | ||
| await this.tools.cppBridge.deleteWallet( | ||
| base64url.stringify(base64.parse(this.walletId)), | ||
| this.currentWalletSettings.backend | ||
| ) | ||
| await this.startEngine() | ||
| } |
There was a problem hiding this comment.
Suggestion: The keysPromise pattern works across restarts only because the closure captures the already-resolved promise. This is subtle and fragile.
Recommendation: Add a comment documenting that keysPromise intentionally persists as a singleton resolved once for the wallet lifetime.
eddy-edge
left a comment
There was a problem hiding this comment.
Requesting changes after full review. Architecture is strong and many comments were addressed, but there are still blockers around type-safety/dependency hygiene plus a few correctness/perf issues.
src/monero/moneroTypes.ts
Outdated
| return { | ||
| dataKey: clean.dataKey, | ||
| moneroKey: clean[`${pluginId}Key`] as string, | ||
| birthdayHeight: clean[`${pluginId}BirthdayHeight`] as number, |
There was a problem hiding this comment.
This cast erases optionality (number | undefined). Please keep undefined in the type so legacy handling remains type-enforced.
eddy-edge
left a comment
There was a problem hiding this comment.
Requesting changes after full review. Architecture is strong and many comments were addressed, but there are still blockers around type-safety/dependency hygiene plus a few correctness/perf issues.
src/monero/MoneroEngine.ts
Outdated
| if (isReceive) { | ||
| nativeAmount = tx.amount.toString() | ||
| } else { | ||
| nativeAmount = `-${(tx.amount + tx.fee).toString()}` |
There was a problem hiding this comment.
JS number addition for atomic amounts can lose precision at larger values. Please use bigint/string math (e.g. biggystring).
eddy-edge
left a comment
There was a problem hiding this comment.
Requesting changes after full review. Architecture is strong and many comments were addressed, but there are still blockers around type-safety/dependency hygiene plus a few correctness/perf issues.
| return '0' | ||
| } | ||
| return maxSpendable | ||
| } catch (error: unknown) { |
There was a problem hiding this comment.
Returning full unlocked balance on error can overstate spendable value; prefer throw or conservative fallback.
eddy-edge
left a comment
There was a problem hiding this comment.
Requesting changes after full review. Architecture is strong and many comments were addressed, but there are still blockers around type-safety/dependency hygiene plus a few correctness/perf issues.
| ) | ||
|
|
||
| // Subscribe to native wallet events for immediate tx detection | ||
| const unsubscribeWalletEvent = this.tools.moneroIo.on( |
There was a problem hiding this comment.
This triggers tx queries for every wallet event; please filter to tx-relevant events.
eddy-edge
left a comment
There was a problem hiding this comment.
Additional blocking dependency/version notes from full review: please align Monero package semver between dev/peer deps (including removing v prefix), avoid exact beta pin for edge-core-js unless intentional, and add a CHANGELOG entry before merge.
| @@ -158,7 +160,7 @@ | |||
| "chai": "^4.2.0", | |||
| "clipanion": "^4.0.0-rc.2", | |||
There was a problem hiding this comment.
edge-core-js was changed from a ranged stable dependency to an exact beta pin. Please confirm this is intentional for merge and, if not, switch back to an agreed stable/ranged version.
| @@ -199,6 +202,7 @@ | |||
| "webpack-dev-server": "^4.11.1" | |||
| }, | |||
There was a problem hiding this comment.
Please align react-native-monero-lwsf semver across dev/peer deps and remove the v prefix in peer deps ("v0.1.0" -> valid semver/range). The current combination is easy to mis-resolve for consumers.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Sync progress ratio can go negative after reorg
- I clamped the computed sync block ratio to the [0, 1] range before passing it to the weighted sync tracker.
- ✅ Fixed: Hardcoded currency code instead of using currencyInfo
- I replaced the hardcoded 'XMR' in processed transactions with this.currencyInfo.currencyCode to align with engine conventions.
- ✅ Fixed: Wallet event log exposes potentially sensitive data
- I removed event.data from wallet event logging so only the event name is logged.
Or push these changes by commenting:
@cursor push 6e67221e80
Preview (6e67221e80)
diff --git a/src/monero/MoneroEngine.ts b/src/monero/MoneroEngine.ts
--- a/src/monero/MoneroEngine.ts
+++ b/src/monero/MoneroEngine.ts
@@ -162,7 +162,7 @@
event => {
if (event.walletId !== base64UrlWalletId) return
if (event.eventName !== 'pendingTransactionReceived') return
- this.log(`Wallet event: ${event.eventName} data=${event.data}`)
+ this.log(`Wallet event: ${event.eventName}`)
this.queryTransactions(base64UrlWalletId).catch(err =>
this.log.error(
`Event-triggered queryTransactions error: ${String(err)}`
@@ -344,8 +344,9 @@
return 20000
} else {
const range = status.networkHeight - this.syncStartHeight
- const ratio =
+ const rawRatio =
range > 0 ? (status.syncedHeight - this.syncStartHeight) / range : 0
+ const ratio = Math.max(0, Math.min(1, rawRatio))
this.syncTracker.updateBlockRatio(
ratio,
@@ -524,7 +525,7 @@
const edgeTransaction: EdgeTransaction = {
blockHeight,
- currencyCode: 'XMR',
+ currencyCode: this.currencyInfo.currencyCode,
date: tx.timestamp,
isSend: !isReceive,
memos,This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| } else { | ||
| const range = status.networkHeight - this.syncStartHeight | ||
| const ratio = | ||
| range > 0 ? (status.syncedHeight - this.syncStartHeight) / range : 0 |
There was a problem hiding this comment.
Sync progress ratio can go negative after reorg
Low Severity
The sync progress ratio is computed as (syncedHeight - syncStartHeight) / range without clamping. If syncedHeight drops below syncStartHeight after a chain reorg, the ratio becomes negative, which propagates through updateBlockRatio into the weighted sync calculation and could produce a negative totalRatio.
5888d25 to
9d42eef
Compare
| } else { | ||
| const range = status.networkHeight - this.syncStartHeight | ||
| const ratio = | ||
| range > 0 ? (status.syncedHeight - this.syncStartHeight) / range : 0 |
There was a problem hiding this comment.
Sync progress ratio can go negative after reorg
Medium Severity
The sync progress ratio is not clamped. If syncedHeight drops below syncStartHeight (e.g., after a blockchain reorganization), the expression (status.syncedHeight - this.syncStartHeight) / range produces a negative value. This negative ratio flows into WeightedSyncTracker.updateBlockRatio, making totalRatio negative, which could cause unexpected UI behavior.
9d42eef to
11fc752
Compare
11fc752 to
8b5c1d1
Compare
| } else { | ||
| const range = status.networkHeight - this.syncStartHeight | ||
| const ratio = | ||
| range > 0 ? (status.syncedHeight - this.syncStartHeight) / range : 0 |
There was a problem hiding this comment.
Sync progress ratio can go negative after reorg
Low Severity
If syncedHeight drops below syncStartHeight (e.g., after a chain reorg), the computed ratio becomes negative. This negative value is passed to updateBlockRatio, which stores it and factors it into totalRatio via the weighted sync tracker. While the tracker's maybeSendUpdate won't forward negative progress, lastTotalRatio gets set to a negative value, temporarily corrupting internal progress state. Clamping with Math.max(0, ...) before passing to updateBlockRatio would prevent this.
8b5c1d1 to
b52cdcf
Compare
| "clipanion": "^4.0.0-rc.2", | ||
| "crypto-browserify": "^3.12.0", | ||
| "edge-core-js": "^2.42.0", | ||
| "edge-core-js": "2.42.1-beta.0", |
There was a problem hiding this comment.
Peer dependency pins unstable pre-release version
Medium Severity
edge-core-js was changed from the ranged stable pin ^2.42.0 to the exact beta pin 2.42.1-beta.0, and react-native-monero-lwsf is declared as v0.1.0 in peerDependencies (with an invalid v prefix) but as 0.1.0-beta.0 in devDependencies. The dev environment therefore tests against a pre-release that does not satisfy the declared peer range, meaning consumers installing the stable peer dep will use an untested version of the library.
Additional Locations (1)
Triggered by team rule: Use review sub agents
b52cdcf to
b99e1d5
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 6 total unresolved issues (including 5 from previous reviews).
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Birthday height zero bypasses fallback guard
- Updated
resolveBirthdayHeightto only short-circuit for positive stored heights so a saved value of 0 now falls through to the fallback guard path instead of opening from genesis.
- Updated
Or push these changes by commenting:
@cursor push 42c46b7b76
Preview (42c46b7b76)
diff --git a/src/monero/MoneroEngine.ts b/src/monero/MoneroEngine.ts
--- a/src/monero/MoneroEngine.ts
+++ b/src/monero/MoneroEngine.ts
@@ -216,7 +216,7 @@
edgeLwsServer: string,
loginResult?: LoginResponse
): Promise<number> {
- if (height != null) return height
+ if (height != null && height > 0) return height
// For Edge LWS, the login response may already have it
if (loginResult?.start_height != null) {This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| edgeLwsServer: string, | ||
| loginResult?: LoginResponse | ||
| ): Promise<number> { | ||
| if (height != null) return height |
There was a problem hiding this comment.
Birthday height zero bypasses fallback guard
Medium Severity
resolveBirthdayHeight uses height != null to short-circuit early, but 0 != null is true, so a stored birthdayHeight of 0 is returned immediately, bypassing the fallback logic that explicitly throws when start_height === 0. The call-site comment says "never open a wallet with height 0", yet this guard doesn't enforce it. Meanwhile, importPrivateKey only validates birthdayHeight > currentNetworkHeight without rejecting 0, so a zero height can legitimately be stored, causing the wallet to attempt scanning from genesis.
Additional Locations (1)
It's useful for other similar slow sync privacy chains
8651ff6 to
1caf499
Compare



CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
noneNote
High Risk
Adds an entirely new Monero engine/tools implementation with native bridge calls, wallet lifecycle management, and transaction creation/broadcasting, which is complex and touches critical funds-handling code paths. Also updates core dependencies and sync tracking behavior used by other engines (e.g. Zano), increasing regression risk.
Overview
Introduces first-class Monero (XMR) support by adding a new plugin (
moneroInfo) plusMoneroTools/MoneroEnginethat integrate withreact-native-monero-lwsffor key management, wallet open/close lifecycle, sync, transaction querying, and spend/broadcast flows.Refactors sync progress tracking by generalizing/renaming the existing Zano tracker to
WeightedSyncTrackerand switchingZanoEngineto use it; updates type declarations to includeMoneroLwsfModule+NativeEventEmitter, and wires Monero into exports/packaging (src/index.ts,rn-monero.*,package.json).Adjusts CLI/tests to always pass
userSettingsand the newly requiredwalletSettings, and bumpsedge-core-jsto2.42.1-beta.0while adding Monero native dependency entries.Written by Cursor Bugbot for commit 1caf499. This will update automatically on new commits. Configure here.