manual maker selection#93
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR introduces manual maker selection for swaps, persists selections, updates Tor endpoint display and market grid layout, hides setup info by default and shows it on test failure, and forwards selected maker addresses from the UI through IPC to the worker as preferred makers. ChangesManual Maker Selection Feature
Setup Flow Visibility Improvements
Sequence Diagram(s)sequenceDiagram
participant SwapUI
participant IPC as api1.js
participant Worker as coinswap-worker.js
SwapUI->>IPC: window.api.coinswap.start(payload[, selectedMakerAddresses])
IPC->>IPC: validate selectedMakerAddresses
IPC->>Worker: spawn Worker with workerData (includes selectedMakerAddresses)
Worker->>Worker: set swapParams.preferredMakers = selectedMakerAddresses (if non-empty)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/swap/Swap.js (1)
601-603:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFee estimate ignores manually selected makers
calculateFees()callsgetTopCandidateMakers(), which always returns the first N makers sorted by index — regardless ofmakerSelectionMode. When the user manually selects higher-fee makers, the displayed "Estimated Maker Fee" and "Total Fee" are computed from the auto-selected makers' fee structures, not the chosen ones. The user may approve a swap based on a materially understated cost estimate.🛡️ Proposed fix — use selected makers when in manual mode
function getTopCandidateMakers() { + if (makerSelectionMode === 'manual' && selectedMakerAddresses.length > 0) { + const selected = availableMakers.filter((m) => + selectedMakerAddresses.includes(m.address) + ); + if (selected.length >= getNumberOfMakers()) return selected; + } return availableMakers.slice(0, getNumberOfMakers()); }Both
makerSelectionModeandselectedMakerAddressesare already in scope via closure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/swap/Swap.js` around lines 601 - 603, calculateFees currently uses getTopCandidateMakers which always slices availableMakers by index, ignoring makerSelectionMode and any user-chosen makers; update getTopCandidateMakers (or calculateFees) to return the user-selected makers when makerSelectionMode === 'manual' by mapping selectedMakerAddresses to their corresponding entries in availableMakers (preserving order and then trimming to getNumberOfMakers()), otherwise fall back to the existing slice behavior; ensure calculateFees uses this updated list so "Estimated Maker Fee" and "Total Fee" reflect manually selected makers (refer to calculateFees, getTopCandidateMakers, makerSelectionMode, selectedMakerAddresses, availableMakers, and getNumberOfMakers).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api1.js`:
- Line 1456: The handler's parameter selectedMakerAddresses is not validated
before being forwarded and can cause a TypeError when the worker accesses
.length; add an input guard in the async handler (the function starting with
async (event, { amount, makerCount, outpoints, password, selectedMakerAddresses
}) => {) right after the existing amount validation: ensure
selectedMakerAddresses is either undefined/null or an Array (use Array.isArray),
and if present validate its items are the expected type (e.g., strings matching
address format) and optionally non-empty; if validation fails, return or throw a
controlled error (or normalize to an empty array) so coinswap-worker.js (which
reads .length at worker code) never receives a non-array value.
In `@src/components/market/Market.js`:
- Line 88: The current construction of fullAddress can produce ":" when addr is
an object with both onion_addr and port falsy; change the logic in the
fullAddress assignment so it only inserts the ":" when a port is present and
returns an empty string (or other safe sentinel) when neither onion_addr nor
port exist. Concretely, when addr is not a string compute host =
addr?.onion_addr || '' and portSuffix = addr?.port ? `:${addr.port}` : '' and
set fullAddress = host || portSuffix ? `${host}${portSuffix}` : '' (and ensure
callers like window.viewFidelityBond(...) and any preferredMaker usage handle
the empty-string sentinel appropriately).
In `@src/components/settings/FirstTimeSetup.js`:
- Line 1098: Move the torFailed calculation to the same place as nodeFailed for
consistency: instead of computing const torFailed = !socksResult?.success ||
!controlResult?.success before calling renderConnectionResults, compute
torFailed from the rendered results array after renderConnectionResults (similar
to how nodeFailed is derived). Update the logic that uses torFailed to reference
the newly computed variable location and keep the original
socksResult/controlResult usage only for passing into renderConnectionResults.
In `@src/components/swap/Swap.js`:
- Around line 465-475: renderMakerList must prune stale addresses from
selectedMakerAddresses before restoring checkboxes and updating the counter:
compute a new array/filter of selectedMakerAddresses that only includes
addresses present in availableMakers (compare maker.address) and replace/assign
selectedMakerAddresses with this pruned array at the top of renderMakerList,
then continue with the existing checkbox restore and count update so the
displayed "Selected: N" and the value used by validateSwapConfig and backend
preferredMaker forwarding reflect only currently available makers (ensure
validateSwapConfig is called with or reads the pruned selectedMakerAddresses).
- Around line 446-463: The code sets makerListContainer.innerHTML using
unescaped values (availableMakers, maker.address via formatTorEndpoint,
maker.baseFee, maker.volumeFeePct, maker.minSize, maker.maxSize), allowing XSS
from third‑party Tor offerbook data; fix by escaping/encoding all dynamic values
before building HTML (or use DOM methods instead of innerHTML). Locate the
innerHTML assignment in Swap.js where makerListContainer and availableMakers are
used and either: 1) create/replace with a small HTML-escape helper and apply it
to formatTorEndpoint(maker.address) and every inserted field (maker.baseFee,
maker.volumeFeePct, maker.minSize, maker.maxSize), or 2) construct elements with
document.createElement and set textContent/attributes (checkbox id
maker-addr-${index} should be sanitized or set via element.id) to avoid
inserting raw HTML. Ensure formatTorEndpoint output is escaped or populated via
textContent rather than concatenated into innerHTML.
---
Outside diff comments:
In `@src/components/swap/Swap.js`:
- Around line 601-603: calculateFees currently uses getTopCandidateMakers which
always slices availableMakers by index, ignoring makerSelectionMode and any
user-chosen makers; update getTopCandidateMakers (or calculateFees) to return
the user-selected makers when makerSelectionMode === 'manual' by mapping
selectedMakerAddresses to their corresponding entries in availableMakers
(preserving order and then trimming to getNumberOfMakers()), otherwise fall back
to the existing slice behavior; ensure calculateFees uses this updated list so
"Estimated Maker Fee" and "Total Fee" reflect manually selected makers (refer to
calculateFees, getTopCandidateMakers, makerSelectionMode,
selectedMakerAddresses, availableMakers, and getNumberOfMakers).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9fc9f783-7004-4e18-aecf-9071aab960dc
📒 Files selected for processing (6)
api1.jscoinswap-worker.jssrc/components/market/Market.jssrc/components/settings/FirstTimeSetup.jssrc/components/swap/Swap.jssrc/styles/output.css
| const port = addressObj.port || '6102'; | ||
| fullAddress = `${onionAddr}:${port}`; | ||
| } | ||
| fullAddress = typeof addr === 'string' ? addr : `${addr?.onion_addr || ''}:${addr?.port || ''}`; |
There was a problem hiding this comment.
Malformed ":" address when both onion_addr and port are absent
When addr is an object where both onion_addr and port are falsy, the template literal produces ":". This string is then used as the maker's lookup key in the fidelity bond modal (window.viewFidelityBond('...')) and can be forwarded as a preferredMaker to the swap backend. The previous behaviour at least supplied a default port.
🛡️ Proposed guard
- fullAddress = typeof addr === 'string' ? addr : `${addr?.onion_addr || ''}:${addr?.port || ''}`;
+ fullAddress = typeof addr === 'string'
+ ? addr
+ : (addr?.onion_addr && addr?.port)
+ ? `${addr.onion_addr}:${addr.port}`
+ : addr?.onion_addr || 'unknown';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/market/Market.js` at line 88, The current construction of
fullAddress can produce ":" when addr is an object with both onion_addr and port
falsy; change the logic in the fullAddress assignment so it only inserts the ":"
when a port is present and returns an empty string (or other safe sentinel) when
neither onion_addr nor port exist. Concretely, when addr is not a string compute
host = addr?.onion_addr || '' and portSuffix = addr?.port ? `:${addr.port}` : ''
and set fullAddress = host || portSuffix ? `${host}${portSuffix}` : '' (and
ensure callers like window.viewFidelityBond(...) and any preferredMaker usage
handle the empty-string sentinel appropriately).
| window.api.testTcpPort({ host: '127.0.0.1', port: controlPort }), | ||
| ]); | ||
|
|
||
| const torFailed = !socksResult?.success || !controlResult?.success; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
torFailed placement is inconsistent with the RPC path — minor style nit.
In the RPC path nodeFailed is derived from the results array after renderConnectionResults (line 989 follows line 988). Here torFailed is computed from raw API values before renderConnectionResults (line 1098 precedes line 1099). Both are semantically equivalent, but aligning the two paths would improve readability and make future maintenance clearer.
♻️ Suggested alignment
- const torFailed = !socksResult?.success || !controlResult?.success;
renderConnectionResults(resultDiv, [
{
label: 'SOCKS Port',
ok: Boolean(socksResult?.success),
message: socksResult?.success
? `Port ${socksPort} reachable`
: socksResult?.error,
},
{
label: 'Control Port',
ok: Boolean(controlResult?.success),
message: controlResult?.success
? `Port ${controlPort} reachable`
: controlResult?.error,
},
]);
+ const torFailed = !socksResult?.success || !controlResult?.success;
const infoDiv = modal.querySelector('#tor-setup-info');
if (infoDiv) infoDiv.classList.toggle('hidden', !torFailed);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/settings/FirstTimeSetup.js` at line 1098, Move the torFailed
calculation to the same place as nodeFailed for consistency: instead of
computing const torFailed = !socksResult?.success || !controlResult?.success
before calling renderConnectionResults, compute torFailed from the rendered
results array after renderConnectionResults (similar to how nodeFailed is
derived). Update the logic that uses torFailed to reference the newly computed
variable location and keep the original socksResult/controlResult usage only for
passing into renderConnectionResults.
Comment on lines
+446
to
+463
| makerListContainer.innerHTML = availableMakers | ||
| .map((maker, index) => { | ||
| return ` | ||
| <label class="flex items-center gap-3 bg-[#0f1419] hover:bg-[#242d3d] rounded-lg p-3 cursor-pointer transition-colors"> | ||
| <input type="checkbox" id="maker-addr-${index}" class="w-4 h-4 accent-[#FF6B35]" /> | ||
| <div class="flex-1"> | ||
| <div class="flex justify-between items-center"> | ||
| <span class="font-mono text-sm text-gray-300">${formatTorEndpoint(maker.address)}</span> | ||
| </div> | ||
| <div class="flex justify-between items-center mt-1"> | ||
| <span class="text-xs text-gray-500">${maker.baseFee} sats + ${maker.volumeFeePct.toFixed(3)}%</span> | ||
| <span class="text-xs text-gray-500">${maker.minSize.toLocaleString()} – ${maker.maxSize.toLocaleString()} sats</span> | ||
| </div> | ||
| </div> | ||
| </label> | ||
| `; | ||
| }) | ||
| .join(''); |
There was a problem hiding this comment.
XSS via innerHTML with externally-sourced maker address
maker.address originates from the Tor offerbook (a third-party source). formatTorEndpoint only slices the string — it performs no HTML encoding. For an address shorter than 33 characters (e.g., <img src=x onerror=eval(atob(...))>:6102), the function returns it unchanged and it is injected into innerHTML.
A malicious maker node can craft their onion_addr field with an HTML payload. In Electron, the impact depends on contextIsolation/nodeIntegration configuration, and can range from UI defacement to remote code execution.
🛡️ Proposed fix — escape HTML before injection
Add a minimal escaping helper and apply it to all dynamic values:
+ function escapeHtml(str) {
+ return String(str)
+ .replace(/&/g, '&')
+ .replace(/</g, '<')
+ .replace(/>/g, '>')
+ .replace(/"/g, '"')
+ .replace(/'/g, ''');
+ }
makerListContainer.innerHTML = availableMakers
.map((maker, index) => {
return `
<label class="flex items-center gap-3 bg-[`#0f1419`] hover:bg-[`#242d3d`] rounded-lg p-3 cursor-pointer transition-colors">
<input type="checkbox" id="maker-addr-${index}" class="w-4 h-4 accent-[`#FF6B35`]" />
<div class="flex-1">
<div class="flex justify-between items-center">
- <span class="font-mono text-sm text-gray-300">${formatTorEndpoint(maker.address)}</span>
+ <span class="font-mono text-sm text-gray-300">${escapeHtml(formatTorEndpoint(maker.address))}</span>
</div>
<div class="flex justify-between items-center mt-1">
- <span class="text-xs text-gray-500">${maker.baseFee} sats + ${maker.volumeFeePct.toFixed(3)}%</span>
- <span class="text-xs text-gray-500">${maker.minSize.toLocaleString()} – ${maker.maxSize.toLocaleString()} sats</span>
+ <span class="text-xs text-gray-500">${escapeHtml(maker.baseFee)} sats + ${escapeHtml(maker.volumeFeePct.toFixed(3))}%</span>
+ <span class="text-xs text-gray-500">${escapeHtml(maker.minSize.toLocaleString())} – ${escapeHtml(maker.maxSize.toLocaleString())} sats</span>
</div>
</div>
</label>
`;
})🧰 Tools
🪛 OpenGrep (1.20.0)
[WARNING] 446-463: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/swap/Swap.js` around lines 446 - 463, The code sets
makerListContainer.innerHTML using unescaped values (availableMakers,
maker.address via formatTorEndpoint, maker.baseFee, maker.volumeFeePct,
maker.minSize, maker.maxSize), allowing XSS from third‑party Tor offerbook data;
fix by escaping/encoding all dynamic values before building HTML (or use DOM
methods instead of innerHTML). Locate the innerHTML assignment in Swap.js where
makerListContainer and availableMakers are used and either: 1) create/replace
with a small HTML-escape helper and apply it to formatTorEndpoint(maker.address)
and every inserted field (maker.baseFee, maker.volumeFeePct, maker.minSize,
maker.maxSize), or 2) construct elements with document.createElement and set
textContent/attributes (checkbox id maker-addr-${index} should be sanitized or
set via element.id) to avoid inserting raw HTML. Ensure formatTorEndpoint output
is escaped or populated via textContent rather than concatenated into innerHTML.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api1.js`:
- Around line 1467-1474: The validator for selectedMakerAddresses should also
enforce that, when provided, the array contains at least makerCount entries and
that entries are unique and non-empty; update the handler logic around
selectedMakerAddresses and makerCount to (1) check selectedMakerAddresses.length
>= makerCount, (2) deduplicate entries (e.g., via a Set) and ensure the
deduplicated count >= makerCount, and (3) return a clear error like
'selectedMakerAddresses must contain at least {makerCount} unique non-empty
strings' if those checks fail.
In `@src/components/market/Market.js`:
- Line 766: Extract the repeated "2fr 1fr 1fr 1fr 1fr 1fr 1fr" grid-template
string into a single constant (e.g., const GRID_TEMPLATE_COLUMNS = "2fr 1fr 1fr
1fr 1fr 1fr 1fr") at the top of the Market component and replace the inline
style usages in the row JSX and the header JSX with style={{
gridTemplateColumns: GRID_TEMPLATE_COLUMNS }} (or use the constant in a style
object) so both the row div (the element with class "grid gap-4 p-4
hover:bg-[`#242d3d`] transition-colors") and the header render reference the same
constant.
- Around line 88-94: The current construction of fullAddress can produce a
leading ":<port>" when addr.onion_addr is falsy but addr.port is truthy; update
the logic in Market.js where fullAddress is computed (variable fullAddress,
input addr with fields onion_addr and port) to only build an address when
onion_addr is present: if typeof addr === 'string' keep it, else if
addr.onion_addr is truthy concatenate onion_addr with an optional `:${port}`;
otherwise set fullAddress to an empty string so downstream lookups
(viewFidelityBond) and formatTorEndpoint never receive a port-only string.
In `@src/components/swap/Swap.js`:
- Around line 751-763: The label and displayed text currently switch to auto
mode when makerSelectionMode === 'manual' but selectedMakerAddresses is empty;
change the logic in the block around makersLabelEl / makerSelectionMode /
selectedMakerAddresses so that when makerSelectionMode === 'manual' you always
set makersLabelEl.textContent = 'Selected Makers' and selectedMakersText =
selectedMakerAddresses.length ? selectedMakerAddresses.map(addr =>
formatTorEndpoint(addr)).join(', ') : 'None selected'; only use
getTopCandidateMakers() and set the 'Top Maker Candidates' label when
makerSelectionMode !== 'manual'.
- Around line 275-278: The code builds a malformed makerAddress when
item.address is an object but onion_addr is falsy, producing strings like
":<port>" or ":" which pollute selectedMakerAddresses, availableMakers and are
sent as invalid preferredMaker via the coinswap:start IPC; change the
makerAddress construction in Swap.js to require a truthy item.address.onion_addr
(i.e., only construct `${item.address.onion_addr}:${item.address.port}` when
onion_addr exists) and otherwise treat the maker as invalid (set makerAddress to
empty/null) and then filter out entries with empty makerAddress before
populating availableMakers or using selectedMakerAddresses / sending
preferredMaker.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14f4911e-8ab8-41fd-aa2c-8da31e57b99a
📒 Files selected for processing (3)
api1.jssrc/components/market/Market.jssrc/components/swap/Swap.js
Comment on lines
+1467
to
+1474
| if ( | ||
| selectedMakerAddresses != null && | ||
| (!Array.isArray(selectedMakerAddresses) || | ||
| selectedMakerAddresses.some((a) => typeof a !== 'string' || !a.trim())) | ||
| ) { | ||
| return { success: false, error: 'Invalid selectedMakerAddresses: must be an array of non-empty strings' }; | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Validation looks good; consider also enforcing length and uniqueness.
The added type/shape validation correctly addresses the prior concern. As an optional hardening, the handler does not currently enforce that selectedMakerAddresses.length is at least makerCount and does not deduplicate. A renderer with a stale UI state (or any other IPC caller) could submit fewer unique entries than makerCount, which would then surface as a worker-side failure rather than a clean error here.
♻️ Optional tightening
if (
selectedMakerAddresses != null &&
(!Array.isArray(selectedMakerAddresses) ||
selectedMakerAddresses.some((a) => typeof a !== 'string' || !a.trim()))
) {
return { success: false, error: 'Invalid selectedMakerAddresses: must be an array of non-empty strings' };
}
+
+ if (Array.isArray(selectedMakerAddresses) && selectedMakerAddresses.length > 0) {
+ const unique = new Set(selectedMakerAddresses.map((a) => a.trim()));
+ if (unique.size !== selectedMakerAddresses.length) {
+ return { success: false, error: 'selectedMakerAddresses contains duplicates' };
+ }
+ if (unique.size < makerCount) {
+ return {
+ success: false,
+ error: `Need ${makerCount} makers, only ${unique.size} selected`,
+ };
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ( | |
| selectedMakerAddresses != null && | |
| (!Array.isArray(selectedMakerAddresses) || | |
| selectedMakerAddresses.some((a) => typeof a !== 'string' || !a.trim())) | |
| ) { | |
| return { success: false, error: 'Invalid selectedMakerAddresses: must be an array of non-empty strings' }; | |
| } | |
| if ( | |
| selectedMakerAddresses != null && | |
| (!Array.isArray(selectedMakerAddresses) || | |
| selectedMakerAddresses.some((a) => typeof a !== 'string' || !a.trim())) | |
| ) { | |
| return { success: false, error: 'Invalid selectedMakerAddresses: must be an array of non-empty strings' }; | |
| } | |
| if (Array.isArray(selectedMakerAddresses) && selectedMakerAddresses.length > 0) { | |
| const unique = new Set(selectedMakerAddresses.map((a) => a.trim())); | |
| if (unique.size !== selectedMakerAddresses.length) { | |
| return { success: false, error: 'selectedMakerAddresses contains duplicates' }; | |
| } | |
| if (unique.size < makerCount) { | |
| return { | |
| success: false, | |
| error: `Need ${makerCount} makers, only ${unique.size} selected`, | |
| }; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api1.js` around lines 1467 - 1474, The validator for selectedMakerAddresses
should also enforce that, when provided, the array contains at least makerCount
entries and that entries are unique and non-empty; update the handler logic
around selectedMakerAddresses and makerCount to (1) check
selectedMakerAddresses.length >= makerCount, (2) deduplicate entries (e.g., via
a Set) and ensure the deduplicated count >= makerCount, and (3) return a clear
error like 'selectedMakerAddresses must contain at least {makerCount} unique
non-empty strings' if those checks fail.
Comment on lines
88
to
94
| if (typeof addr === 'string') { | ||
| fullAddress = addr.includes(':') ? addr : `${addr}:6102`; | ||
| fullAddress = addr; | ||
| } else { | ||
| const addressObj = addr || {}; | ||
| const onionAddr = addressObj.onion_addr || ''; | ||
| const port = addressObj.port || '6102'; | ||
| fullAddress = `${onionAddr}:${port}`; | ||
| const host = addr?.onion_addr || ''; | ||
| const portSuffix = addr?.port ? `:${addr.port}` : ''; | ||
| fullAddress = host || portSuffix ? `${host}${portSuffix}` : ''; | ||
| } |
There was a problem hiding this comment.
Edge case: port-only object can still produce a leading-colon string.
The new logic correctly avoids the bare ":" from the previous code when both fields are missing, but if addr.onion_addr is falsy while addr.port is truthy, the result becomes ":<port>" (e.g., ":6102"). That string then flows into the table title/viewFidelityBond lookup and formatTorEndpoint (which would treat the entire :port as host after slicing on the last :).
Suggest gating on onion_addr being present rather than on either field:
🛡️ Proposed guard
if (typeof addr === 'string') {
fullAddress = addr;
} else {
- const host = addr?.onion_addr || '';
- const portSuffix = addr?.port ? `:${addr.port}` : '';
- fullAddress = host || portSuffix ? `${host}${portSuffix}` : '';
+ const host = addr?.onion_addr || '';
+ const portSuffix = host && addr?.port ? `:${addr.port}` : '';
+ fullAddress = host ? `${host}${portSuffix}` : '';
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/market/Market.js` around lines 88 - 94, The current
construction of fullAddress can produce a leading ":<port>" when addr.onion_addr
is falsy but addr.port is truthy; update the logic in Market.js where
fullAddress is computed (variable fullAddress, input addr with fields onion_addr
and port) to only build an address when onion_addr is present: if typeof addr
=== 'string' keep it, else if addr.onion_addr is truthy concatenate onion_addr
with an optional `:${port}`; otherwise set fullAddress to an empty string so
downstream lookups (viewFidelityBond) and formatTorEndpoint never receive a
port-only string.
| (maker) => { | ||
| return ` | ||
| <div class="grid grid-cols-7 gap-4 p-4 hover:bg-[#242d3d] transition-colors"> | ||
| <div class="grid gap-4 p-4 hover:bg-[#242d3d] transition-colors" style="grid-template-columns: 2fr 1fr 1fr 1fr 1fr 1fr 1fr"> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Inline grid-template-columns duplicated between header and rows.
The same 7-column template string is repeated in the header (line 867) and on every row render (line 766). Extracting it into a single constant avoids drift if the layout is adjusted later (e.g., adding a column).
Also applies to: 867-867
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/market/Market.js` at line 766, Extract the repeated "2fr 1fr
1fr 1fr 1fr 1fr 1fr" grid-template string into a single constant (e.g., const
GRID_TEMPLATE_COLUMNS = "2fr 1fr 1fr 1fr 1fr 1fr 1fr") at the top of the Market
component and replace the inline style usages in the row JSX and the header JSX
with style={{ gridTemplateColumns: GRID_TEMPLATE_COLUMNS }} (or use the constant
in a style object) so both the row div (the element with class "grid gap-4 p-4
hover:bg-[`#242d3d`] transition-colors") and the header render reference the same
constant.
Comment on lines
+275
to
+278
| const makerAddress = typeof item.address === 'string' | ||
| ? item.address | ||
| : `${item.address?.onion_addr || ''}:${item.address?.port || ''}`; | ||
|
|
There was a problem hiding this comment.
Same ":<port>"/":" address sentinel risk as in Market.js, with worse consequences here.
When item.address is an object with falsy onion_addr, this constructs ":<port>" (or ":" if both are falsy). The resulting string then becomes the maker's identity used everywhere downstream:
- It is stored in
selectedMakerAddressesand compared withSet/includes, so two malformed entries collide on the same key. - It is forwarded to the backend as a
preferredMaker(via thecoinswap:startIPC) — a clearly invalid Tor endpoint.
Mirror the Market.js fix and require onion_addr before constructing the address, otherwise drop the maker (since it cannot be selected meaningfully anyway):
🛡️ Proposed guard
.map((item, index) => {
const offer = item.offer;
- const makerAddress = typeof item.address === 'string'
- ? item.address
- : `${item.address?.onion_addr || ''}:${item.address?.port || ''}`;
+ let makerAddress;
+ if (typeof item.address === 'string') {
+ makerAddress = item.address;
+ } else if (item.address?.onion_addr) {
+ makerAddress = item.address.port
+ ? `${item.address.onion_addr}:${item.address.port}`
+ : item.address.onion_addr;
+ } else {
+ makerAddress = '';
+ }…and then filter out entries with empty makerAddress before they enter availableMakers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/swap/Swap.js` around lines 275 - 278, The code builds a
malformed makerAddress when item.address is an object but onion_addr is falsy,
producing strings like ":<port>" or ":" which pollute selectedMakerAddresses,
availableMakers and are sent as invalid preferredMaker via the coinswap:start
IPC; change the makerAddress construction in Swap.js to require a truthy
item.address.onion_addr (i.e., only construct
`${item.address.onion_addr}:${item.address.port}` when onion_addr exists) and
otherwise treat the maker as invalid (set makerAddress to empty/null) and then
filter out entries with empty makerAddress before populating availableMakers or
using selectedMakerAddresses / sending preferredMaker.
Comment on lines
+751
to
+763
| const makersLabelEl = content.querySelector('#makers-label'); | ||
| let selectedMakersText; | ||
| if (makerSelectionMode === 'manual' && selectedMakerAddresses.length > 0) { | ||
| selectedMakersText = | ||
| selectedMakerAddresses.map((addr) => formatTorEndpoint(addr)).join(', '); | ||
| if (makersLabelEl) makersLabelEl.textContent = 'Selected Makers'; | ||
| } else { | ||
| selectedMakersText = | ||
| getTopCandidateMakers() | ||
| .map((maker) => formatTorEndpoint(maker.address)) | ||
| .join(', ') || 'None selected'; | ||
| if (makersLabelEl) makersLabelEl.textContent = 'Top Maker Candidates'; | ||
| } |
There was a problem hiding this comment.
Summary label shows "Top Maker Candidates" even when user is in manual mode.
In manual mode with zero selections yet, the condition makerSelectionMode === 'manual' && selectedMakerAddresses.length > 0 falls through to the else branch, which sets the label to "Top Maker Candidates" and lists the auto picks. That contradicts the user's chosen mode and the manual maker-selection panel below it. Consider keeping the manual label and showing "None selected":
♻️ Proposed adjustment
- if (makerSelectionMode === 'manual' && selectedMakerAddresses.length > 0) {
- selectedMakersText =
- selectedMakerAddresses.map((addr) => formatTorEndpoint(addr)).join(', ');
- if (makersLabelEl) makersLabelEl.textContent = 'Selected Makers';
- } else {
+ if (makerSelectionMode === 'manual') {
+ selectedMakersText =
+ selectedMakerAddresses.length > 0
+ ? selectedMakerAddresses.map((addr) => formatTorEndpoint(addr)).join(', ')
+ : 'None selected';
+ if (makersLabelEl) makersLabelEl.textContent = 'Selected Makers';
+ } else {
selectedMakersText =
getTopCandidateMakers()
.map((maker) => formatTorEndpoint(maker.address))
.join(', ') || 'None selected';
if (makersLabelEl) makersLabelEl.textContent = 'Top Maker Candidates';
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/swap/Swap.js` around lines 751 - 763, The label and displayed
text currently switch to auto mode when makerSelectionMode === 'manual' but
selectedMakerAddresses is empty; change the logic in the block around
makersLabelEl / makerSelectionMode / selectedMakerAddresses so that when
makerSelectionMode === 'manual' you always set makersLabelEl.textContent =
'Selected Makers' and selectedMakersText = selectedMakerAddresses.length ?
selectedMakerAddresses.map(addr => formatTorEndpoint(addr)).join(', ') : 'None
selected'; only use getTopCandidateMakers() and set the 'Top Maker Candidates'
label when makerSelectionMode !== 'manual'.
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
1 participant
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.
Summary by CodeRabbit
New Features
Improvements
Style