move Monero account payload to package#4586
Open
sdmg15 wants to merge 2 commits intobisq-network:mainfrom
Open
move Monero account payload to package#4586sdmg15 wants to merge 2 commits intobisq-network:mainfrom
sdmg15 wants to merge 2 commits intobisq-network:mainfrom
Conversation
WalkthroughRelocates Monero account classes into a new subpackage, adds Monero cryptography (Keccak, Base58, Ed25519 math), WalletAddress/subaddress derivation, a Monero subaddress service, UI wiring for subaddress creation, and accompanying license and utility files. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Input
participant Controller as MoneroFormController
participant Service as MoneroSubaddressService
participant WalletAddr as WalletAddress
participant Crypto as CryptoUtil/Ed25519
participant Base58 as Base58
User->>Controller: createSubAddress(mainAddress, privateViewKey, acctId, subId)
Controller->>Controller: validate inputs, parse indices
Controller->>Service: generateSubAddress(mainAddress, privateViewKey, acctId, subId)
Service->>WalletAddr: new WalletAddress(mainAddress)
WalletAddr->>Base58: decode Base58 payload
WalletAddr->>Crypto: compute hashToScalar / group ops for subaddress keys
Crypto-->>WalletAddr: derived key bytes
WalletAddr->>Base58: encode subaddress payload
Base58-->>Service: Base58 subaddress string
Service-->>Controller: subaddress string
Controller-->>User: return subaddress
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java (2)
82-89:⚠️ Potential issue | 🟠 MajorDon't persist subaddress-only fields when subaddresses are disabled.
createSubAddress()now uses""as the invalid/incomplete-input sentinel, butcreateAccountPayload()still wraps the Monero-only fields withOptional.ofNullable(...). That turns blank strings into present values, and a private view key entered before toggling the checkbox off will still be saved on a non-subaddress account. Gate these optionals onuseSubAddressesand normalize blanks toOptional.empty().Suggested fix
boolean useSubAddresses = model.getUseSubAddresses().get(); - Optional<String> mainAddress = Optional.ofNullable(model.getMainAddress().get()); - Optional<String> privateViewKey = Optional.ofNullable(model.getPrivateViewKey().get()); - Optional<String> subAddress = Optional.ofNullable(model.getSubAddress().get()); - Optional<Integer> accountIndex = Optional.ofNullable(model.getAccountIndex().get()) - .map(Integer::valueOf); - Optional<Integer> initialSubAddressIndex = Optional.ofNullable(model.getInitialSubAddressIndex().get()) - .map(Integer::valueOf); + Optional<String> mainAddress = useSubAddresses && !StringUtils.isEmpty(model.getMainAddress().get()) + ? Optional.of(model.getMainAddress().get()) + : Optional.empty(); + Optional<String> privateViewKey = useSubAddresses && !StringUtils.isEmpty(model.getPrivateViewKey().get()) + ? Optional.of(model.getPrivateViewKey().get()) + : Optional.empty(); + Optional<String> subAddress = useSubAddresses && !StringUtils.isEmpty(model.getSubAddress().get()) + ? Optional.of(model.getSubAddress().get()) + : Optional.empty(); + Optional<Integer> accountIndex = useSubAddresses && !StringUtils.isEmpty(model.getAccountIndex().get()) + ? Optional.of(Integer.valueOf(model.getAccountIndex().get())) + : Optional.empty(); + Optional<Integer> initialSubAddressIndex = useSubAddresses && !StringUtils.isEmpty(model.getInitialSubAddressIndex().get()) + ? Optional.of(Integer.valueOf(model.getInitialSubAddressIndex().get())) + : Optional.empty();Also applies to: 116-117
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java` around lines 82 - 89, The Optional-wrapping of Monero-only fields (e.g., model.getMainAddress(), getPrivateViewKey(), getSubAddress(), getAccountIndex(), getInitialSubAddressIndex()) currently turns blank-string sentinels into present values and doesn't respect the useSubAddresses flag; update createAccountPayload (and the same pattern around createSubAddress) to first check useSubAddresses and to normalize empty or "" strings into Optional.empty() (e.g., treat "" or null as absent), only constructing Optionals for these fields when useSubAddresses is true, and for numeric fields parse/map only non-blank strings to Integer::valueOf otherwise return Optional.empty().
47-52:⚠️ Potential issue | 🟠 MajorKeep the reactive subaddress path exception-safe and quiet.
This runs on every edit.
WalletAddress(String)andWalletAddress.getSubaddressBase58(...)still throw runtime failures for malformed input (account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java:38-59and:128-133), so the current catches are too narrow. Theerrorstack traces will also spam normal validation failures, and theinfolog persists every generated subaddress.Suggested fix
- log.info("Created new subAddress {}. Took {} ms.", subAddress, System.currentTimeMillis() - start); + log.debug("Created Monero subaddress in {} ms.", System.currentTimeMillis() - start); return subAddress; - } catch (IllegalArgumentException e) { - log.error("Monero subaddress validation failed", e); + } catch (WalletAddress.InvalidWalletAddressException e) { + log.debug("Monero subaddress generation failed", e); return ""; - } catch (WalletAddress.InvalidWalletAddressException e) { - log.error("WalletAddress.getSubaddressBase58 failed", e); + } catch (RuntimeException e) { + log.debug("Monero subaddress generation failed", e); return ""; }Also applies to: 122-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java` around lines 47 - 52, The reactive subscription that builds subaddresses via createSubAddress (used in createSubAddressPin and the similar block at lines ~122-146) is not exception-safe: WalletAddress constructors and getSubaddressBase58 can throw runtime exceptions on malformed input, causing noisy error logs and stack traces on every keystroke; update the createSubAddress invocation so it catches Throwable (or at least Exception) around the address construction/derivation, return a safe null/empty value when generation fails, and avoid logging full stack traces on validation failures (log at debug/trace or a single concise message without stack trace), ensuring the EasyBind subscription never throws. Ensure the same pattern is applied to both createSubAddressPin and the other createSubAddress usage block.
🧹 Nitpick comments (6)
account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java (1)
14-14: Consider makingbytesfield private with a getter.The public mutable field exposes internal state. Per coding guidelines, use Lombok
@Getterfor accessor generation.♻️ Suggested change
+import lombok.Getter; + public class Scalar { - public byte[] bytes; + `@Getter` + private final byte[] bytes;Note: If callers mutate the array, consider returning a defensive copy in the getter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java` at line 14, Make the public mutable field bytes in class Scalar private and add a Lombok-generated accessor; change the declaration of the field bytes to private and annotate the class (or field) with `@Getter` so callers use the accessor instead of directly accessing the array, and if callers may mutate the returned array return a defensive copy from the getter (i.e., ensure getBytes() returns Arrays.copyOf(bytes, bytes.length) if mutation safety is required).account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/HexEncoder.java (1)
18-24: Thecatch(Exception)is overly broad but acceptable.
ByteUtil.hexToBytesonly throws unchecked exceptions, so a narrowercatch(RuntimeException)would be more precise. However, the current approach is acceptable for this thin wrapper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/HexEncoder.java` around lines 18 - 24, Change the overly broad catch in HexEncoder.getBytes to only catch RuntimeException instead of Exception: update the catch clause in the getBytes method (class HexEncoder, method getBytes) from catch(Exception e) to catch(RuntimeException e) so it matches the unchecked exceptions ByteUtil.hexToBytes throws and preserves the existing IllegalArgumentException wrapping.account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java (1)
7-14: Consider making constant fieldsfinal.These static fields are constants and should be declared
finalto prevent accidental modification.♻️ Suggested change
- public static String alphabet = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz"; + public static final String alphabet = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz"; - public static int[] encoded_block_sizes = new int[]{0, 2, 3, 5, 6, 7, 9, 10, 11}; + public static final int[] encoded_block_sizes = new int[]{0, 2, 3, 5, 6, 7, 9, 10, 11}; - public static int full_block_size = 8; - public static int full_encoded_block_size = 11; + public static final int full_block_size = 8; + public static final int full_encoded_block_size = 11; - public static BigInteger UINT64_MAX = BigInteger.valueOf(2).pow(64); + public static final BigInteger UINT64_MAX = BigInteger.valueOf(2).pow(64);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java` around lines 7 - 14, The fields alphabet, encoded_block_sizes, full_block_size, full_encoded_block_size and UINT64_MAX are mutable static constants—declare them as final to prevent modification; specifically change their declarations to static final for alphabet, full_block_size, full_encoded_block_size and UINT64_MAX, and make encoded_block_sizes an immutable constant (e.g., static final int[] encoded_block_sizes and ensure callers cannot mutate it by returning copies or replacing it with an unmodifiable List), referencing the symbols alphabet, encoded_block_sizes, full_block_size, full_encoded_block_size, and UINT64_MAX in Base58.account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.java (2)
40-40: Naming: Use uppercase for constant field.Static final fields representing constants should use
UPPER_SNAKE_CASE. The Ed25519 group orderl(ell) is a cryptographic constant.✏️ Suggested rename
- public static BigInteger l = BigInteger.valueOf(2).pow(252).add(new BigInteger("27742317777372353535851937790883648493")); + public static final BigInteger L = BigInteger.valueOf(2).pow(252).add(new BigInteger("27742317777372353535851937790883648493"));Note: Update all usages of
ltoLthroughout the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.java` at line 40, The field CryptoUtil.l should be renamed and declared as a proper constant: change the declaration to static final and rename from l to L (UPPER_SNAKE_CASE or at least uppercase L) to reflect the Ed25519 group order constant; update all usages of CryptoUtil.l to CryptoUtil.L across the codebase and ensure imports/compilations pass after the rename.
57-58: Missing braces on single-line conditionals.As per coding guidelines, always use braces for
if/elseblocks.✏️ Proposed fix
- if (r.length > 32) System.arraycopy(r, 1, s, 0, s.length); - else System.arraycopy(r, 0, s, 32 - r.length, r.length); + if (r.length > 32) { + System.arraycopy(r, 1, s, 0, s.length); + } else { + System.arraycopy(r, 0, s, 32 - r.length, r.length); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.java` around lines 57 - 58, The single-line if/else in CryptoUtil that performs System.arraycopy calls lacks braces; update the conditional to use explicit braces for both the if and else branches (wrap the existing System.arraycopy(r, 1, s, 0, s.length); and System.arraycopy(r, 0, s, 32 - r.length, r.length); calls inside { } blocks) to comply with coding guidelines and avoid ambiguity in the CryptoUtil class.account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java (1)
101-102: Consider usingIllegalArgumentExceptionfor invalid input.
RuntimeExceptionis generic. Since this validates method arguments (accountId/subaddressId must not both be zero),IllegalArgumentExceptionis more semantically appropriate.✏️ Suggested change
if (accountId == 0 && subaddressId == 0) - throw new RuntimeException("Not to be called for the base wallet address"); + throw new IllegalArgumentException("Not to be called for the base wallet address");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java` around lines 101 - 102, The constructor/validation in WalletAddress currently throws a generic RuntimeException when both accountId and subaddressId are zero; replace that with throwing an IllegalArgumentException to better indicate invalid input (i.e., change the throw in the WalletAddress code path that checks accountId == 0 && subaddressId == 0 to throw new IllegalArgumentException with a clear message about base wallet address not allowed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java`:
- Around line 88-91: The input validation in encode_block incorrectly compares
data.length to full_encoded_block_size (11) instead of the raw-data max
full_block_size (8); update the conditional in encode_block to throw only when
data.length < 1 || data.length > full_block_size, referencing the encode_block
method and the constants full_block_size and full_encoded_block_size so callers
and future readers see the correct validation.
- Line 1: The package declaration in Base58.java is incorrect: change the
current package line "package bisq.core.xmr.knaccc.monero.address;" to match the
directory structure (e.g. "package
bisq.account.accounts.crypto.monero.knaccc.monero.address;") so the package name
aligns with the file path and the class Base58 can be resolved; update any other
files in this package that import Base58 if needed and run a build to verify no
import errors remain.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.java`:
- Line 1: The package declaration in ByteUtil.java is incorrect (declares
bisq.core.xmr.knaccc.monero.address) and must match the file path; update the
package line in ByteUtil.java to the correct package namespace used by this
module (e.g., bisq.account.accounts.crypto.monero.knaccc.monero.address) so
classes like ByteUtil resolve correctly with imports and build artifacts.
- Around line 25-33: The hexToBytes method in ByteUtil silently produces invalid
output for odd-length strings or non-hex chars; add input validation at the
start of hexToBytes to (1) throw IllegalArgumentException for null or odd-length
strings, (2) for each nibble call Character.digit and if it returns -1 throw
IllegalArgumentException indicating the offending character and index, and (3)
only then compute the byte from the two validated nibble values—this ensures
hexToBytes fails fast and reports precise errors.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.java`:
- Around line 28-38: The static Keccak instance (keccak) is not thread-safe and
causes race conditions in fastHash(byte[] a); replace the shared mutable field
with a thread-confined Keccak instance — either instantiate a new Keccak(256)
inside fastHash before calling reset(), update(), and digestArray(), or change
the static field to a ThreadLocal<Keccak> that returns a fresh Keccak(256) per
thread and use that in fastHash. Ensure calls still use reset(), update(a) and
digestArray() on the thread-local/new instance and remove reliance on the shared
keccak field to eliminate concurrent state corruption.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java`:
- Around line 1-11: The package declaration and imports in Scalar.java are
incorrect for its file path; change the package line to match the source tree
(e.g., package bisq.account.accounts.crypto.monero.knaccc.monero.crypto;) and
update all imports and static imports that reference bisq.core... to the
matching bisq.account... package (replace references to
bisq.core.xmr.knaccc.monero.address.ByteUtil and
bisq.core.xmr.knaccc.monero.crypto.CryptoUtil with their bisq.account.*
equivalents), keeping the same imported symbols (ByteUtil, bytesToHex,
getBigIntegerFromUnsignedLittleEndianByteArray,
ensure32BytesAndConvertToLittleEndian, l) so class Scalar and its uses resolve
correctly.
- Around line 29-32: The current Scalar.equals(Object obj) implementation is
unsafe (no null or instanceof checks) and the class lacks a corresponding
hashCode override; update equals in class Scalar to first check (this == obj),
then (obj instanceof Scalar) before casting, compare byte arrays with
Arrays.equals(this.bytes, other.bytes), and add a matching public int hashCode()
that returns Arrays.hashCode(bytes); alternatively annotate the class with
Lombok's `@EqualsAndHashCode` to generate both methods and ensure they use the
bytes field.
In `@account/src/main/java/bisq/account/accounts/crypto/monero/org/LICENSE`:
- Around line 1-21: Add upstream provenance metadata to the existing LICENSE
file in monero/org by appending a short provenance block that lists the original
project name (NEM), the upstream source URL, the exact upstream version or
release tag, and the commit hash (if available), plus the date you imported it;
update the header to retain the MIT text and include a one-line notice such as
"Upstream: <project> <url> <version/commit> <date>" so auditors can trace the
origin.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/Curve.java`:
- Line 1: The package declaration at the top of Curve.java ("package
bisq.core.xmr.org.nem.core.crypto") does not match the file's actual location
and will fail compilation; update the package declaration to the correct package
that reflects the directory structure containing the Curve class (so it matches
where the class is compiled) — locate the top of the file in class Curve and
replace the existing package line with the proper package name that corresponds
to the source tree (ensuring all other classes that import Curve use the same
package).
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/CoordinateSystem.java`:
- Line 1: The package declaration at the top of CoordinateSystem.java is
incorrect; update the package statement to match the file path so the class
CoordinateSystem is compiled in the correct namespace (change from
"bisq.core.xmr.org.nem.core.crypto.ed25519.arithmetic" to
"bisq.account.accounts.crypto.monero.org.nem.core.crypto.ed25519.arithmetic"),
then recompile to confirm the class resolves correctly.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedGroupElement.java`:
- Around line 16-21: The class stores and exposes the caller's byte[] directly,
making instances mutable; fix by defensively copying on ingress and egress: in
the Ed25519EncodedGroupElement(byte[] values) constructor (and any other
constructor overload that accepts a byte[]), assign this.values = values.clone()
instead of the original reference, and in getRaw() return values.clone() rather
than the backing array; this preserves immutability and prevents external
mutation from breaking equals()/hashCode() or decoded coordinates.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Field.java`:
- Around line 23-24: PUBLIC mutable byte-array constants ZERO_SHORT and
ZERO_LONG in Ed25519Field can be mutated by callers; make them private (private
static final byte[] ZERO_SHORT/ ZERO_LONG) and replace external access with
accessor methods (e.g., public static byte[] getZeroShort() and getZeroLong())
that return a defensive copy (return ZERO_SHORT.clone()). Update all uses of
ZERO_SHORT and ZERO_LONG to call the new getters (or, alternatively, remove the
fields and implement the getters to allocate new byte[] values) so callers
cannot mutate shared state.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ArrayUtils.java`:
- Line 1: The package declaration at the top of ArrayUtils.java is incorrect;
update the package statement in the ArrayUtils class (class name: ArrayUtils) to
match the file path by changing it to "package
bisq.account.accounts.crypto.monero.org.nem.core.utils;" and then adjust any
imports/references in this file or other classes if necessary so they use the
new package name.
- Around line 113-121: The isEqualConstantTime method can throw
ArrayIndexOutOfBoundsException when b and c differ in length because the loop
uses b.length as the bound but reads c[i]; change the loop to iterate to
Math.max(b.length, c.length) and for each i use conditional reads (e.g. bi = i <
b.length ? b[i] : 0; ci = i < c.length ? c[i] : 0) and OR their XOR into result;
keep the existing result |= b.length - c.length and return
ByteUtils.isEqualConstantTime(result, 0) as before (refer to isEqualConstantTime
and ByteUtils.isEqualConstantTime).
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.java`:
- Line 1: The package declaration at the top of ByteUtils.java is incorrect;
change the package from "bisq.core.xmr.org.nem.core.utils" to match the file
path's package "bisq.account.accounts.crypto.monero.org.nem.core.utils" so the
class ByteUtils is in the correct package, then update any affected
imports/usages across the codebase (references to ByteUtils) to the new package
name.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/HexEncoder.java`:
- Around line 1-3: The package declaration and import in HexEncoder.java are
incorrect for its file path; update the package declaration to match the
directory hierarchy (e.g., change package bisq.core.xmr.org.nem.core.utils to
bisq.account.accounts.crypto.monero.org.nem.core.utils) and fix the import to
the correct ByteUtil location (replace
bisq.core.xmr.knaccc.monero.address.ByteUtil with
bisq.account.accounts.crypto.monero.address.ByteUtil) so the class HexEncoder
and referenced symbol ByteUtil resolve correctly.
---
Outside diff comments:
In
`@apps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java`:
- Around line 82-89: The Optional-wrapping of Monero-only fields (e.g.,
model.getMainAddress(), getPrivateViewKey(), getSubAddress(), getAccountIndex(),
getInitialSubAddressIndex()) currently turns blank-string sentinels into present
values and doesn't respect the useSubAddresses flag; update createAccountPayload
(and the same pattern around createSubAddress) to first check useSubAddresses
and to normalize empty or "" strings into Optional.empty() (e.g., treat "" or
null as absent), only constructing Optionals for these fields when
useSubAddresses is true, and for numeric fields parse/map only non-blank strings
to Integer::valueOf otherwise return Optional.empty().
- Around line 47-52: The reactive subscription that builds subaddresses via
createSubAddress (used in createSubAddressPin and the similar block at lines
~122-146) is not exception-safe: WalletAddress constructors and
getSubaddressBase58 can throw runtime exceptions on malformed input, causing
noisy error logs and stack traces on every keystroke; update the
createSubAddress invocation so it catches Throwable (or at least Exception)
around the address construction/derivation, return a safe null/empty value when
generation fails, and avoid logging full stack traces on validation failures
(log at debug/trace or a single concise message without stack trace), ensuring
the EasyBind subscription never throws. Ensure the same pattern is applied to
both createSubAddressPin and the other createSubAddress usage block.
---
Nitpick comments:
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java`:
- Around line 7-14: The fields alphabet, encoded_block_sizes, full_block_size,
full_encoded_block_size and UINT64_MAX are mutable static constants—declare them
as final to prevent modification; specifically change their declarations to
static final for alphabet, full_block_size, full_encoded_block_size and
UINT64_MAX, and make encoded_block_sizes an immutable constant (e.g., static
final int[] encoded_block_sizes and ensure callers cannot mutate it by returning
copies or replacing it with an unmodifiable List), referencing the symbols
alphabet, encoded_block_sizes, full_block_size, full_encoded_block_size, and
UINT64_MAX in Base58.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java`:
- Around line 101-102: The constructor/validation in WalletAddress currently
throws a generic RuntimeException when both accountId and subaddressId are zero;
replace that with throwing an IllegalArgumentException to better indicate
invalid input (i.e., change the throw in the WalletAddress code path that checks
accountId == 0 && subaddressId == 0 to throw new IllegalArgumentException with a
clear message about base wallet address not allowed).
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.java`:
- Line 40: The field CryptoUtil.l should be renamed and declared as a proper
constant: change the declaration to static final and rename from l to L
(UPPER_SNAKE_CASE or at least uppercase L) to reflect the Ed25519 group order
constant; update all usages of CryptoUtil.l to CryptoUtil.L across the codebase
and ensure imports/compilations pass after the rename.
- Around line 57-58: The single-line if/else in CryptoUtil that performs
System.arraycopy calls lacks braces; update the conditional to use explicit
braces for both the if and else branches (wrap the existing System.arraycopy(r,
1, s, 0, s.length); and System.arraycopy(r, 0, s, 32 - r.length, r.length);
calls inside { } blocks) to comply with coding guidelines and avoid ambiguity in
the CryptoUtil class.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java`:
- Line 14: Make the public mutable field bytes in class Scalar private and add a
Lombok-generated accessor; change the declaration of the field bytes to private
and annotate the class (or field) with `@Getter` so callers use the accessor
instead of directly accessing the array, and if callers may mutate the returned
array return a defensive copy from the getter (i.e., ensure getBytes() returns
Arrays.copyOf(bytes, bytes.length) if mutation safety is required).
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/HexEncoder.java`:
- Around line 18-24: Change the overly broad catch in HexEncoder.getBytes to
only catch RuntimeException instead of Exception: update the catch clause in the
getBytes method (class HexEncoder, method getBytes) from catch(Exception e) to
catch(RuntimeException e) so it matches the unchecked exceptions
ByteUtil.hexToBytes throws and preserves the existing IllegalArgumentException
wrapping.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 20009e64-e1ff-4adb-92cd-9a15e11b10ff
📒 Files selected for processing (31)
account/src/main/java/bisq/account/accounts/crypto/CryptoAssetAccount.javaaccount/src/main/java/bisq/account/accounts/crypto/CryptoAssetAccountPayload.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/MoneroAccount.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/MoneroAccountPayload.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/com/joemelsha/crypto/hash/Keccak.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/LICENSEaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/LICENSEaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/Curve.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/Ed25519Curve.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/CoordinateSystem.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedFieldElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedGroupElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Field.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519FieldElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Group.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519GroupElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ArrayUtils.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/HexEncoder.javaaccount/src/main/java/bisq/account/bisq1_import/crypto/ImportMoneroAccountParser.javaaccount/src/test/java/bisq/account/accounts/AccountPayloadBisq1CompatibleFingerprintTest.javaapps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/CryptoAssetAccountsController.javaapps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.javaapps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/summary/PaymentSummaryController.javaapps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/summary/details/MoneroSummaryDetails.javaapps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/details/MoneroAccountDetails.java
account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java
Outdated
Show resolved
Hide resolved
Comment on lines
+88
to
+91
| private static void encode_block(byte[] data, char[] buf, int index) { | ||
| if (data.length < 1 || data.length > full_encoded_block_size) { | ||
| throw new RuntimeException("Invalid block length: " + data.length); | ||
| } |
There was a problem hiding this comment.
Input validation in encode_block checks wrong constant.
The method receives raw data blocks (max 8 bytes = full_block_size), but the validation checks against full_encoded_block_size (11). While callers currently pass valid sizes, the validation is misleading.
🐛 Proposed fix
private static void encode_block(byte[] data, char[] buf, int index) {
- if (data.length < 1 || data.length > full_encoded_block_size) {
+ if (data.length < 1 || data.length > full_block_size) {
throw new RuntimeException("Invalid block length: " + data.length);
}📝 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
| private static void encode_block(byte[] data, char[] buf, int index) { | |
| if (data.length < 1 || data.length > full_encoded_block_size) { | |
| throw new RuntimeException("Invalid block length: " + data.length); | |
| } | |
| private static void encode_block(byte[] data, char[] buf, int index) { | |
| if (data.length < 1 || data.length > full_block_size) { | |
| throw new RuntimeException("Invalid block length: " + data.length); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java`
around lines 88 - 91, The input validation in encode_block incorrectly compares
data.length to full_encoded_block_size (11) instead of the raw-data max
full_block_size (8); update the conditional in encode_block to throw only when
data.length < 1 || data.length > full_block_size, referencing the encode_block
method and the constants full_block_size and full_encoded_block_size so callers
and future readers see the correct validation.
account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.java
Outdated
Show resolved
Hide resolved
Comment on lines
+25
to
+33
| public static byte[] hexToBytes(String s) { | ||
| int len = s.length(); | ||
| byte[] data = new byte[len / 2]; | ||
| for (int i = 0; i < len; i += 2) { | ||
| data[i / 2] = (byte) ((Character.digit(s.charAt(i), 16) << 4) | ||
| + Character.digit(s.charAt(i + 1), 16)); | ||
| } | ||
| return data; | ||
| } |
There was a problem hiding this comment.
hexToBytes silently produces incorrect output for invalid input.
Character.digit returns -1 for invalid hex characters, which will silently corrupt the output. Odd-length strings will skip the last character. Consider adding validation.
🛡️ Suggested validation
public static byte[] hexToBytes(String s) {
int len = s.length();
+ if (len % 2 != 0) {
+ throw new IllegalArgumentException("Hex string must have even length");
+ }
byte[] data = new byte[len / 2];
for (int i = 0; i < len; i += 2) {
- data[i / 2] = (byte) ((Character.digit(s.charAt(i), 16) << 4)
- + Character.digit(s.charAt(i + 1), 16));
+ int high = Character.digit(s.charAt(i), 16);
+ int low = Character.digit(s.charAt(i + 1), 16);
+ if (high == -1 || low == -1) {
+ throw new IllegalArgumentException("Invalid hex character at position " + i);
+ }
+ data[i / 2] = (byte) ((high << 4) + low);
}
return data;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.java`
around lines 25 - 33, The hexToBytes method in ByteUtil silently produces
invalid output for odd-length strings or non-hex chars; add input validation at
the start of hexToBytes to (1) throw IllegalArgumentException for null or
odd-length strings, (2) for each nibble call Character.digit and if it returns
-1 throw IllegalArgumentException indicating the offending character and index,
and (3) only then compute the byte from the two validated nibble values—this
ensures hexToBytes fails fast and reports precise errors.
Comment on lines
+28
to
+38
| private static Keccak keccak = new Keccak(256); | ||
|
|
||
| public static byte[] fastHash(byte[] a) { | ||
| try { | ||
| keccak.reset(); | ||
| keccak.update(a); | ||
| return keccak.digestArray(); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Race condition on shared mutable Keccak instance.
The static keccak instance is shared across all threads without synchronization. When fastHash() is called concurrently, threads interleave reset(), update(), and digestArray() calls on the same instance, corrupting internal state and producing incorrect hash outputs.
This breaks cryptographic correctness for Monero subaddress generation.
🔒 Proposed fix: Use ThreadLocal for thread-safe instance reuse
- private static Keccak keccak = new Keccak(256);
+ private static final ThreadLocal<Keccak> keccakThreadLocal =
+ ThreadLocal.withInitial(() -> new Keccak(256));
public static byte[] fastHash(byte[] a) {
try {
+ Keccak keccak = keccakThreadLocal.get();
keccak.reset();
keccak.update(a);
return keccak.digestArray();
} catch (Exception e) {
throw new RuntimeException(e);
}
}Alternative: Create a new Keccak instance per call if performance is not critical.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.java`
around lines 28 - 38, The static Keccak instance (keccak) is not thread-safe and
causes race conditions in fastHash(byte[] a); replace the shared mutable field
with a thread-confined Keccak instance — either instantiate a new Keccak(256)
inside fastHash before calling reset(), update(), and digestArray(), or change
the static field to a ThreadLocal<Keccak> that returns a fresh Keccak(256) per
thread and use that in fastHash. Ensure calls still use reset(), update(a) and
digestArray() on the thread-local/new instance and remove reliance on the shared
keccak field to eliminate concurrent state corruption.
Comment on lines
+23
to
+24
| public static final byte[] ZERO_SHORT = new byte[32]; | ||
| public static final byte[] ZERO_LONG = new byte[64]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and examine the file
find . -name "Ed25519Field.java" -type fRepository: bisq-network/bisq2
Length of output: 178
🏁 Script executed:
# Read the file to see the context around lines 23-24
cat -n "account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Field.java"Repository: bisq-network/bisq2
Length of output: 2064
🏁 Script executed:
# Search for usages of ZERO_SHORT and ZERO_LONG
rg "ZERO_SHORT|ZERO_LONG" -A 3 -B 1Repository: bisq-network/bisq2
Length of output: 2304
🏁 Script executed:
# Find and read the Ed25519EncodedFieldElement file
find . -name "Ed25519EncodedFieldElement.java" -type fRepository: bisq-network/bisq2
Length of output: 192
🏁 Script executed:
# Read the Ed25519EncodedFieldElement file to understand how this.zero is used
find . -name "Ed25519EncodedFieldElement.java" -type f -exec cat -n {} \;Repository: bisq-network/bisq2
Length of output: 40127
🏁 Script executed:
# Search for any mutations to arrays from Ed25519Field constants
rg "ZERO_SHORT|ZERO_LONG" -B 3 -A 5 --type javaRepository: bisq-network/bisq2
Length of output: 3619
🏁 Script executed:
# Search for any assignments or mutations to ZERO_SHORT or ZERO_LONG
rg "ZERO_SHORT|ZERO_LONG" -A 2 -B 2 | grep -E "(\[.*\]|=|clone|copy)" | head -30Repository: bisq-network/bisq2
Length of output: 1130
🏁 Script executed:
# Check the getRaw() method usage to see if callers could mutate the exposed arrays
rg "getRaw\(\)" -A 3 -B 1Repository: bisq-network/bisq2
Length of output: 8407
🏁 Script executed:
# Look for any array modifications in the Ed25519 crypto package
rg "\.zero\s*\[" --type javaRepository: bisq-network/bisq2
Length of output: 44
Avoid public mutable byte-array constants here.
public static final byte[] is still mutable in Java. Direct write access like ZERO_SHORT[0] = 1 could mutate global crypto state for all callers. Hide these arrays behind cloning accessors or use immutable structures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Field.java`
around lines 23 - 24, PUBLIC mutable byte-array constants ZERO_SHORT and
ZERO_LONG in Ed25519Field can be mutated by callers; make them private (private
static final byte[] ZERO_SHORT/ ZERO_LONG) and replace external access with
accessor methods (e.g., public static byte[] getZeroShort() and getZeroLong())
that return a defensive copy (return ZERO_SHORT.clone()). Update all uses of
ZERO_SHORT and ZERO_LONG to call the new getters (or, alternatively, remove the
fields and implement the getters to allocate new byte[] values) so callers
cannot mutate shared state.
account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ArrayUtils.java
Outdated
Show resolved
Hide resolved
Comment on lines
+113
to
+121
| public static int isEqualConstantTime(final byte[] b, final byte[] c) { | ||
| int result = 0; | ||
| result |= b.length - c.length; | ||
| for (int i = 0; i < b.length; i++) { | ||
| result |= b[i] ^ c[i]; | ||
| } | ||
|
|
||
| return ByteUtils.isEqualConstantTime(result, 0); | ||
| } |
There was a problem hiding this comment.
Potential ArrayIndexOutOfBoundsException when arrays have different lengths.
If c.length < b.length, the loop at line 116-118 will throw ArrayIndexOutOfBoundsException when accessing c[i]. The method should safely handle mismatched lengths.
🐛 Proposed fix
public static int isEqualConstantTime(final byte[] b, final byte[] c) {
int result = 0;
result |= b.length - c.length;
- for (int i = 0; i < b.length; i++) {
+ int minLen = Math.min(b.length, c.length);
+ for (int i = 0; i < minLen; i++) {
result |= b[i] ^ c[i];
}
return ByteUtils.isEqualConstantTime(result, 0);
}📝 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
| public static int isEqualConstantTime(final byte[] b, final byte[] c) { | |
| int result = 0; | |
| result |= b.length - c.length; | |
| for (int i = 0; i < b.length; i++) { | |
| result |= b[i] ^ c[i]; | |
| } | |
| return ByteUtils.isEqualConstantTime(result, 0); | |
| } | |
| public static int isEqualConstantTime(final byte[] b, final byte[] c) { | |
| int result = 0; | |
| result |= b.length - c.length; | |
| int minLen = Math.min(b.length, c.length); | |
| for (int i = 0; i < minLen; i++) { | |
| result |= b[i] ^ c[i]; | |
| } | |
| return ByteUtils.isEqualConstantTime(result, 0); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ArrayUtils.java`
around lines 113 - 121, The isEqualConstantTime method can throw
ArrayIndexOutOfBoundsException when b and c differ in length because the loop
uses b.length as the bound but reads c[i]; change the loop to iterate to
Math.max(b.length, c.length) and for each i use conditional reads (e.g. bi = i <
b.length ? b[i] : 0; ci = i < c.length ? c[i] : 0) and OR their XOR into result;
keep the existing result |= b.length - c.length and return
ByteUtils.isEqualConstantTime(result, 0) as before (refer to isEqualConstantTime
and ByteUtils.isEqualConstantTime).
account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.java
Outdated
Show resolved
Hide resolved
account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/HexEncoder.java
Outdated
Show resolved
Hide resolved
sdmg15
force-pushed
the
trading/monero-subaddress
branch
from
6308b47 to
de9f36a
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.java (1)
55-61: Missing braces around if/else blocks.As per coding guidelines, always use braces for control structures.
♻️ Proposed fix
public static byte[] ensure32BytesAndConvertToLittleEndian(byte[] r) { byte[] s = new byte[32]; - if (r.length > 32) System.arraycopy(r, 1, s, 0, s.length); - else System.arraycopy(r, 0, s, 32 - r.length, r.length); + if (r.length > 32) { + System.arraycopy(r, 1, s, 0, s.length); + } else { + System.arraycopy(r, 0, s, 32 - r.length, r.length); + } reverseByteArrayInPlace(s); return s; }As per coding guidelines, follow K&R brace style and always use braces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.java` around lines 55 - 61, The method ensure32BytesAndConvertToLittleEndian currently uses if/else without braces; update the method in CryptoUtil.ensure32BytesAndConvertToLittleEndian to add K&R-style braces around both the if and the else blocks so both System.arraycopy calls are enclosed in { ... }, leaving logic and the reverseByteArrayInPlace(s) call unchanged to satisfy coding guidelines.account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ArrayUtils.java (1)
73-89: PotentialArrayIndexOutOfBoundsExceptionifBigInteger.toByteArray()returns empty array.While
BigInteger.toByteArray()typically returns at least one byte, checkingbigIntegerBytes[0]at line 77 without a length check is defensive-coding gap.🛡️ Proposed defensive check
public static byte[] toByteArray(final BigInteger value, final int numBytes) { final byte[] outputBytes = new byte[numBytes]; final byte[] bigIntegerBytes = value.toByteArray(); + if (bigIntegerBytes.length == 0) { + return outputBytes; + } int copyStartIndex = (0x00 == bigIntegerBytes[0]) ? 1 : 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ArrayUtils.java` around lines 73 - 89, The toByteArray method may throw ArrayIndexOutOfBoundsException when BigInteger.toByteArray() returns an empty array; update ArrayUtils.toByteArray to defensively handle an empty bigIntegerBytes before accessing bigIntegerBytes[0] by checking bigIntegerBytes.length == 0 and treating it as a single zero byte (or setting copyStartIndex/numBytesToCopy accordingly), then proceed with the existing logic that computes copyStartIndex, numBytesToCopy and fills outputBytes so outputBytes always gets populated safely even for empty or leading-zero BigInteger byte arrays.account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java (1)
7-14: Constants should befinalto prevent accidental modification.The class-level constants (
alphabet,encoded_block_sizes,full_block_size, etc.) are not declaredfinal, allowing them to be reassigned at runtime.♻️ Proposed fix
- public static String alphabet = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz"; + public static final String alphabet = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz"; - public static int[] encoded_block_sizes = new int[]{0, 2, 3, 5, 6, 7, 9, 10, 11}; + public static final int[] encoded_block_sizes = new int[]{0, 2, 3, 5, 6, 7, 9, 10, 11}; - public static int full_block_size = 8; - public static int full_encoded_block_size = 11; + public static final int full_block_size = 8; + public static final int full_encoded_block_size = 11; - public static BigInteger UINT64_MAX = BigInteger.valueOf(2).pow(64); + public static final BigInteger UINT64_MAX = BigInteger.valueOf(2).pow(64);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java` around lines 7 - 14, Make the class-level constants in Base58 immutable by declaring them final: change alphabet, encoded_block_sizes, full_block_size, full_encoded_block_size, and UINT64_MAX to be public static final (or private static final if they don't need external mutation) so they cannot be reassigned at runtime; for the array encoded_block_sizes also ensure immutability by either making it private static final and exposing an unmodifiable copy via a getter or by defensively copying it where used.account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java (1)
34-36: Extract complex expression for readability.The
addmethod is a single 200+ character line that's difficult to read and debug.♻️ Proposed refactor
public Scalar add(Scalar a) { - return new Scalar(ensure32BytesAndConvertToLittleEndian(getBigIntegerFromUnsignedLittleEndianByteArray(this.bytes).add(getBigIntegerFromUnsignedLittleEndianByteArray(a.bytes)).mod(l).toByteArray())); + BigInteger thisValue = getBigIntegerFromUnsignedLittleEndianByteArray(this.bytes); + BigInteger otherValue = getBigIntegerFromUnsignedLittleEndianByteArray(a.bytes); + BigInteger sum = thisValue.add(otherValue).mod(l); + return new Scalar(ensure32BytesAndConvertToLittleEndian(sum.toByteArray())); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java` around lines 34 - 36, The add method in class Scalar is a single long expression; refactor it for readability by breaking it into named intermediate variables inside Scalar.add: convert this.bytes and a.bytes to BigInteger using getBigIntegerFromUnsignedLittleEndianByteArray, compute sum = (biThis.add(biA)).mod(l), convert sum.toByteArray() to a byte[] variable, then call ensure32BytesAndConvertToLittleEndian on that byte[] and return new Scalar(...) — keep the same method calls and semantics but replace the inlined chain with these clearly named steps.account/src/main/java/bisq/account/accounts/crypto/monero/MoneroSubaddressService.java (1)
26-45: Unused service class duplicates logic inMoneroFormController.
MoneroSubaddressServiceprovides validation and subaddress generation, butMoneroFormController(lines 111-148) implements identical logic directly. Either:
- Inject and use this service in the controller, or
- Remove this unused class to avoid maintenance burden
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/MoneroSubaddressService.java` around lines 26 - 45, MoneroSubaddressService duplicates the validation and subaddress generation logic already implemented in MoneroFormController; either remove this unused class or refactor the controller to use it: inject MoneroSubaddressService into MoneroFormController (add a constructor parameter or `@Autowired` field) and replace the controller’s inline logic (the block that validates mainAddress/privateViewKey/accountIndex/subAddressIndex and calls WalletAddress.getSubaddressBase58) with a call to MoneroSubaddressService.generateSubAddress(mainAddress, privateViewKey, accountIndex, subAddressIndex); if you choose removal, delete MoneroSubaddressService and any imports/usages and ensure MoneroFormController remains the single canonical implementation and that existing unit/integration tests are updated accordingly.account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java (1)
24-30: Model the optional payment ID explicitly.
integratedPaymentIdis a bare nullable field, so the “present vs absent” state is implicit and has to be re-encoded with manualnullchecks later. PreferOptional<String>or mark the field@Nullableso the contract is explicit.As per coding guidelines, "Avoid nullable values; use
Optionaland@Nullableannotation where needed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java` around lines 24 - 30, The field integratedPaymentId in WalletAddress is a nullable String; make the optionality explicit by changing its type to Optional<String> (or annotate it with `@Nullable` if project prefers annotations). Update the WalletAddress constructor/any factory methods to accept Optional<String> (or a `@Nullable` param and wrap with Optional.ofNullable), adjust the getter to return Optional<String> (or keep the `@Nullable-annotated` getter), and update any usages (e.g., where integratedPaymentId is read or written) to use Optional APIs instead of manual null checks so the presence/absence contract is explicit and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java`:
- Around line 38-57: The WalletAddress constructor currently slices the decoded
hex string without validating its length, allowing short inputs to throw
StringIndexOutOfBoundsException and overlong payloads to be accepted; fix this
in the WalletAddress constructor by validating the decoded payload length
(hex.length()) immediately after hex = ByteUtil.bytesToHex(Base58.decode(...))
and before any substring operations: require that hex.length() equals either 138
(standard/subaddress: 2 + 64 + 64 + 8) or 154 (integrated: 2 + 64 + 64 + 16 +
8), throw InvalidWalletAddressException for any other length, and then proceed
to parse networkByte, publicSpendKeyHex, publicViewKeyHex, optional
integratedPaymentId, and checksum using the existing pos-based slicing so no
StringIndexOutOfBoundsException or silent acceptance of overlong payloads
occurs.
- Around line 97-124: Add explicit bounds checks in
getSubaddressPublicSpendKeyBytes and getSubaddressBase58 to reject negative or
>32-bit values for accountId and subaddressId before calling
longToLittleEndianUint32ByteArray: if accountId < 0 or accountId > 0xFFFFFFFFL
throw an IllegalArgumentException("Invalid accountId: "+accountId) and likewise
for subaddressId; do this at the start of both methods so values are validated
rather than silently masked.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java`:
- Line 14: The public mutable field bytes in class Scalar must be made private
to prevent external mutation; change the field visibility to private byte[]
bytes and add a Lombok `@Getter` on the class (or on the field) but implement the
getter to return a defensive copy (e.g., Arrays.copyOf) instead of exposing the
internal array; also ensure constructors/factory methods (e.g., any
Scalar(byte[] ... ) constructors or clone methods) defensively copy inputs into
the private bytes field so the internal state is never shared.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/MoneroSubaddressService.java`:
- Line 39: The log in MoneroSubaddressService currently prints the full
subAddress which leaks sensitive info; update the logging in the method that
creates new subaddresses (where subAddress is used) to avoid printing the full
address—either omit subAddress entirely or log a masked/truncated form (e.g.,
show only the first N and last M characters or replace middle chars with
ellipsis) and keep the elapsed time as before; locate the log.info call that
references subAddress and replace it with a maskedAddress variable or remove the
address from the message so only non-sensitive details (like timing) are logged.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedFieldElement.java`:
- Around line 22-35: The constructor Ed25519EncodedFieldElement currently
accepts both 32- and 64-byte arrays but the public methods assume a fixed width;
update the class to enforce and track the byte width and validate in each public
operation: store an explicit width flag (e.g., isShort or a byteLength field)
set in the constructor, then in decode(), modQ(), and multiplyAndAddModQ() (and
any other methods that assume a size) check that the instance has the expected
length and throw IllegalArgumentException if not; for multiplyAndAddModQ(),
either require all operands share the same stored width and validate that, or
implement explicit handling for mixed widths (preferably reject mixed widths) so
the upper half is never silently ignored.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519FieldElement.java`:
- Around line 1007-1020: hashCode() currently hashes the raw limb array
(this.values) while equals() compares canonical encodings via encode(), so two
canonically equal field elements can have different hashCodes; change hashCode()
to compute the hash of the canonical encoding used by equals() by calling
this.encode() (or the byte[] it returns) and hashing that result so hashCode()
is consistent with equals()—update the Ed25519FieldElement.hashCode()
implementation to derive its result from encode() rather than this.values.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519GroupElement.java`:
- Around line 650-700: equals() is not symmetric and hashCode() is unstable
because comparisons and hashing depend on the current coordinateSystem and
encode() may throw for PRECOMPUTED/CACHED/AFFINE; fix by normalizing both
operands to a single canonical representation before comparing and by deriving
hashCode from that same canonical representation instead of calling encode()
directly: update equals(Object) to convert both this and ge to a common
coordinate system (e.g., P2/P3 or AFFINE) using toCoordinateSystem/toP2/toP3 so
comparisons use the same X/Y/Z/T, and update hashCode() to compute from that
same canonical form (convert this to the canonical system first and then compute
a stable hash from its fields or from encode() only after conversion), ensuring
any conversion exceptions are handled consistently.
- Around line 796-813: The scalarMultiply method calls select(...) which
dereferences the precomputedForSingle table and can NPE when that table hasn't
been built lazily; fix by ensuring the single-scalar lookup table is initialized
before use—either add a null-check/initializer inside select(…) that builds
precomputedForSingle on demand, or call a helper (e.g.,
ensurePrecomputedForSingle()) at the start of scalarMultiply() before the loops
so select(...) never sees a null precomputedForSingle; reference the existing
symbols scalarMultiply, select, and precomputedForSingle when adding the
lazy-initialization.
In
`@apps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java`:
- Line 139: MoneroFormController currently logs the full subAddress variable in
the info statement (log.info("Created new subAddress {}...")), which exposes
sensitive crypto address data; change this to avoid printing the entire address
by masking or truncating subAddress (e.g., show only a short suffix/prefix or a
hash/ID) and update the log call to include only the masked value or
non-sensitive identifier so the creation timing remains logged without revealing
the full address.
- Line 32: The import in MoneroFormController references the old package
bisq.core.xmr.knaccc.monero.address.WalletAddress which causes compilation
failure; update the import to the new package
bisq.account.accounts.crypto.monero.knaccc.monero.address.WalletAddress (replace
the old import line in MoneroFormController) so the WalletAddress symbol
resolves correctly and the class compiles.
---
Nitpick comments:
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java`:
- Around line 7-14: Make the class-level constants in Base58 immutable by
declaring them final: change alphabet, encoded_block_sizes, full_block_size,
full_encoded_block_size, and UINT64_MAX to be public static final (or private
static final if they don't need external mutation) so they cannot be reassigned
at runtime; for the array encoded_block_sizes also ensure immutability by either
making it private static final and exposing an unmodifiable copy via a getter or
by defensively copying it where used.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java`:
- Around line 24-30: The field integratedPaymentId in WalletAddress is a
nullable String; make the optionality explicit by changing its type to
Optional<String> (or annotate it with `@Nullable` if project prefers annotations).
Update the WalletAddress constructor/any factory methods to accept
Optional<String> (or a `@Nullable` param and wrap with Optional.ofNullable),
adjust the getter to return Optional<String> (or keep the `@Nullable-annotated`
getter), and update any usages (e.g., where integratedPaymentId is read or
written) to use Optional APIs instead of manual null checks so the
presence/absence contract is explicit and consistent.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.java`:
- Around line 55-61: The method ensure32BytesAndConvertToLittleEndian currently
uses if/else without braces; update the method in
CryptoUtil.ensure32BytesAndConvertToLittleEndian to add K&R-style braces around
both the if and the else blocks so both System.arraycopy calls are enclosed in {
... }, leaving logic and the reverseByteArrayInPlace(s) call unchanged to
satisfy coding guidelines.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java`:
- Around line 34-36: The add method in class Scalar is a single long expression;
refactor it for readability by breaking it into named intermediate variables
inside Scalar.add: convert this.bytes and a.bytes to BigInteger using
getBigIntegerFromUnsignedLittleEndianByteArray, compute sum =
(biThis.add(biA)).mod(l), convert sum.toByteArray() to a byte[] variable, then
call ensure32BytesAndConvertToLittleEndian on that byte[] and return new
Scalar(...) — keep the same method calls and semantics but replace the inlined
chain with these clearly named steps.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/MoneroSubaddressService.java`:
- Around line 26-45: MoneroSubaddressService duplicates the validation and
subaddress generation logic already implemented in MoneroFormController; either
remove this unused class or refactor the controller to use it: inject
MoneroSubaddressService into MoneroFormController (add a constructor parameter
or `@Autowired` field) and replace the controller’s inline logic (the block that
validates mainAddress/privateViewKey/accountIndex/subAddressIndex and calls
WalletAddress.getSubaddressBase58) with a call to
MoneroSubaddressService.generateSubAddress(mainAddress, privateViewKey,
accountIndex, subAddressIndex); if you choose removal, delete
MoneroSubaddressService and any imports/usages and ensure MoneroFormController
remains the single canonical implementation and that existing unit/integration
tests are updated accordingly.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ArrayUtils.java`:
- Around line 73-89: The toByteArray method may throw
ArrayIndexOutOfBoundsException when BigInteger.toByteArray() returns an empty
array; update ArrayUtils.toByteArray to defensively handle an empty
bigIntegerBytes before accessing bigIntegerBytes[0] by checking
bigIntegerBytes.length == 0 and treating it as a single zero byte (or setting
copyStartIndex/numBytesToCopy accordingly), then proceed with the existing logic
that computes copyStartIndex, numBytesToCopy and fills outputBytes so
outputBytes always gets populated safely even for empty or leading-zero
BigInteger byte arrays.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 015c1e79-6107-4e98-85bb-23bc0c38e736
📒 Files selected for processing (20)
account/src/main/java/bisq/account/accounts/crypto/monero/MoneroSubaddressService.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/com/joemelsha/crypto/hash/Keccak.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/Curve.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/Ed25519Curve.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/CoordinateSystem.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedFieldElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedGroupElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Field.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519FieldElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Group.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519GroupElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ArrayUtils.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/HexEncoder.javaapps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java
✅ Files skipped from review due to trivial changes (2)
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/CoordinateSystem.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/Curve.java
🚧 Files skipped from review as they are similar to previous changes (6)
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/HexEncoder.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/Ed25519Curve.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Group.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Field.java
- account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.java
Comment on lines
+38
to
+57
| public WalletAddress(String base58WalletAddress) throws InvalidWalletAddressException { | ||
|
|
||
| base58 = base58WalletAddress; | ||
| int pos = 0; | ||
| hex = ByteUtil.bytesToHex(Base58.decode(base58WalletAddress)); | ||
| networkByte = hex.substring(0, pos += 2); | ||
| if (!( | ||
| networkByte.equals(ByteUtil.byteToHex(PUBLIC_ADDRESS_PREFIX)) || | ||
| networkByte.equals(ByteUtil.byteToHex(PUBLIC_INTEGRATED_ADDRESS_PREFIX)) || | ||
| networkByte.equals(ByteUtil.byteToHex(PUBLIC_SUBADDRESS_PREFIX)) | ||
| )) throw new InvalidWalletAddressException("Unrecognized address type: " + networkByte + " (hex)"); | ||
| publicSpendKeyHex = hex.substring(pos, pos += 64); | ||
| publicViewKeyHex = hex.substring(pos, pos += 64); | ||
| if (networkByte.equals(ByteUtil.byteToHex(PUBLIC_INTEGRATED_ADDRESS_PREFIX))) | ||
| integratedPaymentId = hex.substring(pos, pos += 16); | ||
| checksum = hex.substring(pos, pos += 8); | ||
|
|
||
| String recalculatedChecksumHex = ByteUtil.bytesToHex(CryptoUtil.fastHash(hexToBytes(networkByte + publicSpendKeyHex + publicViewKeyHex + (integratedPaymentId == null ? "" : integratedPaymentId)))).substring(0, 8); | ||
| if (!checksum.equals(recalculatedChecksumHex)) | ||
| throw new InvalidWalletAddressException("Checksum does not match"); |
There was a problem hiding this comment.
Reject malformed or overlong decoded payloads before parsing.
This constructor assumes the Base58 decode produced exactly a standard/subaddress or integrated payload and starts slicing immediately. Short inputs can escape as StringIndexOutOfBoundsException, and longer inputs with a valid embedded checksum are accepted because pos is never checked against hex.length().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java`
around lines 38 - 57, The WalletAddress constructor currently slices the decoded
hex string without validating its length, allowing short inputs to throw
StringIndexOutOfBoundsException and overlong payloads to be accepted; fix this
in the WalletAddress constructor by validating the decoded payload length
(hex.length()) immediately after hex = ByteUtil.bytesToHex(Base58.decode(...))
and before any substring operations: require that hex.length() equals either 138
(standard/subaddress: 2 + 64 + 64 + 8) or 154 (integrated: 2 + 64 + 64 + 16 +
8), throw InvalidWalletAddressException for any other length, and then proceed
to parse networkByte, publicSpendKeyHex, publicViewKeyHex, optional
integratedPaymentId, and checksum using the existing pos-based slicing so no
StringIndexOutOfBoundsException or silent acceptance of overlong payloads
occurs.
Comment on lines
+97
to
+124
| public static byte[] getSubaddressPublicSpendKeyBytes(Scalar privateViewKey, | ||
| byte[] publicSpendKeyBytes, | ||
| long accountId, | ||
| long subaddressId) { | ||
| if (accountId == 0 && subaddressId == 0) | ||
| throw new RuntimeException("Not to be called for the base wallet address"); | ||
| byte[] data = concat("SubAddr\0".getBytes(), privateViewKey.bytes, longToLittleEndianUint32ByteArray(accountId), longToLittleEndianUint32ByteArray(subaddressId)); | ||
| Scalar m = CryptoUtil.hashToScalar(data); | ||
| Ed25519GroupElement M = G.scalarMultiply(new Ed25519EncodedFieldElement(m.bytes)); | ||
| Ed25519GroupElement B = new Ed25519EncodedGroupElement(publicSpendKeyBytes).decode(); | ||
| Ed25519GroupElement D = B.add(M.toCached()); | ||
| return D.encode().getRaw(); | ||
| } | ||
|
|
||
| public static String getSubaddressBase58(Scalar privateViewKey, | ||
| byte[] publicSpendKeyBytes, | ||
| long accountId, | ||
| long subaddressId) { | ||
|
|
||
| Ed25519GroupElement D = new Ed25519EncodedGroupElement(getSubaddressPublicSpendKeyBytes(privateViewKey, publicSpendKeyBytes, accountId, subaddressId)).decode(); | ||
| D.precomputeForScalarMultiplication(); | ||
| Ed25519GroupElement C = D.scalarMultiply(new Ed25519EncodedFieldElement(privateViewKey.bytes)); | ||
|
|
||
| byte[] subaddressBytes = concat((byte) PUBLIC_SUBADDRESS_PREFIX, concat(D.encode().getRaw(), C.encode().getRaw())); | ||
| String hex = ByteUtil.bytesToHex(subaddressBytes); | ||
| String calculatedChecksumHex = ByteUtil.bytesToHex(CryptoUtil.fastHash(hexToBytes(hex))).substring(0, 8); | ||
| hex += calculatedChecksumHex; | ||
| return Base58.encode(hexToBytes(hex)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'longToLittleEndianUint32ByteArray'Repository: bisq-network/bisq2
Length of output: 3395
🏁 Script executed:
cat -n account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.javaRepository: bisq-network/bisq2
Length of output: 3020
🏁 Script executed:
cat -n account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java | head -130Repository: bisq-network/bisq2
Length of output: 6934
Add bounds checks for subaddress indices before truncation to uint32.
The longToLittleEndianUint32ByteArray() method masks out-of-range values instead of rejecting them. Negative or >32-bit values for accountId and subaddressId will silently derive a different subaddress than the caller intended. Both getSubaddressPublicSpendKeyBytes() and getSubaddressBase58() need explicit validation:
if (accountId < 0 || accountId > 0xFFFF_FFFFL)
throw new IllegalArgumentException("Invalid accountId: " + accountId);
if (subaddressId < 0 || subaddressId > 0xFFFF_FFFFL)
throw new IllegalArgumentException("Invalid subaddressId: " + subaddressId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java`
around lines 97 - 124, Add explicit bounds checks in
getSubaddressPublicSpendKeyBytes and getSubaddressBase58 to reject negative or
>32-bit values for accountId and subaddressId before calling
longToLittleEndianUint32ByteArray: if accountId < 0 or accountId > 0xFFFFFFFFL
throw an IllegalArgumentException("Invalid accountId: "+accountId) and likewise
for subaddressId; do this at the start of both methods so values are validated
rather than silently masked.
|
|
||
| public class Scalar { | ||
|
|
||
| public byte[] bytes; |
There was a problem hiding this comment.
Encapsulation violation: mutable public field in cryptographic class.
The bytes field is public, allowing external code to mutate the internal state of a Scalar instance. For a cryptographic primitive, this breaks immutability guarantees and could lead to subtle security bugs.
🔒 Proposed fix
- public byte[] bytes;
+ private final byte[] bytes;
+
+ public byte[] getBytes() {
+ return bytes.clone();
+ }As per coding guidelines, use Lombok for generating getters. Consider @Getter with defensive copying for byte arrays.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java`
at line 14, The public mutable field bytes in class Scalar must be made private
to prevent external mutation; change the field visibility to private byte[]
bytes and add a Lombok `@Getter` on the class (or on the field) but implement the
getter to return a defensive copy (e.g., Arrays.copyOf) instead of exposing the
internal array; also ensure constructors/factory methods (e.g., any
Scalar(byte[] ... ) constructors or clone methods) defensively copy inputs into
the private bytes field so the internal state is never shared.
| WalletAddress walletAddress = new WalletAddress(mainAddress); | ||
| long start = System.currentTimeMillis(); | ||
| String subAddress = walletAddress.getSubaddressBase58(privateViewKey, accountIndex, subAddressIndex); | ||
| log.info("Created new subAddress {}. Took {} ms.", subAddress, System.currentTimeMillis() - start); |
There was a problem hiding this comment.
Avoid logging full cryptocurrency addresses.
Logging the complete subAddress exposes sensitive financial information. Consider logging only a truncated version or omitting it entirely.
🔒 Proposed fix
- log.info("Created new subAddress {}. Took {} ms.", subAddress, System.currentTimeMillis() - start);
+ log.info("Created new subAddress (prefix: {}...). Took {} ms.",
+ subAddress.substring(0, Math.min(8, subAddress.length())),
+ System.currentTimeMillis() - start);📝 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
| log.info("Created new subAddress {}. Took {} ms.", subAddress, System.currentTimeMillis() - start); | |
| log.info("Created new subAddress (prefix: {}...). Took {} ms.", | |
| subAddress.substring(0, Math.min(8, subAddress.length())), | |
| System.currentTimeMillis() - start); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/MoneroSubaddressService.java`
at line 39, The log in MoneroSubaddressService currently prints the full
subAddress which leaks sensitive info; update the logging in the method that
creates new subaddresses (where subAddress is used) to avoid printing the full
address—either omit subAddress entirely or log a masked/truncated form (e.g.,
show only the first N and last M characters or replace middle chars with
ellipsis) and keep the elapsed time as before; locate the log.info call that
references subAddress and replace it with a maskedAddress variable or remove the
address from the message so only non-sensitive details (like timing) are logged.
Comment on lines
+22
to
+35
| public Ed25519EncodedFieldElement(final byte[] values) { | ||
| switch (values.length) { | ||
| case 32: | ||
| this.zero = Ed25519Field.ZERO_SHORT; | ||
| break; | ||
| case 64: | ||
| this.zero = Ed25519Field.ZERO_LONG; | ||
| break; | ||
| default: | ||
| throw new IllegalArgumentException("Invalid 2^8 bit representation."); | ||
| } | ||
|
|
||
| this.values = values; | ||
| } |
There was a problem hiding this comment.
Enforce the expected byte width per operation.
This wrapper allows both 32- and 64-byte payloads, but the public methods do not. decode() only makes sense for 32 bytes, modQ() assumes 64 bytes and will blow up on a 32-byte instance, and multiplyAndAddModQ() silently ignores the upper half if any operand was created from 64 bytes.
Suggested fix
public Ed25519FieldElement decode() {
+ if (this.values.length != 32) {
+ throw new IllegalStateException("decode() requires 32 bytes");
+ }
long h0 = fourBytesToLong(this.values, 0);
long h1 = threeBytesToLong(this.values, 4) << 6;
...
}
public Ed25519EncodedFieldElement modQ() {
+ if (this.values.length != 64) {
+ throw new IllegalStateException("modQ() requires 64 bytes");
+ }
// s0, ..., s22 have 21 bits, s23 has 29 bits
...
}
public Ed25519EncodedFieldElement multiplyAndAddModQ(final Ed25519EncodedFieldElement b,
final Ed25519EncodedFieldElement c) {
+ if (this.values.length != 32 || b.values.length != 32 || c.values.length != 32) {
+ throw new IllegalArgumentException("multiplyAndAddModQ() requires 32-byte operands");
+ }
final long a0 = 0x1FFFFF & threeBytesToLong(this.values, 0);
...
}Also applies to: 74-75, 149-150, 510-511
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedFieldElement.java`
around lines 22 - 35, The constructor Ed25519EncodedFieldElement currently
accepts both 32- and 64-byte arrays but the public methods assume a fixed width;
update the class to enforce and track the byte width and validate in each public
operation: store an explicit width flag (e.g., isShort or a byteLength field)
set in the constructor, then in decode(), modQ(), and multiplyAndAddModQ() (and
any other methods that assume a size) check that the instance has the expected
length and throw IllegalArgumentException if not; for multiplyAndAddModQ(),
either require all operands share the same stored width and validate that, or
implement explicit handling for mixed widths (preferably reject mixed widths) so
the upper half is never silently ignored.
Comment on lines
+1007
to
+1020
| @Override | ||
| public int hashCode() { | ||
| return Arrays.hashCode(this.values); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(final Object obj) { | ||
| if (!(obj instanceof Ed25519FieldElement)) { | ||
| return false; | ||
| } | ||
|
|
||
| final Ed25519FieldElement f = (Ed25519FieldElement) obj; | ||
| return this.encode().equals(f.encode()); | ||
| } |
There was a problem hiding this comment.
Hash the canonical field encoding, not the raw limbs.
equals() canonicalizes through encode(), but hashCode() uses the unreduced limb array. Two elements that represent the same value modulo p but differ only by carries/reduction will compare equal and still end up in different hash buckets.
Suggested fix
`@Override`
public int hashCode() {
- return Arrays.hashCode(this.values);
+ return this.encode().hashCode();
}📝 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
| @Override | |
| public int hashCode() { | |
| return Arrays.hashCode(this.values); | |
| } | |
| @Override | |
| public boolean equals(final Object obj) { | |
| if (!(obj instanceof Ed25519FieldElement)) { | |
| return false; | |
| } | |
| final Ed25519FieldElement f = (Ed25519FieldElement) obj; | |
| return this.encode().equals(f.encode()); | |
| } | |
| `@Override` | |
| public int hashCode() { | |
| return this.encode().hashCode(); | |
| } | |
| `@Override` | |
| public boolean equals(final Object obj) { | |
| if (!(obj instanceof Ed25519FieldElement)) { | |
| return false; | |
| } | |
| final Ed25519FieldElement f = (Ed25519FieldElement) obj; | |
| return this.encode().equals(f.encode()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519FieldElement.java`
around lines 1007 - 1020, hashCode() currently hashes the raw limb array
(this.values) while equals() compares canonical encodings via encode(), so two
canonically equal field elements can have different hashCodes; change hashCode()
to compute the hash of the canonical encoding used by equals() by calling
this.encode() (or the byte[] it returns) and hashing that result so hashCode()
is consistent with equals()—update the Ed25519FieldElement.hashCode()
implementation to derive its result from encode() rather than this.values.
Comment on lines
+650
to
+700
| @Override | ||
| public int hashCode() { | ||
| return this.encode().hashCode(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(final Object obj) { | ||
| if (!(obj instanceof Ed25519GroupElement)) { | ||
| return false; | ||
| } | ||
| Ed25519GroupElement ge = (Ed25519GroupElement) obj; | ||
| if (!this.coordinateSystem.equals(ge.coordinateSystem)) { | ||
| try { | ||
| ge = ge.toCoordinateSystem(this.coordinateSystem); | ||
| } catch (final Exception e) { | ||
| return false; | ||
| } | ||
| } | ||
| switch (this.coordinateSystem) { | ||
| case P2: | ||
| case P3: | ||
| if (this.Z.equals(ge.Z)) { | ||
| return this.X.equals(ge.X) && this.Y.equals(ge.Y); | ||
| } | ||
|
|
||
| final Ed25519FieldElement x1 = this.X.multiply(ge.Z); | ||
| final Ed25519FieldElement y1 = this.Y.multiply(ge.Z); | ||
| final Ed25519FieldElement x2 = ge.X.multiply(this.Z); | ||
| final Ed25519FieldElement y2 = ge.Y.multiply(this.Z); | ||
|
|
||
| return x1.equals(x2) && y1.equals(y2); | ||
| case P1xP1: | ||
| return this.toP2().equals(ge); | ||
| case PRECOMPUTED: | ||
| return this.X.equals(ge.X) && this.Y.equals(ge.Y) && this.Z.equals(ge.Z); | ||
| case CACHED: | ||
| if (this.Z.equals(ge.Z)) { | ||
| return this.X.equals(ge.X) && this.Y.equals(ge.Y) && this.T.equals(ge.T); | ||
| } | ||
|
|
||
| final Ed25519FieldElement x3 = this.X.multiply(ge.Z); | ||
| final Ed25519FieldElement y3 = this.Y.multiply(ge.Z); | ||
| final Ed25519FieldElement t3 = this.T.multiply(ge.Z); | ||
| final Ed25519FieldElement x4 = ge.X.multiply(this.Z); | ||
| final Ed25519FieldElement y4 = ge.Y.multiply(this.Z); | ||
| final Ed25519FieldElement t4 = ge.T.multiply(this.Z); | ||
|
|
||
| return x3.equals(x4) && y3.equals(y4) && t3.equals(t4); | ||
| default: | ||
| return false; | ||
| } |
There was a problem hiding this comment.
equals() and hashCode() are representation-dependent.
equals() only tries to convert ge into this.coordinateSystem, so mixed comparisons like P1xP1 vs P2/P3 can be false in one direction and true in the other. hashCode() always goes through encode(), which throws for PRECOMPUTED, CACHED, and AFFINE, so valid instances do not have a stable object contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519GroupElement.java`
around lines 650 - 700, equals() is not symmetric and hashCode() is unstable
because comparisons and hashing depend on the current coordinateSystem and
encode() may throw for PRECOMPUTED/CACHED/AFFINE; fix by normalizing both
operands to a single canonical representation before comparing and by deriving
hashCode from that same canonical representation instead of calling encode()
directly: update equals(Object) to convert both this and ge to a common
coordinate system (e.g., P2/P3 or AFFINE) using toCoordinateSystem/toP2/toP3 so
comparisons use the same X/Y/Z/T, and update hashCode() to compute from that
same canonical form (convert this to the canonical system first and then compute
a stable hash from its fields or from encode() only after conversion), ensuring
any conversion exceptions are handled consistently.
Comment on lines
+796
to
+813
| public Ed25519GroupElement scalarMultiply(final Ed25519EncodedFieldElement a) { | ||
| Ed25519GroupElement g; | ||
| int i; | ||
| final byte[] e = toRadix16(a); | ||
| Ed25519GroupElement h = Ed25519Group.ZERO_P3; | ||
| for (i = 1; i < 64; i += 2) { | ||
| g = this.select(i / 2, e[i]); | ||
| h = h.precomputedAdd(g).toP3(); | ||
| } | ||
|
|
||
| h = h.dbl().toP2().dbl().toP2().dbl().toP2().dbl().toP3(); | ||
|
|
||
| for (i = 0; i < 64; i += 2) { | ||
| g = this.select(i / 2, e[i]); | ||
| h = h.precomputedAdd(g).toP3(); | ||
| } | ||
|
|
||
| return h; |
There was a problem hiding this comment.
Initialize the single-scalar lookup table on demand.
scalarMultiply() calls select(...) immediately, but select(...) dereferences precomputedForSingle. On any point that was not precomputed upfront, this fails with an NPE even though the Javadoc says the table is built lazily.
Suggested fix
public Ed25519GroupElement scalarMultiply(final Ed25519EncodedFieldElement a) {
+ if (this.precomputedForSingle == null) {
+ this.precomputeForScalarMultiplication();
+ }
Ed25519GroupElement g;
int i;
final byte[] e = toRadix16(a);📝 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
| public Ed25519GroupElement scalarMultiply(final Ed25519EncodedFieldElement a) { | |
| Ed25519GroupElement g; | |
| int i; | |
| final byte[] e = toRadix16(a); | |
| Ed25519GroupElement h = Ed25519Group.ZERO_P3; | |
| for (i = 1; i < 64; i += 2) { | |
| g = this.select(i / 2, e[i]); | |
| h = h.precomputedAdd(g).toP3(); | |
| } | |
| h = h.dbl().toP2().dbl().toP2().dbl().toP2().dbl().toP3(); | |
| for (i = 0; i < 64; i += 2) { | |
| g = this.select(i / 2, e[i]); | |
| h = h.precomputedAdd(g).toP3(); | |
| } | |
| return h; | |
| public Ed25519GroupElement scalarMultiply(final Ed25519EncodedFieldElement a) { | |
| if (this.precomputedForSingle == null) { | |
| this.precomputeForScalarMultiplication(); | |
| } | |
| Ed25519GroupElement g; | |
| int i; | |
| final byte[] e = toRadix16(a); | |
| Ed25519GroupElement h = Ed25519Group.ZERO_P3; | |
| for (i = 1; i < 64; i += 2) { | |
| g = this.select(i / 2, e[i]); | |
| h = h.precomputedAdd(g).toP3(); | |
| } | |
| h = h.dbl().toP2().dbl().toP2().dbl().toP2().dbl().toP3(); | |
| for (i = 0; i < 64; i += 2) { | |
| g = this.select(i / 2, e[i]); | |
| h = h.precomputedAdd(g).toP3(); | |
| } | |
| return h; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519GroupElement.java`
around lines 796 - 813, The scalarMultiply method calls select(...) which
dereferences the precomputedForSingle table and can NPE when that table hasn't
been built lazily; fix by ensuring the single-scalar lookup table is initialized
before use—either add a null-check/initializer inside select(…) that builds
precomputedForSingle on demand, or call a helper (e.g.,
ensurePrecomputedForSingle()) at the start of scalarMultiply() before the loops
so select(...) never sees a null precomputedForSingle; reference the existing
symbols scalarMultiply, select, and precomputedForSingle when adding the
lazy-initialization.
...esktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java
Outdated
Show resolved
Hide resolved
| WalletAddress walletAddress = new WalletAddress(mainAddress); | ||
| long start = System.currentTimeMillis(); | ||
| String subAddress = walletAddress.getSubaddressBase58(privateViewKey, accountIndexValue, subAddressIndexValue); | ||
| log.info("Created new subAddress {}. Took {} ms.", subAddress, System.currentTimeMillis() - start); |
There was a problem hiding this comment.
Avoid logging full cryptocurrency addresses.
Same concern as in MoneroSubaddressService - logging the complete subAddress exposes sensitive financial information.
🔒 Proposed fix
- log.info("Created new subAddress {}. Took {} ms.", subAddress, System.currentTimeMillis() - start);
+ log.info("Created new Monero subAddress. Took {} ms.", System.currentTimeMillis() - start);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java`
at line 139, MoneroFormController currently logs the full subAddress variable in
the info statement (log.info("Created new subAddress {}...")), which exposes
sensitive crypto address data; change this to avoid printing the entire address
by masking or truncating subAddress (e.g., show only a short suffix/prefix or a
hash/ID) and update the log call to include only the masked value or
non-sensitive identifier so the creation timing remains logged without revealing
the full address.
de9f36a to
e8143b0
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (3)
account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java (2)
29-32:⚠️ Potential issue | 🟠 MajorFix
equalssafety and add matchinghashCode(or use Lombok).At Line 29-32,
equalscan throw onnull/wrong type and violates the equals/hashCode contract becausehashCodeis missing.🐛 Proposed fix
`@Override` public boolean equals(Object obj) { - return Arrays.equals(this.bytes, ((Scalar) obj).bytes); + if (this == obj) { + return true; + } + if (!(obj instanceof Scalar other)) { + return false; + } + return Arrays.equals(this.bytes, other.bytes); } + +@Override +public int hashCode() { + return Arrays.hashCode(bytes); +}As per coding guidelines,
**/*.java: “Use Lombok for generating getters, setters, toString, equals, and hashCode”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java` around lines 29 - 32, The equals method in class Scalar is unsafe and there is no matching hashCode; update Scalar.equals to first check for reference equality, null, and that obj is an instance of Scalar before comparing bytes (e.g., if (this == obj) return true; if (!(obj instanceof Scalar)) return false;), and add a corresponding hashCode method that returns Arrays.hashCode(this.bytes) (or annotate the class with Lombok `@EqualsAndHashCode/`@Data to generate both methods) so equals/hashCode contract is preserved and null/wrong-type inputs are handled safely.
14-22:⚠️ Potential issue | 🟠 MajorMake
bytesprivate/final and defensively copy constructor input.At Line 14-22, the scalar state is externally mutable (
public byte[]) and aliased (this.bytes = bytes). For a crypto primitive, this is risky and can corrupt state across call sites.🔒 Proposed fix
public class Scalar { - public byte[] bytes; + private final byte[] bytes; public Scalar(byte[] bytes) { - this.bytes = bytes; + this.bytes = Arrays.copyOf(bytes, bytes.length); } public Scalar(String hex) { this.bytes = ByteUtil.hexToBytes(hex); } + + public byte[] getBytes() { + return Arrays.copyOf(bytes, bytes.length); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java` around lines 14 - 22, Make the Scalar class field bytes private and final, and defensively copy constructor inputs to avoid external mutation: change the public byte[] bytes to private final byte[] bytes, in the Scalar(byte[] bytes) constructor copy the incoming array (e.g., Arrays.copyOf) into the field instead of assigning, and in Scalar(String hex) keep using ByteUtil.hexToBytes(hex) but assign a copy to the field; also provide any needed accessors (e.g., a getBytes() that returns a defensive copy) so callers cannot mutate the internal byte array.account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.java (1)
25-33:⚠️ Potential issue | 🟠 Major
hexToBytesneeds strict input validation (null/odd-length/invalid hex).At Line 25-33, invalid input can currently fail unclearly or decode corrupted bytes. This should fail fast with explicit
IllegalArgumentException.🐛 Proposed fix
public static byte[] hexToBytes(String s) { + if (s == null) { + throw new IllegalArgumentException("Hex string must not be null"); + } int len = s.length(); + if ((len & 1) != 0) { + throw new IllegalArgumentException("Hex string must have even length"); + } byte[] data = new byte[len / 2]; for (int i = 0; i < len; i += 2) { - data[i / 2] = (byte) ((Character.digit(s.charAt(i), 16) << 4) - + Character.digit(s.charAt(i + 1), 16)); + int high = Character.digit(s.charAt(i), 16); + int low = Character.digit(s.charAt(i + 1), 16); + if (high < 0 || low < 0) { + throw new IllegalArgumentException("Invalid hex character at index " + i); + } + data[i / 2] = (byte) ((high << 4) | low); } return data; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.java` around lines 25 - 33, Validate input at the start of ByteUtil.hexToBytes: throw IllegalArgumentException if the input String is null or has odd length, and when converting each hex char check Character.digit(...) returns >= 0 for both nybbles and throw IllegalArgumentException with a clear message if any char is invalid; keep the existing conversion loop in hexToBytes but replace/augment silent failures by explicit checks and meaningful exception messages to fail fast on bad input.
🧹 Nitpick comments (5)
account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java (4)
71-72: Add braces toforandifstatements.♻️ Proposed refactor
- for (int i = 0; i < encoded_block_sizes.length; i++) - if (encoded_block_sizes[i] == last_block_size) last_block_decoded_size = i; + for (int i = 0; i < encoded_block_sizes.length; i++) { + if (encoded_block_sizes[i] == last_block_size) { + last_block_decoded_size = i; + } + }As per coding guidelines: "Follow K&R brace style and always use braces".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java` around lines 71 - 72, The loop in Base58.java that sets last_block_decoded_size uses a single-line for and if without braces; update the for loop and the nested if (which reference encoded_block_sizes, last_block_size, and last_block_decoded_size) to use K&R-style braces so both the for and the if have explicit { ... } blocks (open brace after the for(...) and after the if(...), close braces after the statements) to satisfy the coding guideline requiring braces.
58-64: Add braces to conditional statements.Lines 61-62 use single-line conditionals without braces.
♻️ Proposed refactor
private static byte[] uint64_to_8be(BigInteger n, int size) { byte[] r = new byte[size]; byte[] a = n.toByteArray(); - if (a.length <= r.length) System.arraycopy(a, 0, r, r.length - a.length, a.length); - else System.arraycopy(a, a.length - r.length, r, 0, r.length); + if (a.length <= r.length) { + System.arraycopy(a, 0, r, r.length - a.length, a.length); + } else { + System.arraycopy(a, a.length - r.length, r, 0, r.length); + } return r; }As per coding guidelines: "Follow K&R brace style and always use braces".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java` around lines 58 - 64, The uint64_to_8be method in class Base58 uses single-line if/else without braces; update the method (uint64_to_8be) to use K&R-style braces for both the if (a.length <= r.length) and the else branches so each System.arraycopy call is enclosed in braces, preserving the existing logic and behavior.
16-20: Add braces toforandifstatements.Per coding guidelines, always use braces. The single-line
for/ifcombination at line 18 violates this.♻️ Proposed refactor
public static int lookupIndex(int[] array, int match) { int index = -1; - for (int i = 0; i < array.length; i++) if (array[i] == match) index = i; + for (int i = 0; i < array.length; i++) { + if (array[i] == match) { + index = i; + } + } return index; }As per coding guidelines: "Follow K&R brace style and always use braces".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java` around lines 16 - 20, The lookupIndex method uses single-line for/if without braces; update the method (lookupIndex(int[] array, int match)) to use K&R-style braces by expanding the for loop and the inner if into block form (for (...) { if (...) { index = i; } }) to satisfy the coding guideline of always using braces around control blocks while keeping existing behavior and variable names (index, array, match).
7-14: Make constantsprivate static final.These fields are effectively constants but are declared as mutable public fields. This exposes implementation details and allows accidental modification.
♻️ Proposed refactor
- public static String alphabet = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz"; + private static final String ALPHABET = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz"; - public static int[] encoded_block_sizes = new int[]{0, 2, 3, 5, 6, 7, 9, 10, 11}; + private static final int[] ENCODED_BLOCK_SIZES = new int[]{0, 2, 3, 5, 6, 7, 9, 10, 11}; - public static int full_block_size = 8; - public static int full_encoded_block_size = 11; + private static final int FULL_BLOCK_SIZE = 8; + private static final int FULL_ENCODED_BLOCK_SIZE = 11; - public static BigInteger UINT64_MAX = BigInteger.valueOf(2).pow(64); + private static final BigInteger UINT64_MAX = BigInteger.valueOf(2).pow(64);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java` around lines 7 - 14, The public mutable fields in Base58 (alphabet, encoded_block_sizes, full_block_size, full_encoded_block_size, UINT64_MAX) should be converted to constants: change their declarations to private static final (and for encoded_block_sizes, make the array private static final and consider returning a copy or an unmodifiable view via a getter if external access is required); update usages to access them within the class or add explicit public getters if they must remain accessible. Ensure the BigInteger UINT64_MAX remains initialized as a final constant and that any code relying on mutability is updated to use the new getters or internal constants.account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedFieldElement.java (1)
42-44: Consider defensive copy ongetRaw().Returning the internal array allows callers to mutate the field element's state, which could break
equals()/hashCode()invariants and corrupt decoded values.♻️ Proposed fix
public byte[] getRaw() { - return this.values; + return this.values.clone(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedFieldElement.java` around lines 42 - 44, The getRaw() method in Ed25519EncodedFieldElement exposes the internal byte[] values allowing external mutation which can break equals()/hashCode() and decoded state; change getRaw() to return a defensive copy (e.g., Arrays.copyOf(this.values, this.values.length)) instead of this.values, and ensure the class continues to use Arrays.equals/Arrays.hashCode (or other immutable-aware comparisons) for equals() and hashCode() so callers cannot mutate internal state via the returned array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/com/joemelsha/crypto/hash/Keccak.java`:
- Around line 239-356: Add unit tests that exercise the Keccak implementation
paths used in subaddress derivation: specifically call pad(), the various digest
overloads (digest(), digest(int), digest(byte[]), digest(ByteBuffer)) and
keccak() to produce outputs, and assert against canonical Keccak-256
known-answer vectors plus at least one Bisq 1 parity vector; place tests to
cover both the byte-wise and bit/partial-word branches (the code paths guarded
by padded flag and the rateBits/rateBytes handling) so any porting drift in
these methods is detected. Ensure tests validate outputs from digestArray(int),
digest(ByteBuffer) with direct and non-direct buffers, and include vectors that
force the rateWords/rateBytes intermediate branches (e.g., outputs larger than
one rate block and outputs with trailing partial bytes). Run these tests as part
of CI so pad(), digest*, and keccak() regressions are caught before release.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.java`:
- Line 13: Change the lookup table field in ByteUtil: make the field private,
keep it static final, and rename hexArray to follow constant conventions
(HEX_ARRAY); update any internal references in the ByteUtil class (e.g., methods
that currently reference hexArray) to use HEX_ARRAY so the field is no longer
exposed as protected.
- Around line 61-64: The method longToLittleEndianUint32ByteArray silently
truncates out-of-range values and uses a brace-less for loop; update
longToLittleEndianUint32ByteArray to validate the input is within the uint32
range (0 <= value <= 0xFFFFFFFF) and throw an IllegalArgumentException for
out-of-range values, and add explicit braces around the for loop body that
populates the byte[] array (array variable) to satisfy the K&R style rule.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Group.java`:
- Around line 12-15: The javadoc for GROUP_ORDER in Ed25519Group incorrectly
states the formula as "2^252 - 27742317777372353535851937790883648493" while the
constant GROUP_ORDER (BigInteger.ONE.shiftLeft(252).add(...)) uses addition;
update the comment to the correct expression "2^252 +
27742317777372353535851937790883648493" so the comment matches the
implementation and references the GROUP_ORDER constant.
In
`@apps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java`:
- Around line 130-147: The Monero subaddress creation can throw unchecked
RuntimeException from Ed25519/Base58 operations which are not currently caught
in MoneroFormController; update the try/catch around
WalletAddress.getSubaddressBase58 to also catch RuntimeException (in addition to
IllegalArgumentException and WalletAddress.InvalidWalletAddressException), log a
clear error via log.error including the exception and a descriptive message, and
return an empty string on failure so the reactive subscription does not crash;
refer to the MoneroFormController method that calls
WalletAddress.getSubaddressBase58 to locate the change.
---
Duplicate comments:
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.java`:
- Around line 25-33: Validate input at the start of ByteUtil.hexToBytes: throw
IllegalArgumentException if the input String is null or has odd length, and when
converting each hex char check Character.digit(...) returns >= 0 for both
nybbles and throw IllegalArgumentException with a clear message if any char is
invalid; keep the existing conversion loop in hexToBytes but replace/augment
silent failures by explicit checks and meaningful exception messages to fail
fast on bad input.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java`:
- Around line 29-32: The equals method in class Scalar is unsafe and there is no
matching hashCode; update Scalar.equals to first check for reference equality,
null, and that obj is an instance of Scalar before comparing bytes (e.g., if
(this == obj) return true; if (!(obj instanceof Scalar)) return false;), and add
a corresponding hashCode method that returns Arrays.hashCode(this.bytes) (or
annotate the class with Lombok `@EqualsAndHashCode/`@Data to generate both
methods) so equals/hashCode contract is preserved and null/wrong-type inputs are
handled safely.
- Around line 14-22: Make the Scalar class field bytes private and final, and
defensively copy constructor inputs to avoid external mutation: change the
public byte[] bytes to private final byte[] bytes, in the Scalar(byte[] bytes)
constructor copy the incoming array (e.g., Arrays.copyOf) into the field instead
of assigning, and in Scalar(String hex) keep using ByteUtil.hexToBytes(hex) but
assign a copy to the field; also provide any needed accessors (e.g., a
getBytes() that returns a defensive copy) so callers cannot mutate the internal
byte array.
---
Nitpick comments:
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java`:
- Around line 71-72: The loop in Base58.java that sets last_block_decoded_size
uses a single-line for and if without braces; update the for loop and the nested
if (which reference encoded_block_sizes, last_block_size, and
last_block_decoded_size) to use K&R-style braces so both the for and the if have
explicit { ... } blocks (open brace after the for(...) and after the if(...),
close braces after the statements) to satisfy the coding guideline requiring
braces.
- Around line 58-64: The uint64_to_8be method in class Base58 uses single-line
if/else without braces; update the method (uint64_to_8be) to use K&R-style
braces for both the if (a.length <= r.length) and the else branches so each
System.arraycopy call is enclosed in braces, preserving the existing logic and
behavior.
- Around line 16-20: The lookupIndex method uses single-line for/if without
braces; update the method (lookupIndex(int[] array, int match)) to use K&R-style
braces by expanding the for loop and the inner if into block form (for (...) {
if (...) { index = i; } }) to satisfy the coding guideline of always using
braces around control blocks while keeping existing behavior and variable names
(index, array, match).
- Around line 7-14: The public mutable fields in Base58 (alphabet,
encoded_block_sizes, full_block_size, full_encoded_block_size, UINT64_MAX)
should be converted to constants: change their declarations to private static
final (and for encoded_block_sizes, make the array private static final and
consider returning a copy or an unmodifiable view via a getter if external
access is required); update usages to access them within the class or add
explicit public getters if they must remain accessible. Ensure the BigInteger
UINT64_MAX remains initialized as a final constant and that any code relying on
mutability is updated to use the new getters or internal constants.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedFieldElement.java`:
- Around line 42-44: The getRaw() method in Ed25519EncodedFieldElement exposes
the internal byte[] values allowing external mutation which can break
equals()/hashCode() and decoded state; change getRaw() to return a defensive
copy (e.g., Arrays.copyOf(this.values, this.values.length)) instead of
this.values, and ensure the class continues to use Arrays.equals/Arrays.hashCode
(or other immutable-aware comparisons) for equals() and hashCode() so callers
cannot mutate internal state via the returned array.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e1da8e81-f1a0-48e5-a615-8614c36d886e
📒 Files selected for processing (20)
account/src/main/java/bisq/account/accounts/crypto/monero/MoneroSubaddressService.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/com/joemelsha/crypto/hash/Keccak.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/Curve.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/Ed25519Curve.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/CoordinateSystem.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedFieldElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedGroupElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Field.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519FieldElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Group.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519GroupElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ArrayUtils.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/HexEncoder.javaapps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java
✅ Files skipped from review due to trivial changes (4)
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/CoordinateSystem.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/Curve.java
- account/src/main/java/bisq/account/accounts/crypto/monero/MoneroSubaddressService.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519FieldElement.java
🚧 Files skipped from review as they are similar to previous changes (7)
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/HexEncoder.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/Ed25519Curve.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.java
- account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Field.java
- account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519GroupElement.java
Comment on lines
+239
to
+356
| public ByteBuffer digest() { | ||
| return digest(digestSize()); | ||
| } | ||
|
|
||
| public ByteBuffer digest(int outSize) { | ||
| return digest(outSize, false); | ||
| } | ||
|
|
||
| public ByteBuffer digest(int outSize, boolean direct) { | ||
| ByteBuffer buffer = direct ? ByteBuffer.allocateDirect(outSize) : ByteBuffer.allocate(outSize); | ||
| digest(buffer); | ||
| buffer.flip(); | ||
| return buffer; | ||
| } | ||
|
|
||
| public byte[] digestArray() { | ||
| return digestArray(digestSize()); | ||
| } | ||
|
|
||
| public byte[] digestArray(int outSize) { | ||
| byte[] array = new byte[outSize]; | ||
| digest(array, 0, outSize); | ||
| return array; | ||
| } | ||
|
|
||
| public void digest(byte[] out) { | ||
| digest(ByteBuffer.wrap(out)); | ||
| } | ||
|
|
||
| public void digest(byte[] out, int offset, int length) { | ||
| digest(ByteBuffer.wrap(out, offset, length)); | ||
| } | ||
|
|
||
| public void digest(ByteBuffer out) { | ||
| int outBytes = out.remaining(); | ||
| if (outBytes <= 0) | ||
| return; | ||
|
|
||
| long[] state = this.state; | ||
| int rateBits = this.rateBits; | ||
| int rateBytes; | ||
| if (!padded) { | ||
| pad(); | ||
| padded = true; | ||
| rateBits = 0; | ||
| rateBytes = 0; | ||
| } else { | ||
| if ((rateBits & 0x7) > 0) | ||
| throw new IllegalStateException("Cannot digest while in bit-mode"); //this could be implemented but would introduce considerable performance degradation - also, it's never technically possible. | ||
|
|
||
| rateBytes = rateBits >>> 3; | ||
| int rateBytesWord = rateBytes & 0x7; | ||
| if (rateBytesWord > 0) { | ||
| int c = 8 - rateBytesWord; | ||
| if (c > outBytes) | ||
| c = outBytes; | ||
| long w = state[rateBytes >>> 3]; | ||
| outBytes -= c; | ||
| rateBytes += c; | ||
| rateBytesWord <<= 3; | ||
| c = (c << 3) + rateBytesWord; | ||
| do { | ||
| out.put((byte) (w >>> rateBytesWord)); | ||
| rateBytesWord += 8; | ||
| } while (rateBytesWord < c); | ||
| if (outBytes <= 0) { | ||
| this.rateBits = rateBytes << 3; | ||
| return; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| int rateSizeWords = rateSizeBits >>> 6; | ||
| int rateWords = rateBytes >>> 3; | ||
|
|
||
| int outWords = outBytes >>> 3; | ||
| if (outWords > 0) { | ||
| ByteOrder order = out.order(); | ||
| try { | ||
| out.order(ByteOrder.LITTLE_ENDIAN); | ||
| do { | ||
| if (rateWords >= rateSizeWords) { | ||
| squeeze(); | ||
| rateWords = 0; | ||
| } | ||
| int c = rateSizeWords - rateWords; | ||
| if (c > outWords) | ||
| c = outWords; | ||
| outWords -= c; | ||
| c += rateWords; | ||
| do { | ||
| out.putLong(state[rateWords]); | ||
| rateWords++; | ||
| } while (rateWords < c); | ||
| } while (outWords > 0); | ||
| } finally { | ||
| out.order(order); | ||
| } | ||
| outBytes &= 0x7; | ||
| if (outBytes <= 0) { | ||
| this.rateBits = rateWords << 6; | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (rateWords >= rateSizeWords) { | ||
| squeeze(); | ||
| rateWords = 0; | ||
| } | ||
| long w = state[rateWords]; | ||
| outBytes <<= 3; | ||
| int i = 0; | ||
| do { | ||
| out.put((byte) (w >>> i)); | ||
| i += 8; | ||
| } while (i < outBytes); | ||
| this.rateBits = (rateWords << 6) | outBytes; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add known-answer tests for this Keccak path before release.
pad(), digest*(), and keccak() sit on the subaddress-derivation critical path. Please cover them with canonical Keccak-256 vectors and at least one Bisq 1 parity vector; otherwise a silent porting drift here will only show up as invalid Monero addresses downstream.
Also applies to: 362-482
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/com/joemelsha/crypto/hash/Keccak.java`
around lines 239 - 356, Add unit tests that exercise the Keccak implementation
paths used in subaddress derivation: specifically call pad(), the various digest
overloads (digest(), digest(int), digest(byte[]), digest(ByteBuffer)) and
keccak() to produce outputs, and assert against canonical Keccak-256
known-answer vectors plus at least one Bisq 1 parity vector; place tests to
cover both the byte-wise and bit/partial-word branches (the code paths guarded
by padded flag and the rateBits/rateBytes handling) so any porting drift in
these methods is detected. Ensure tests validate outputs from digestArray(int),
digest(ByteBuffer) with direct and non-direct buffers, and include vectors that
force the rateWords/rateBytes intermediate branches (e.g., outputs larger than
one rate block and outputs with trailing partial bytes). Run these tests as part
of CI so pad(), digest*, and keccak() regressions are caught before release.
| return bytesToHex(array); | ||
| } | ||
|
|
||
| final protected static char[] hexArray = "0123456789abcdef".toCharArray(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "ByteUtil.java" | head -20Repository: bisq-network/bisq2
Length of output: 157
🏁 Script executed:
cat -n account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.java | head -30Repository: bisq-network/bisq2
Length of output: 1270
🏁 Script executed:
rg "hexArray" --type java -B 2 -A 2Repository: bisq-network/bisq2
Length of output: 1473
Make the hex lookup table private and follow constant naming conventions.
The protected static modifier unnecessarily exposes this internal lookup table. The field is only used within ByteUtil itself and should be private. Additionally, use HEX_ARRAY to follow the standard naming convention for static final fields.
🔧 Proposed fix
- final protected static char[] hexArray = "0123456789abcdef".toCharArray();
+ private static final char[] HEX_ARRAY = "0123456789abcdef".toCharArray();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.java`
at line 13, Change the lookup table field in ByteUtil: make the field private,
keep it static final, and rename hexArray to follow constant conventions
(HEX_ARRAY); update any internal references in the ByteUtil class (e.g., methods
that currently reference hexArray) to use HEX_ARRAY so the field is no longer
exposed as protected.
Comment on lines
+61
to
+64
| public static byte[] longToLittleEndianUint32ByteArray(long value) { | ||
| byte[] array = new byte[4]; | ||
| for (int i = 3; i >= 0; i--) array[i] = (byte) (value >> i * 8); | ||
| return array; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.java | head -80Repository: bisq-network/bisq2
Length of output: 3020
🏁 Script executed:
rg -r "longToLittleEndianUint32ByteArray" --type javaRepository: bisq-network/bisq2
Length of output: 458
🏁 Script executed:
rg "longToLittleEndianUint32ByteArray" --type javaRepository: bisq-network/bisq2
Length of output: 713
🏁 Script executed:
rg -A 5 -B 5 "longToLittleEndianUint32ByteArray(accountId)" account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.javaRepository: bisq-network/bisq2
Length of output: 44
🏁 Script executed:
cat -n account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java | grep -A 10 -B 10 "longToLittleEndianUint32ByteArray"Repository: bisq-network/bisq2
Length of output: 2602
Fix K&R brace style violation and add uint32 bounds validation.
The for loop at line 63 lacks braces, violating the K&R brace style requirement ("always use braces"). Additionally, the method accepts a long but silently truncates values outside the uint32 range (0 to 0xFFFF_FFFF); add validation to prevent undetected overflow since callers may pass unchecked values.
Proposed fix
public static byte[] longToLittleEndianUint32ByteArray(long value) {
+ if (value < 0 || value > 0xFFFF_FFFFL) {
+ throw new IllegalArgumentException("Value out of uint32 range: " + value);
+ }
byte[] array = new byte[4];
- for (int i = 3; i >= 0; i--) array[i] = (byte) (value >> i * 8);
+ for (int i = 3; i >= 0; i--) {
+ array[i] = (byte) (value >> (i * 8));
+ }
return array;
}📝 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
| public static byte[] longToLittleEndianUint32ByteArray(long value) { | |
| byte[] array = new byte[4]; | |
| for (int i = 3; i >= 0; i--) array[i] = (byte) (value >> i * 8); | |
| return array; | |
| public static byte[] longToLittleEndianUint32ByteArray(long value) { | |
| if (value < 0 || value > 0xFFFF_FFFFL) { | |
| throw new IllegalArgumentException("Value out of uint32 range: " + value); | |
| } | |
| byte[] array = new byte[4]; | |
| for (int i = 3; i >= 0; i--) { | |
| array[i] = (byte) (value >> (i * 8)); | |
| } | |
| return array; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.java`
around lines 61 - 64, The method longToLittleEndianUint32ByteArray silently
truncates out-of-range values and uses a brace-less for loop; update
longToLittleEndianUint32ByteArray to validate the input is within the uint32
range (0 <= value <= 0xFFFFFFFF) and throw an IllegalArgumentException for
out-of-range values, and add explicit braces around the for loop body that
populates the byte[] array (array variable) to satisfy the K&R style rule.
Comment on lines
+12
to
+15
| /** | ||
| * 2^252 - 27742317777372353535851937790883648493 | ||
| */ | ||
| public static final BigInteger GROUP_ORDER = BigInteger.ONE.shiftLeft(252).add(new BigInteger("27742317777372353535851937790883648493")); |
There was a problem hiding this comment.
Comment incorrectly states subtraction; code correctly uses addition.
The Ed25519 group order is 2^252 + 27742317777372353535851937790883648493. The code is correct (uses .add()), but the comment states 2^252 - ... which is misleading.
📝 Proposed fix
/**
- * 2^252 - 27742317777372353535851937790883648493
+ * 2^252 + 27742317777372353535851937790883648493
*/
public static final BigInteger GROUP_ORDER = BigInteger.ONE.shiftLeft(252).add(new BigInteger("27742317777372353535851937790883648493"));📝 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
| /** | |
| * 2^252 - 27742317777372353535851937790883648493 | |
| */ | |
| public static final BigInteger GROUP_ORDER = BigInteger.ONE.shiftLeft(252).add(new BigInteger("27742317777372353535851937790883648493")); | |
| /** | |
| * 2^252 + 27742317777372353535851937790883648493 | |
| */ | |
| public static final BigInteger GROUP_ORDER = BigInteger.ONE.shiftLeft(252).add(new BigInteger("27742317777372353535851937790883648493")); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Group.java`
around lines 12 - 15, The javadoc for GROUP_ORDER in Ed25519Group incorrectly
states the formula as "2^252 - 27742317777372353535851937790883648493" while the
constant GROUP_ORDER (BigInteger.ONE.shiftLeft(252).add(...)) uses addition;
update the comment to the correct expression "2^252 +
27742317777372353535851937790883648493" so the comment matches the
implementation and references the GROUP_ORDER constant.
...esktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java
Show resolved
Hide resolved
sdmg15
force-pushed
the
trading/monero-subaddress
branch
from
e8143b0 to
709f340
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java (1)
88-91:⚠️ Potential issue | 🟡 MinorInput validation in
encode_blockchecks wrong constant.The method receives raw data blocks (max 8 bytes =
full_block_size), but the validation checks againstfull_encoded_block_size(11). While callers currently pass valid sizes, the validation is incorrect.🐛 Proposed fix
private static void encode_block(byte[] data, char[] buf, int index) { - if (data.length < 1 || data.length > full_encoded_block_size) { + if (data.length < 1 || data.length > full_block_size) { throw new RuntimeException("Invalid block length: " + data.length); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java` around lines 88 - 91, The input validation in encode_block currently compares data.length to full_encoded_block_size (11) but should validate against the raw input max full_block_size (8); update the check in encode_block to ensure data.length >= 1 && data.length <= full_block_size (use the full_block_size constant) and keep throwing the same RuntimeException with the invalid length message; reference the encode_block method and the full_block_size and full_encoded_block_size constants to locate and correct the condition.apps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java (1)
138-139:⚠️ Potential issue | 🟠 MajorAvoid logging full cryptocurrency addresses.
Logging the complete
subAddressexposes sensitive financial information that could be used to track user transactions. This same issue exists inMoneroSubaddressService.java:39.🔒 Proposed fix
- log.info("Created new subAddress {}. Took {} ms.", subAddress, System.currentTimeMillis() - start); + log.info("Created new Monero subAddress. Took {} ms.", System.currentTimeMillis() - start);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java` around lines 138 - 139, The log in MoneroFormController (and similarly in MoneroSubaddressService) must not emit the full cryptocurrency subAddress; change the logging call that currently references the variable subAddress to either omit the address entirely or log a non-sensitive representation (e.g., a masked version or a short hash/last-N characters) and keep the timing info; update the log invocation in the method that calls walletAddress.getSubaddressBase58(...) so it logs only the masked/hashed/address-suffix or a placeholder token instead of the full subAddress.
🧹 Nitpick comments (8)
apps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java (1)
31-32: Import statement placement.The static import at line 31 and the regular import at line 32 should be grouped with their respective import sections (static imports typically go first, followed by a blank line, then regular imports).
♻️ Proposed fix - move imports to proper locations
package bisq.desktop.main.content.user.accounts.crypto_accounts.create.data.form; +import static com.google.common.base.Preconditions.checkArgument; + import bisq.account.accounts.crypto.monero.MoneroAccountPayload; +import bisq.account.accounts.crypto.monero.knaccc.monero.address.WalletAddress; import bisq.account.payment_method.DigitalAssetPaymentMethod; import bisq.common.util.StringUtils; import bisq.desktop.ServiceProvider; import bisq.presentation.parser.AmountParser; import lombok.extern.slf4j.Slf4j; import org.fxmisc.easybind.EasyBind; import org.fxmisc.easybind.Subscription; import java.util.Optional; - -import static com.google.common.base.Preconditions.checkArgument; -import bisq.account.accounts.crypto.monero.knaccc.monero.address.WalletAddress; -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java` around lines 31 - 32, Move the misplaced imports in MoneroFormController.java: place the static import of checkArgument into the static-import block (above other imports) and ensure the regular import of WalletAddress is in the normal import section separated by a blank line; this groups static imports first and regular imports after with proper spacing.account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java (4)
58-64: Add braces toif/elsestatements.Per coding guidelines, all control structures must have braces.
♻️ Proposed fix
private static byte[] uint64_to_8be(BigInteger n, int size) { byte[] r = new byte[size]; byte[] a = n.toByteArray(); - if (a.length <= r.length) System.arraycopy(a, 0, r, r.length - a.length, a.length); - else System.arraycopy(a, a.length - r.length, r, 0, r.length); + if (a.length <= r.length) { + System.arraycopy(a, 0, r, r.length - a.length, a.length); + } else { + System.arraycopy(a, a.length - r.length, r, 0, r.length); + } return r; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java` around lines 58 - 64, The if/else in the uint64_to_8be method of class Base58 lacks braces; update the method so both the if (a.length <= r.length) and the else branch have explicit braces around their System.arraycopy calls to conform to the coding guideline requiring braces for all control structures (locate method uint64_to_8be and wrap each branch body in { ... }).
7-14: Consider making constant fieldsfinal.These static fields are effectively constants and should be declared
finalto prevent accidental modification and communicate intent.♻️ Proposed fix
- public static String alphabet = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz"; + public static final String ALPHABET = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz"; - public static int[] encoded_block_sizes = new int[]{0, 2, 3, 5, 6, 7, 9, 10, 11}; + public static final int[] ENCODED_BLOCK_SIZES = new int[]{0, 2, 3, 5, 6, 7, 9, 10, 11}; - public static int full_block_size = 8; - public static int full_encoded_block_size = 11; + public static final int FULL_BLOCK_SIZE = 8; + public static final int FULL_ENCODED_BLOCK_SIZE = 11; - public static BigInteger UINT64_MAX = BigInteger.valueOf(2).pow(64); + public static final BigInteger UINT64_MAX = BigInteger.valueOf(2).pow(64);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java` around lines 7 - 14, These static fields in Base58 (alphabet, encoded_block_sizes, full_block_size, full_encoded_block_size, UINT64_MAX) are intended constants; make them immutable by declaring them as final (e.g., change their declarations to static final) and for the array encoded_block_sizes consider using an unmodifiable copy or mark it private and expose a read-only view if needed so callers cannot mutate the array at runtime; update references accordingly.
71-72: Add braces to control structures.Per coding guidelines, braces are required for all control statements.
♻️ Proposed fix
- for (int i = 0; i < encoded_block_sizes.length; i++) - if (encoded_block_sizes[i] == last_block_size) last_block_decoded_size = i; + for (int i = 0; i < encoded_block_sizes.length; i++) { + if (encoded_block_sizes[i] == last_block_size) { + last_block_decoded_size = i; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java` around lines 71 - 72, In the Base58 class update the for loop and its inner if to use explicit braces so control flow is clear and complies with coding guidelines: replace the single-line for (int i = 0; i < encoded_block_sizes.length; i++) if (encoded_block_sizes[i] == last_block_size) last_block_decoded_size = i; with a braced for block and a braced if block that assigns last_block_decoded_size when encoded_block_sizes[i] equals last_block_size (variables: encoded_block_sizes, last_block_size, last_block_decoded_size).
16-20: Add braces to single-lineifstatements.The coding guidelines require braces on all control structures. As per coding guidelines: "Follow K&R brace style and always use braces".
♻️ Proposed fix
public static int lookupIndex(int[] array, int match) { int index = -1; - for (int i = 0; i < array.length; i++) if (array[i] == match) index = i; + for (int i = 0; i < array.length; i++) { + if (array[i] == match) { + index = i; + } + } return index; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java` around lines 16 - 20, The method Base58.lookupIndex contains a single-line if without braces; update the loop in lookupIndex(int[] array, int match) to use K&R-style braces for the if body (always enclose the assignment index = i inside { ... }) so all control structures follow the project's brace guideline; ensure indentation remains consistent with surrounding code.account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.java (2)
13-18: Consider adding input validation for byte array length.Both
bytesToLongandbytesToIntassume the input array has exactly 8 and 4 bytes respectively. Passing a shorter array will throwIndexOutOfBoundsExceptionwithout a clear message. Since this is cryptographic utility code, explicit validation could aid debugging.🛡️ Proposed defensive validation
public static long bytesToLong(final byte[] bytes) { + if (bytes == null || bytes.length < 8) { + throw new IllegalArgumentException("Input must be at least 8 bytes"); + } final ByteBuffer buffer = ByteBuffer.allocate(8); buffer.put(bytes, 0, 8); buffer.flip(); return buffer.getLong(); }public static int bytesToInt(final byte[] bytes) { + if (bytes == null || bytes.length < 4) { + throw new IllegalArgumentException("Input must be at least 4 bytes"); + } final ByteBuffer buffer = ByteBuffer.allocate(4); buffer.put(bytes, 0, 4); buffer.flip(); return buffer.getInt(); }Also applies to: 38-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.java` around lines 13 - 18, Add explicit input validation to ByteUtils.bytesToLong and ByteUtils.bytesToInt: check for null and that bytes.length is at least 8 for bytesToLong (and at least 4 for bytesToInt), and throw an IllegalArgumentException with a clear message (e.g., "bytesToLong requires an 8-byte array, got X") instead of letting ByteBuffer.put throw an IndexOutOfBoundsException; update the methods (ByteUtils.bytesToLong and ByteUtils.bytesToInt) to validate length before allocating/putting into the ByteBuffer.
74-82: Javadoc inconsistency: parameter isintbut documentation says "byte".The method works correctly for detecting negative integers, but the Javadoc is misleading since it describes the parameter as "The byte to check" while the actual parameter type is
int.📝 Proposed fix
/** - * Constant-time check if byte is negative. The constant time behavior eliminates side channel attacks. + * Constant-time check if int is negative. The constant time behavior eliminates side channel attacks. * - * `@param` b The byte to check. + * `@param` b The int to check. * `@return` 1 if the byte is negative, 0 otherwise. */ public static int isNegativeConstantTime(final int b) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.java` around lines 74 - 82, Update the Javadoc on ByteUtils.isNegativeConstantTime to accurately describe the parameter type and semantics: change the parameter description from "The byte to check" to something like "The int value to check (only low-order byte is considered for sign)" or "The int value whose sign bit will be checked" and adjust the method description to refer to an int rather than a byte; ensure the `@param` tag names the parameter "b" and the `@return` tag still documents "1 if the value is negative, 0 otherwise" so the docs match the actual method signature and behavior.account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519FieldElement.java (1)
22-37: Mutable internal state exposed via constructor and getter.The constructor stores the caller's array directly (
this.values = values) andgetRaw()returns it without cloning. This makesEd25519FieldElementexternally mutable, which could cause subtle bugs if callers modify the array after construction.For a crypto primitive that participates in equality checks and encoding, consider defensive copying:
Suggested fix
public Ed25519FieldElement(final int[] values) { if (values.length != 10) { throw new IllegalArgumentException("Invalid 2^25.5 bit representation."); } - this.values = values; + this.values = values.clone(); } public int[] getRaw() { - return this.values; + return this.values.clone(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519FieldElement.java` around lines 22 - 37, The Ed25519FieldElement constructor and getRaw() expose mutable internal state by assigning and returning the caller's int[] directly; fix this by defensively copying the array: in the constructor (Ed25519FieldElement(final int[] values)) create and store a new int[10] copy of values (after validating length) instead of assigning the reference, make the backing field effectively final/unmodifiable, and change getRaw() to return a copy of the internal array rather than the internal reference (preserve the same length/validation semantics).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedGroupElement.java`:
- Around line 112-120: The equals method in Ed25519EncodedGroupElement calls
ArrayUtils.isEqualConstantTime(this.values, encoded.values) which can throw
ArrayIndexOutOfBoundsException if the arrays differ in length; add a defensive
length check before calling the constant-time comparator (in
Ed25519EncodedGroupElement.equals) or fix ArrayUtils.isEqualConstantTime to
first check lengths and incorporate the length difference into the constant-time
result (e.g., compare up to max length while folding any length mismatch into
the final equality flag) so no out-of-bounds access can occur and the timing
properties are preserved.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519GroupElement.java`:
- Around line 740-751: The cmov method leaks the secret bit because it uses loop
counts/branches; replace it with a constant-time conditional move that does not
branch on b. Implement cmov(Ed25519GroupElement u, int b) by creating a new
Ed25519GroupElement ret and for each internal field element (e.g., X, Y, Z, T)
perform a constant-time conditional move using a mask derived from b (e.g., mask
= 0 - (b & 1)) and per-limb bitwise selects (or call an existing
fe_cmov/FieldElement.cmov helper) so each limb is chosen via (mask & u.limb) |
(~mask & this.limb) rather than branches or loop counts. Ensure b is normalized
to 0 or 1 before mask creation and operate on all internal fields to avoid
leaking via partial updates.
- Around line 359-380: precomputedForSingle is assigned without synchronization
in precomputeForScalarMultiplication(), causing a data race if multiple threads
call it; fix by ensuring safe publication—either declare the field
precomputedForSingle as volatile or guard the check-then-act with
synchronization (e.g., synchronized(this) around the null check and assignment)
so only one thread builds and publishes the table atomically; update references
in Ed25519GroupElement.precomputeForScalarMultiplication and the field
declaration for precomputedForSingle accordingly.
- Around line 19-39: Ed25519GroupElement currently implements Serializable but
lacks a serialVersionUID and its field type Ed25519FieldElement is not
serializable; either remove "implements Serializable" from the
Ed25519GroupElement declaration, or (if serialization is intended) add a private
static final long serialVersionUID to Ed25519GroupElement and make
Ed25519FieldElement implement Serializable as well (update Ed25519FieldElement
class signature and ensure its fields are serializable). Locate the
Ed25519GroupElement class and fields X, Y, Z, T and choose one of the two fixes
consistently across both classes.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.java`:
- Around line 64-72: isEqualConstantTime currently only propagates the lower 8
bits because the loop runs 8 times; change the bit-propagation to cover the full
32-bit int so differences in upper bits are detected (e.g., iterate 32 times or
otherwise collapse all 32 bits of xor into result). Locate the method
isEqualConstantTime(int b, int c) in ByteUtils and update the loop (or logic) to
shift through 32 bit positions (0..31) or use a full int-check pattern so
ArrayUtils.isEqualConstantTime(byte[], byte[]) which may set high bits behaves
correctly.
---
Duplicate comments:
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java`:
- Around line 88-91: The input validation in encode_block currently compares
data.length to full_encoded_block_size (11) but should validate against the raw
input max full_block_size (8); update the check in encode_block to ensure
data.length >= 1 && data.length <= full_block_size (use the full_block_size
constant) and keep throwing the same RuntimeException with the invalid length
message; reference the encode_block method and the full_block_size and
full_encoded_block_size constants to locate and correct the condition.
In
`@apps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java`:
- Around line 138-139: The log in MoneroFormController (and similarly in
MoneroSubaddressService) must not emit the full cryptocurrency subAddress;
change the logging call that currently references the variable subAddress to
either omit the address entirely or log a non-sensitive representation (e.g., a
masked version or a short hash/last-N characters) and keep the timing info;
update the log invocation in the method that calls
walletAddress.getSubaddressBase58(...) so it logs only the
masked/hashed/address-suffix or a placeholder token instead of the full
subAddress.
---
Nitpick comments:
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.java`:
- Around line 58-64: The if/else in the uint64_to_8be method of class Base58
lacks braces; update the method so both the if (a.length <= r.length) and the
else branch have explicit braces around their System.arraycopy calls to conform
to the coding guideline requiring braces for all control structures (locate
method uint64_to_8be and wrap each branch body in { ... }).
- Around line 7-14: These static fields in Base58 (alphabet,
encoded_block_sizes, full_block_size, full_encoded_block_size, UINT64_MAX) are
intended constants; make them immutable by declaring them as final (e.g., change
their declarations to static final) and for the array encoded_block_sizes
consider using an unmodifiable copy or mark it private and expose a read-only
view if needed so callers cannot mutate the array at runtime; update references
accordingly.
- Around line 71-72: In the Base58 class update the for loop and its inner if to
use explicit braces so control flow is clear and complies with coding
guidelines: replace the single-line for (int i = 0; i <
encoded_block_sizes.length; i++) if (encoded_block_sizes[i] == last_block_size)
last_block_decoded_size = i; with a braced for block and a braced if block that
assigns last_block_decoded_size when encoded_block_sizes[i] equals
last_block_size (variables: encoded_block_sizes, last_block_size,
last_block_decoded_size).
- Around line 16-20: The method Base58.lookupIndex contains a single-line if
without braces; update the loop in lookupIndex(int[] array, int match) to use
K&R-style braces for the if body (always enclose the assignment index = i inside
{ ... }) so all control structures follow the project's brace guideline; ensure
indentation remains consistent with surrounding code.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519FieldElement.java`:
- Around line 22-37: The Ed25519FieldElement constructor and getRaw() expose
mutable internal state by assigning and returning the caller's int[] directly;
fix this by defensively copying the array: in the constructor
(Ed25519FieldElement(final int[] values)) create and store a new int[10] copy of
values (after validating length) instead of assigning the reference, make the
backing field effectively final/unmodifiable, and change getRaw() to return a
copy of the internal array rather than the internal reference (preserve the same
length/validation semantics).
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.java`:
- Around line 13-18: Add explicit input validation to ByteUtils.bytesToLong and
ByteUtils.bytesToInt: check for null and that bytes.length is at least 8 for
bytesToLong (and at least 4 for bytesToInt), and throw an
IllegalArgumentException with a clear message (e.g., "bytesToLong requires an
8-byte array, got X") instead of letting ByteBuffer.put throw an
IndexOutOfBoundsException; update the methods (ByteUtils.bytesToLong and
ByteUtils.bytesToInt) to validate length before allocating/putting into the
ByteBuffer.
- Around line 74-82: Update the Javadoc on ByteUtils.isNegativeConstantTime to
accurately describe the parameter type and semantics: change the parameter
description from "The byte to check" to something like "The int value to check
(only low-order byte is considered for sign)" or "The int value whose sign bit
will be checked" and adjust the method description to refer to an int rather
than a byte; ensure the `@param` tag names the parameter "b" and the `@return` tag
still documents "1 if the value is negative, 0 otherwise" so the docs match the
actual method signature and behavior.
In
`@apps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java`:
- Around line 31-32: Move the misplaced imports in MoneroFormController.java:
place the static import of checkArgument into the static-import block (above
other imports) and ensure the regular import of WalletAddress is in the normal
import section separated by a blank line; this groups static imports first and
regular imports after with proper spacing.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aece761b-c973-434a-88cb-99a1c61cf1c5
📒 Files selected for processing (20)
account/src/main/java/bisq/account/accounts/crypto/monero/MoneroSubaddressService.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/com/joemelsha/crypto/hash/Keccak.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/Base58.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/Curve.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/Ed25519Curve.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/CoordinateSystem.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedFieldElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedGroupElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Field.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519FieldElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Group.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519GroupElement.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ArrayUtils.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.javaaccount/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/HexEncoder.javaapps/desktop/desktop/src/main/java/bisq/desktop/main/content/user/accounts/crypto_accounts/create/data/form/MoneroFormController.java
✅ Files skipped from review due to trivial changes (8)
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/CoordinateSystem.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/HexEncoder.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/Curve.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Group.java
- account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/Scalar.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519Field.java
- account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/ByteUtil.java
- account/src/main/java/bisq/account/accounts/crypto/monero/com/joemelsha/crypto/hash/Keccak.java
🚧 Files skipped from review as they are similar to previous changes (6)
- account/src/main/java/bisq/account/accounts/crypto/monero/MoneroSubaddressService.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/Ed25519Curve.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ArrayUtils.java
- account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/crypto/CryptoUtil.java
- account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedFieldElement.java
- account/src/main/java/bisq/account/accounts/crypto/monero/knaccc/monero/address/WalletAddress.java
Comment on lines
+112
to
+120
| @Override | ||
| public boolean equals(final Object obj) { | ||
| if (!(obj instanceof Ed25519EncodedGroupElement)) { | ||
| return false; | ||
| } | ||
|
|
||
| final Ed25519EncodedGroupElement encoded = (Ed25519EncodedGroupElement) obj; | ||
| return 1 == ArrayUtils.isEqualConstantTime(this.values, encoded.values); | ||
| } |
There was a problem hiding this comment.
Potential ArrayIndexOutOfBoundsException in constant-time comparison.
The equals() method uses ArrayUtils.isEqualConstantTime(this.values, encoded.values). Per the implementation in ArrayUtils.java (lines 106-121), the loop iterates over b.length indices but accesses c[i] without bounds checking. If b.length > c.length, this throws an ArrayIndexOutOfBoundsException.
While both arrays here are validated to be 32 bytes, this is fragile — the precondition is enforced only by the constructor. If a future refactor or serialization path bypasses validation, the constant-time guarantee is lost (an exception leaks timing information).
Consider validating lengths match before the loop in ArrayUtils.isEqualConstantTime, or adding a defensive length check here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519EncodedGroupElement.java`
around lines 112 - 120, The equals method in Ed25519EncodedGroupElement calls
ArrayUtils.isEqualConstantTime(this.values, encoded.values) which can throw
ArrayIndexOutOfBoundsException if the arrays differ in length; add a defensive
length check before calling the constant-time comparator (in
Ed25519EncodedGroupElement.equals) or fix ArrayUtils.isEqualConstantTime to
first check lengths and incorporate the length difference into the constant-time
result (e.g., compare up to max length while folding any length mismatch into
the final equality flag) so no out-of-bounds access can occur and the timing
properties are preserved.
Comment on lines
+19
to
+39
| public class Ed25519GroupElement implements Serializable { | ||
|
|
||
| private final CoordinateSystem coordinateSystem; | ||
| @SuppressWarnings("NonConstantFieldWithUpperCaseName") | ||
| private final Ed25519FieldElement X; | ||
| @SuppressWarnings("NonConstantFieldWithUpperCaseName") | ||
| private final Ed25519FieldElement Y; | ||
| @SuppressWarnings("NonConstantFieldWithUpperCaseName") | ||
| private final Ed25519FieldElement Z; | ||
| @SuppressWarnings("NonConstantFieldWithUpperCaseName") | ||
| private final Ed25519FieldElement T; | ||
|
|
||
| /** | ||
| * Precomputed table for a single scalar multiplication. | ||
| */ | ||
| private Ed25519GroupElement[][] precomputedForSingle; | ||
|
|
||
| /** | ||
| * Precomputed table for a double scalar multiplication | ||
| */ | ||
| private Ed25519GroupElement[] precomputedForDouble; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if Ed25519FieldElement implements Serializable
rg -n "class Ed25519FieldElement" --type=java -A 2 | head -20Repository: bisq-network/bisq2
Length of output: 504
🏁 Script executed:
#!/bin/bash
# Read the Ed25519GroupElement class declaration area (lines 1-50)
head -50 account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519GroupElement.javaRepository: bisq-network/bisq2
Length of output: 2153
🏁 Script executed:
#!/bin/bash
# Search for serialVersionUID in both classes
rg -n "serialVersionUID" account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/ --type=javaRepository: bisq-network/bisq2
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Check if Ed25519FieldElement has any Serializable interface or serialVersionUID
cat account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519FieldElement.java | head -30Repository: bisq-network/bisq2
Length of output: 996
🏁 Script executed:
#!/bin/bash
# Search for any ObjectInputStream/ObjectOutputStream usage to see if serialization is actually used
rg -n "ObjectInputStream|ObjectOutputStream|readObject|writeObject" account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/ --type=javaRepository: bisq-network/bisq2
Length of output: 44
Remove implements Serializable or add serialVersionUID and make Ed25519FieldElement serializable.
Ed25519GroupElement declares implements Serializable, but:
- No explicit
serialVersionUIDis defined — compiler-generated UIDs are fragile across versions. - Fields
X,Y,Z,Tare of typeEd25519FieldElement, which does not implementSerializable. This will causeNotSerializableExceptionat runtime if the object is serialized.
If serialization is intentional, add serialVersionUID and implement Serializable in Ed25519FieldElement. Otherwise, remove implements Serializable from Ed25519GroupElement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519GroupElement.java`
around lines 19 - 39, Ed25519GroupElement currently implements Serializable but
lacks a serialVersionUID and its field type Ed25519FieldElement is not
serializable; either remove "implements Serializable" from the
Ed25519GroupElement declaration, or (if serialization is intended) add a private
static final long serialVersionUID to Ed25519GroupElement and make
Ed25519FieldElement implement Serializable as well (update Ed25519FieldElement
class signature and ensure its fields are serializable). Locate the
Ed25519GroupElement class and fields X, Y, Z, T and choose one of the two fixes
consistently across both classes.
Comment on lines
+359
to
+380
| public void precomputeForScalarMultiplication() { | ||
| if (null != this.precomputedForSingle) { | ||
| return; | ||
| } | ||
|
|
||
| Ed25519GroupElement Bi = this; | ||
| this.precomputedForSingle = new Ed25519GroupElement[32][8]; | ||
|
|
||
| for (int i = 0; i < 32; i++) { | ||
| Ed25519GroupElement Bij = Bi; | ||
| for (int j = 0; j < 8; j++) { | ||
| final Ed25519FieldElement inverse = Bij.Z.invert(); | ||
| final Ed25519FieldElement x = Bij.X.multiply(inverse); | ||
| final Ed25519FieldElement y = Bij.Y.multiply(inverse); | ||
| this.precomputedForSingle[i][j] = precomputed(y.add(x), y.subtract(x), x.multiply(y).multiply(Ed25519Field.D_Times_TWO)); | ||
| Bij = Bij.add(Bi.toCached()).toP3(); | ||
| } | ||
| // Only every second summand is precomputed (16^2 = 256). | ||
| for (int k = 0; k < 8; k++) { | ||
| Bi = Bi.add(Bi.toCached()).toP3(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Data race on precomputedForSingle in multi-threaded contexts.
precomputeForScalarMultiplication() performs a check-then-act on precomputedForSingle without synchronization:
if (null != this.precomputedForSingle) { return; }
// ... build table ...
this.precomputedForSingle = new Ed25519GroupElement[32][8];If two threads call this concurrently, both may pass the null check and build the table, leading to:
- Wasted computation (minor)
- One thread may read a partially-initialized table written by another (critical)
Since Ed25519Group.BASE_POINT is initialized statically and precomputed during class loading, this is likely safe in practice. However, for robustness, consider making precomputedForSingle volatile or synchronizing access.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519GroupElement.java`
around lines 359 - 380, precomputedForSingle is assigned without synchronization
in precomputeForScalarMultiplication(), causing a data race if multiple threads
call it; fix by ensuring safe publication—either declare the field
precomputedForSingle as volatile or guard the check-then-act with
synchronization (e.g., synchronized(this) around the null check and assignment)
so only one thread builds and publishes the table atomically; update references
in Ed25519GroupElement.precomputeForScalarMultiplication and the field
declaration for precomputedForSingle accordingly.
Comment on lines
+740
to
+751
| private Ed25519GroupElement cmov(final Ed25519GroupElement u, final int b) { | ||
| Ed25519GroupElement ret = null; | ||
| for (int i = 0; i < b; i++) { | ||
| // Only for b == 1 | ||
| ret = u; | ||
| } | ||
| for (int i = 0; i < 1 - b; i++) { | ||
| // Only for b == 0 | ||
| ret = this; | ||
| } | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
cmov is not constant-time — loop optimization leaks the secret bit.
The implementation uses loop iteration count to conditionally assign ret:
for (int i = 0; i < b; i++) { ret = u; } // runs 0 or 1 times
for (int i = 0; i < 1 - b; i++) { ret = this; } // runs 1 or 0 timesThis is not constant-time because:
- The JIT compiler can trivially optimize empty loops away
- Even without optimization, branch prediction and cache timing differ based on
b - The loop trip count directly reveals
b
For cryptographic constant-time selection, use bitwise operations:
Suggested constant-time implementation
private Ed25519GroupElement cmov(final Ed25519GroupElement u, final int b) {
- Ed25519GroupElement ret = null;
- for (int i = 0; i < b; i++) {
- // Only for b == 1
- ret = u;
- }
- for (int i = 0; i < 1 - b; i++) {
- // Only for b == 0
- ret = this;
- }
- return ret;
+ // b must be 0 or 1
+ // Returns u if b == 1, this if b == 0
+ return (b == 1) ? u : this;
+ // Note: For true constant-time, field elements should be conditionally
+ // swapped using bitwise masks on the underlying int[] arrays.
}For a truly constant-time implementation, you would need to conditionally swap the underlying field element values using masks, similar to how ref10 does fe_cmov.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/crypto/ed25519/arithmetic/Ed25519GroupElement.java`
around lines 740 - 751, The cmov method leaks the secret bit because it uses
loop counts/branches; replace it with a constant-time conditional move that does
not branch on b. Implement cmov(Ed25519GroupElement u, int b) by creating a new
Ed25519GroupElement ret and for each internal field element (e.g., X, Y, Z, T)
perform a constant-time conditional move using a mask derived from b (e.g., mask
= 0 - (b & 1)) and per-limb bitwise selects (or call an existing
fe_cmov/FieldElement.cmov helper) so each limb is chosen via (mask & u.limb) |
(~mask & this.limb) rather than branches or loop counts. Ensure b is normalized
to 0 or 1 before mask creation and operate on all internal fields to avoid
leaking via partial updates.
Comment on lines
+64
to
+72
| public static int isEqualConstantTime(final int b, final int c) { | ||
| int result = 0; | ||
| final int xor = b ^ c; | ||
| for (int i = 0; i < 8; i++) { | ||
| result |= xor >> i; | ||
| } | ||
|
|
||
| return (result ^ 0x01) & 0x01; | ||
| } |
There was a problem hiding this comment.
Critical: Constant-time comparison only checks lower 8 bits, causing false equality for values differing in upper bits.
The loop iterates 8 times (shifts 0–7), so only the lower 8 bits of the XOR cascade down to bit 0. Differences in bits 8–31 are not detected.
Example: isEqualConstantTime(0x100, 0) incorrectly returns 1 (equal) because bit 8 never reaches bit 0.
This breaks ArrayUtils.isEqualConstantTime(byte[], byte[]) which accumulates b.length - c.length (can set higher bits) before calling this method.
🐛 Proposed fix: iterate 32 times for full int comparison
public static int isEqualConstantTime(final int b, final int c) {
int result = 0;
final int xor = b ^ c;
- for (int i = 0; i < 8; i++) {
+ for (int i = 0; i < 32; i++) {
result |= xor >> i;
}
return (result ^ 0x01) & 0x01;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@account/src/main/java/bisq/account/accounts/crypto/monero/org/nem/core/utils/ByteUtils.java`
around lines 64 - 72, isEqualConstantTime currently only propagates the lower 8
bits because the loop runs 8 times; change the bit-propagation to cover the full
32-bit int so differences in upper bits are detected (e.g., iterate 32 times or
otherwise collapse all 32 bits of xor into result). Locate the method
isEqualConstantTime(int b, int c) in ByteUtils and update the loop (or logic) to
shift through 32 bit positions (0..31) or use a full int-check pattern so
ArrayUtils.isEqualConstantTime(byte[], byte[]) which may set high bits behaves
correctly.
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.
This PR attempts to introduce monero subaddress generation.
Closes #4373
Summary by CodeRabbit
New Features
Refactor
Chores