feat: improve txmetadata features #22
Conversation
HashEngineering
force-pushed
the
feat/improve-txmetadata
branch
from
b31d98e to
29d7618
Compare
|
merge with |
…form into feat/improve-txmetadata
…form into feat/improve-txmetadata
…n-platform into feat/improve-txmetadata
…n-platform into feat/improve-txmetadata
HashEngineering
changed the title
…form into feat/improve-txmetadata
WalkthroughVersion/build bumps; large SWIG/FFI remapping and ignore-rule overhaul; introduced wallet-utils protobuf and migrated TxMetadata to versioned CBOR/Protobuf batching with encryption, publish, decryption, and progress; updated platform app IDs and examples; expanded tests; small Rust SDK error-handling and export visibility changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant BI as BlockchainIdentity
participant TM as TxMetadata
participant Doc as Platform Documents
App->>BI: publishTxMetaData(items, keyParam?, encKeyIndex, version, progress?)
BI->>BI: createTxMetadata(items, keyParam?, encKeyIndex, version)
note right of BI `#DFF0D8`: Batch items -> getBuffer(version)\nEncrypt per-batch payload, produce Documents
loop per generated document
BI->>Doc: publish(document)
Doc-->>BI: ack
BI-->>App: optional progress update
end
BI-->>App: all documents published
sequenceDiagram
autonumber
participant Caller
participant TMD as TxMetadataDocument
participant Dec as Decrypter
Caller->>TMD: decryptTxMetadata(document, keyParam?)
TMD->>Dec: decrypt payload -> bytes
alt bytes[0] == VERSION_CBOR
Note right of TMD `#FFF2CC`: set txMetadataVersion = CBOR
TMD-->>Caller: decode CBOR -> items
else bytes[0] == VERSION_PROTOBUF
Note right of TMD `#FFF2CC`: set txMetadataVersion = PROTOBUF
TMD-->>Caller: parse TxMetadataBatch -> items
else
TMD-->>Caller: throw error (unknown version)
end
sequenceDiagram
autonumber
participant Caller
participant FD as fetch_document.rs
Caller->>FD: DocumentQuery::new(...)
alt Ok(query)
FD-->>Caller: proceed with query.with_where(...)
else Err(e)
Note right of FD `#FDEDEC`: return Err(e.to_string()) instead of panic
FD-->>Caller: Err(String)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Areas to pay extra attention to:
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
55-60: Fix README version to match published artifact.Line 55 advertises
dppVersion = "2.0.1", but the build currently ships as2.0.1-TX-SNAPSHOT(see build.gradle Line 2). Anyone following the README will pull a non-existent artifact and break their build. Please update the snippet (or release a 2.0.1 artifact) so the instructions resolve correctly.
🧹 Nitpick comments (3)
dash-sdk-java/src/test/java/org/dashj/platform/sdk/ValueTest.java (1)
42-47: Remove the commented dead code.The commented
//v2.delete()on line 43 should be removed entirely, as thev2variable no longer exists in this test.Apply this diff to clean up the commented code:
v1.delete(); - //v2.delete(); v3.delete();dpp/src/main/proto/wallet-utils.proto (1)
9-9: Consider fixed-point representation for financial fields.The use of
doubleforexchangeRate(line 9) andoriginalPrice(line 17) can lead to precision issues with financial calculations due to floating-point representation limitations.Consider using one of these alternatives:
- Option 1 (Recommended): Use string representation with a documented format (e.g., "123.45" for currency amounts), allowing the application layer to parse with appropriate decimal precision.
- Option 2: Use fixed-point representation with separate integer fields for the whole and fractional parts, or use an integer representing the smallest currency unit (e.g., cents).
Example for Option 1:
- optional double exchangeRate = 4; + optional string exchangeRate = 4; // decimal string, e.g., "1.2345"- optional double originalPrice = 12; + optional string originalPrice = 12; // decimal string, e.g., "99.99"Also applies to: 17-17
dpp/src/test/kotlin/org/dashj/platform/contracts/wallet/TxMetaDataTests.kt (1)
226-229: Remove the empty test stub.
publishToPlatform()contains no assertions or behavior. Keeping an empty test just adds noise and trips detekt’s empty-block rule. Drop the method or flesh it out with real coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
dash-sdk-android/src/main/rust/Cargo.lockis excluded by!**/*.lockdash-sdk-bindings/Cargo.lockis excluded by!**/*.lockplatform-mobile/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
README.md(1 hunks)build.gradle(1 hunks)dash-sdk-java/build.gradle(1 hunks)dash-sdk-java/src/main/cpp/clone.h(2 hunks)dash-sdk-java/src/main/swig/generics/result.i(0 hunks)dash-sdk-java/src/main/swig/ignore.i(5 hunks)dash-sdk-java/src/test/java/org/dashj/platform/sdk/DataContractTest.java(3 hunks)dash-sdk-java/src/test/java/org/dashj/platform/sdk/ValueTest.java(1 hunks)dpp/src/main/java/org/dashj/platform/dapiclient/SystemIds.kt(1 hunks)dpp/src/main/java/org/dashj/platform/dashpay/BlockchainIdentity.kt(6 hunks)dpp/src/main/java/org/dashj/platform/sdk/platform/Platform.kt(1 hunks)dpp/src/main/java/org/dashj/platform/wallet/IdentityVerify.kt(1 hunks)dpp/src/main/java/org/dashj/platform/wallet/TxMetadata.kt(3 hunks)dpp/src/main/java/org/dashj/platform/wallet/TxMetadataDocument.kt(3 hunks)dpp/src/main/java/org/dashj/platform/wallet/TxMetadataItem.kt(4 hunks)dpp/src/main/proto/wallet-utils.proto(1 hunks)dpp/src/test/kotlin/org/dashj/platform/contracts/wallet/TxMetaDataTests.kt(5 hunks)dpp/src/test/kotlin/org/dashj/platform/contracts/wallet/TxMetadataWalletTest.kt(1 hunks)examples/src/main/kotlin/dashj/org/platform/CreateTxMetadata.kt(2 hunks)examples/src/main/kotlin/dashj/org/platform/CreateTxMetadataTest.kt(1 hunks)examples/src/main/kotlin/dashj/org/platform/DisplayTxMetadata.kt(2 hunks)examples/src/main/kotlin/dashj/org/platform/RegisteredNamesWithBalance.kt(1 hunks)platform-mobile/src/fetch_document.rs(1 hunks)platform-mobile/src/sdk.rs(2 hunks)
💤 Files with no reviewable changes (1)
- dash-sdk-java/src/main/swig/generics/result.i
🧰 Additional context used
🧬 Code graph analysis (1)
dpp/src/main/java/org/dashj/platform/wallet/TxMetadata.kt (2)
dpp/src/main/java/org/dashj/platform/dashpay/Profiles.kt (2)
get(166-168)get(170-185)dpp/src/main/java/org/dashj/platform/sdk/platform/Documents.kt (2)
get(151-167)get(169-187)
🪛 Buf (1.57.2)
dpp/src/main/proto/wallet-utils.proto
3-3: Files with package "org.dashj.platform.wallet" must be within a directory "org/dashj/platform/wallet" relative to root but were in directory "dpp/src/main/proto".
(PACKAGE_DIRECTORY_MATCH)
🪛 detekt (1.23.8)
examples/src/main/kotlin/dashj/org/platform/RegisteredNamesWithBalance.kt
[warning] 74-74: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
dpp/src/test/kotlin/org/dashj/platform/contracts/wallet/TxMetaDataTests.kt
[warning] 227-229: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🔇 Additional comments (20)
platform-mobile/src/fetch_document.rs (1)
106-109: LGTM! Improved error handling.The change from
.unwrap()to explicit error matching prevents potential panics and provides better error propagation, which is consistent with the function's error handling pattern.dash-sdk-java/src/test/java/org/dashj/platform/sdk/ValueTest.java (1)
23-48: LGTM! Redundant test code appropriately removed.The removal of the v2 test code for I16 is appropriate since I16 is already comprehensively tested in
createPlatformValuePrimitiveTest()at line 148. This cleanup reduces code duplication without affecting test coverage.dash-sdk-java/src/test/java/org/dashj/platform/sdk/DataContractTest.java (2)
20-22: LGTM: Improved null-safety.The defensive pattern of checking
isPresent()before callingget()prevents potentialNoSuchElementExceptionif theOptionalis empty.
34-36: LGTM: Improved null-safety.Consistent with the pattern above—checking
isPresent()before callingget()prevents potential runtime exceptions.platform-mobile/src/sdk.rs (2)
225-225: LGTM! Good observability improvement.Adding the SDK version log is consistent with the existing pattern in
create_dash_sdk_with_context(line 173) and improves observability.
62-92: No external FFI bindings referenceupdate_sdk_with_address_list; removing its export attribute is safe.dpp/src/main/java/org/dashj/platform/dapiclient/SystemIds.kt (1)
10-19: LGTM! Wallet-utils identifiers added consistently.The addition of wallet-utils identifiers and the refactoring of existing IDs to use
Sha256Hash.ZERO_HASHfollow a consistent pattern and align with the broader wallet-utils integration across the codebase.dpp/src/main/java/org/dashj/platform/sdk/platform/Platform.kt (1)
80-91: LGTM! App definitions updated to support wallet-utils and identity-verify.The addition of the wallet-utils app and the environment-specific identity-verify app definitions are consistent with the system-wide migration to wallet-utils identifiers introduced in SystemIds.kt.
examples/src/main/kotlin/dashj/org/platform/DisplayTxMetadata.kt (2)
29-38: LGTM! Improved argument handling with default identity support.The refactored argument parsing now supports both interactive phrase input and default identity seeds, which improves the utility's flexibility.
55-55: LGTM! Consistent with updated metadata ordering.The change from
createdAttoupdatedAtaligns with the broader shift in TxMetadata.kt where ordering and filtering now use$updatedAtinstead of$createdAt.dpp/src/main/java/org/dashj/platform/wallet/IdentityVerify.kt (1)
30-30: LGTM! Document constant updated to reflect identity-verify app.The DOCUMENT constant change from "dashwallet.identityVerify" to "identity-verify.identityVerify" is consistent with the Platform.kt updates where the identity-verify app definition replaces dashwallet in environment mappings.
examples/src/main/kotlin/dashj/org/platform/CreateTxMetadata.kt (2)
10-10: LGTM! Import updated to reflect TxMetadataItem package relocation.The import change from
org.dashj.platform.contracts.wallet.TxMetadataItemtoorg.dashj.platform.wallet.TxMetadataItemis consistent with the broader reorganization of wallet-related types into the wallet package.
47-47: LGTM! Updated to use versioned metadata publishing.The addition of the keyIndex parameter (1) and version parameter (TxMetadataDocument.VERSION_PROTOBUF) aligns with the new versioned metadata handling introduced in TxMetadataDocument.kt and the updated publishTxMetaData signature.
dpp/src/main/java/org/dashj/platform/wallet/TxMetadata.kt (4)
69-91: LGTM! New publish method follows established patterns.The new
publish()method correctly retrieves the HIGH security level key, calls the platform SDK with appropriate parameters, and returns a Document. The implementation is consistent with the existingcreate()method pattern.
113-121: LGTM! Version-aware buffer serialization implemented correctly.The
getBuffer()method properly handles both CBOR and Protobuf serialization formats based on the version parameter, with appropriate error handling for invalid versions.
37-37: LGTM! Document constant updated for wallet-utils migration.The DOCUMENT constant change from "dashwallet.tx_metadata" to "wallet-utils.txMetadata" is consistent with the broader migration to wallet-utils identifiers and data contracts.
130-133: ```bash
#!/bin/bashShow get() signature in TxMetadata.kt
rg -n 'fun get' -g 'dpp/src/main/java/org/dashj/platform/wallet/TxMetadata.kt' -C3
Find direct calls to TxMetadata(...).get(
rg -nP 'TxMetadata([^)]).get(' -g '.kt'
Find instantiations of TxMetadata
rg -nP 'val\s+\w+\s*=\sTxMetadata(' -g '.kt'
</blockquote></details> <details> <summary>dpp/src/main/java/org/dashj/platform/wallet/TxMetadataDocument.kt (1)</summary><blockquote> `27-39`: **LGTM! Version tracking added with good encapsulation.** The addition of version constants and the `txMetadataVersion` field with a private setter provides clean version tracking for metadata documents. The field is appropriately initialized to `VERSION_UNKNOWN` and updated during decryption. </blockquote></details> <details> <summary>dpp/src/main/java/org/dashj/platform/wallet/TxMetadataItem.kt (2)</summary><blockquote> `8-8`: **LGTM: Package reorganization and new imports.** The package move to `org.dashj.platform.wallet` and the addition of protobuf/formatting imports appropriately support the new proto conversion features. Also applies to: 11-12, 15-15 --- `281-281`: **Note: toString() now uses locale-dependent formatting.** The toString() method now delegates to `toJson()`, which means numeric fields (exchangeRate, originalPrice) will be displayed with locale-specific currency formatting. This is acceptable for debugging/logging but be aware that the string representation will vary across locales. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| assertEquals(3, dataContract.getDoc_types().size()); | ||
| assertTrue("didn't find txMetadata", dataContract.getDoc_types().stream().allMatch(type -> type.equals("txMetadata"))); |
There was a problem hiding this comment.
Fix the assertion logic.
Line 39 uses allMatch(type -> type.equals("txMetadata")), which checks that every doc_type is named "txMetadata". With 3 doc_types, this assertion would only pass if all three have identical names, which violates the principle that data contract doc types should have distinct names.
Additionally, the error message "didn't find txMetadata" is misleading—it suggests checking for existence, but allMatch verifies that all types are "txMetadata".
Did you intend to:
- Verify that
"txMetadata"exists among the doc_types? UseanyMatch:-assertTrue("didn't find txMetadata", dataContract.getDoc_types().stream().allMatch(type -> type.equals("txMetadata"))); +assertTrue("didn't find txMetadata", dataContract.getDoc_types().stream().anyMatch(type -> type.equals("txMetadata")));
- Or verify specific doc_type names (e.g., three distinct types)? Assert the exact set:
Set<String> expectedTypes = Set.of("txMetadata", "typeA", "typeB"); assertEquals(expectedTypes, new HashSet<>(dataContract.getDoc_types()));
Please clarify the intended assertion and apply the appropriate fix.
🤖 Prompt for AI Agents
In dash-sdk-java/src/test/java/org/dashj/platform/sdk/DataContractTest.java
around lines 38-39, the test uses allMatch which wrongly asserts every doc_type
equals "txMetadata". Replace that with anyMatch to assert existence (and
keep/update the failure message to "didn't find txMetadata"), OR if the intent
is to verify all three specific distinct types, assert the exact set of names by
comparing a Set of expected names to new HashSet<>(dataContract.getDoc_types())
so the test validates the exact distinct types.
dpp/src/test/kotlin/org/dashj/platform/contracts/wallet/TxMetadataWalletTest.kt
Show resolved
Hide resolved
examples/src/main/kotlin/dashj/org/platform/CreateTxMetadataTest.kt
Outdated
Show resolved
Hide resolved
examples/src/main/kotlin/dashj/org/platform/RegisteredNamesWithBalance.kt
Show resolved
Hide resolved
examples/src/main/kotlin/dashj/org/platform/RegisteredNamesWithBalance.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (6)
dpp/src/main/java/org/dashj/platform/wallet/TxMetadataDocument.kt (1)
77-78: Make the error explicit with expected versions.The message is better than before but still omits supported values. Include both constants.
- else -> error("unknown txmetadata version $version") + else -> error("Unknown tx metadata version $version. Expected $VERSION_CBOR (CBOR) or $VERSION_PROTOBUF (Protobuf)")dpp/src/main/java/org/dashj/platform/wallet/TxMetadataItem.kt (5)
80-97: Proto defaults vs presence — avoid sentinel checks.Using
!= 0/""to infer presence is brittle (floats, legit zeros, future defaults). PreferhasXxx()or wrapper/optional fields in the proto.Example:
constructor(p: WalletUtils.TxMetadataItem) : this( p.txId.toByteArray(), if (p.hasTimestamp()) p.timestamp else null, if (p.hasMemo()) p.memo else null, if (p.hasExchangeRate()) p.exchangeRate else null, // ... if (p.otherDataCount > 0) p.otherDataMap else null )If fields aren’t optional, consider switching to wrappers (e.g., google.protobuf.*Value).
103-106: Fix for timestamp map assignment looks good.
map["timestamp"] = itresolves the prior critical bug (usingto).
166-214: toJson should emit raw numbers and avoid locale-dependent formatting.Formatting with
DecimalFormat.getCurrencyInstance()produces locale-specific strings; JSON consumers typically expect numbers.- fun toJson(): Map<String, Any?> { - val map = hashMapOf<String, String?>( + fun toJson(): Map<String, Any?> { + val map = hashMapOf<String, Any?>( "txId" to Sha256Hash.wrap(txId).toString() ) - timestamp?.let { map["timestamp"] = it.toString() } + timestamp?.let { map["timestamp"] = it } memo?.let { map["memo"] = it } - exchangeRate?.let { - val format = DecimalFormat.getCurrencyInstance() - map["exchangeRate"] = format.format(it) - } + exchangeRate?.let { map["exchangeRate"] = it } currencyCode?.let { map["currencyCode"] = it } // ... - originalPrice?.let { - val format = DecimalFormat.getCurrencyInstance() - map["originalPrice"] = format.format(it) - } + originalPrice?.let { map["originalPrice"] = it }If display formatting is needed, provide a separate
toDisplayMap(Locale)helper.
263-269: hashCode byte order reversal — confirm intent or restore for stability.Using bytes [3,2,1,0] breaks compatibility with prior [0,1,2,3] ordering (affects HashMap/HashSet behavior).
If unintended, revert:
- return Ints.fromBytes( - txId[3], - txId[2], - txId[1], - txId[0] - ) + return Ints.fromBytes(txId[0], txId[1], txId[2], txId[3])If intentional, document the breaking change and add migration notes/tests.
272-292: customIconUrl mapping fixed.Correct field (
builder.customIconUrl) is now used; previous copy-paste bug resolved.
🧹 Nitpick comments (10)
examples/src/main/kotlin/dashj/org/platform/PlatformExplorer.kt (1)
556-571: Pagination logic is correct, but consider reducing duplication.The pagination implementation correctly fetches additional pages until fewer than 100 items are returned. However, the API call and printing logic are duplicated between the initial fetch and the loop body.
Consider refactoring to a do-while loop to eliminate duplication:
- var contestedResources = result.unwrap() - var list = contestedResources._0 - println("${list.size} contested resources: ") - for (item in list) { - println(item.value.text) - } - - while (list.size == 100) { - val result2 = dashsdk.platformMobileVotingGetContestedResources( - sdk, - "domain", - Identifier(dpnsContractId), - 100, - contestedResources._0.lastOrNull()?._0, - false - ) - contestedResources = result2.unwrap() - list = contestedResources._0 + var contestedResources = result.unwrap() + var startAfter: Any? = null + do { + val pageResult = dashsdk.platformMobileVotingGetContestedResources( + sdk, + "domain", + Identifier(dpnsContractId), + 100, + startAfter, + false + ) + contestedResources = pageResult.unwrap() + val list = contestedResources._0 println("${list.size} contested resources: ") for (item in list) { println(item.value.text) } - } + startAfter = list.lastOrNull()?._0 + } while (list.size == 100)examples/src/main/kotlin/dashj/org/platform/DisplayIdentityKeys.kt (1)
45-49: Consider adding a security warning comment.The additional output provides useful debugging information for this example program. However, the code prints sensitive data including private keys (lines 45-46) and addresses (line 49). While this is expected for a utility explicitly designed to display identity keys, consider adding a comment warning that this code should never be used in production environments.
Minor: Fix spacing in line 46.
Add a space after the comma for consistency:
- println("privateKey: ${DumpedPrivateKey(client.params,firstKey.privKeyBytes, true)}") + println("privateKey: ${DumpedPrivateKey(client.params, firstKey.privKeyBytes, true)}")examples/src/main/kotlin/dashj/org/platform/CreateTxMetadataTest.kt (2)
169-173: Consider removing commented code.The commented test block for
txMetadataItemFourshould either be uncommented if needed or removed to maintain code cleanliness.
193-193: Clarify magic number check.The condition
encryptedMetadata[0] != 0.toByte()uses a magic number without explanation. Consider extracting this into a named constant or adding a comment explaining what the first byte represents (e.g., encryption status flag).dpp/src/main/java/org/dashj/platform/wallet/TxMetadataDocument.kt (2)
58-66: Guard against short/invalid plaintext and add defensive parse handling.Accessing
decryptedData[0]will throw if the plaintext is too short. Add a minimal-length check and fail fast with a clear message.val decryptedData = cipher.decrypt(EncryptedData(iv, encryptedData), keyParameter) - val version = decryptedData.copyOfRange(0, 1)[0].toInt() and 0xFF + require(decryptedData.isNotEmpty()) { "Decrypted metadata is empty (no version byte present)" } + val version = (decryptedData[0].toInt() and 0xFF)
59-63: Consider adding integrity protection (AEAD/HMAC).AES-CBC provides confidentiality but not integrity. A wrong key or tampering can yield garbage that passes to decoders. If possible, switch to an AEAD mode (e.g., AES-GCM) or include an HMAC over version+payload and verify before parsing.
Would you like a follow-up PR sketching an authenticated format?
dpp/src/main/java/org/dashj/platform/wallet/TxMetadataItem.kt (1)
228-231: Don’t flatten otherData into top-level JSON.Merging
otherDatainto the root risks key collisions (e.g., “memo”) and diverges fromtoObject()which nests it.- otherData?.let { - map.putAll(it) - } + otherData?.let { + map["otherData"] = it + } ``` <!-- review_comment_end --> </blockquote></details> <details> <summary>dpp/src/test/kotlin/org/dashj/platform/dashpay/BlockchainIdentityTest.kt (3)</summary><blockquote> `155-157`: **Avoid magic slack in size check.** `+ 100` is arbitrary. Either compute exact overhead (IV + padding + version byte) or assert `<= MAX_ENCRYPTED_SIZE` on the plaintext before encryption. Example: ```kotlin // Check serialized payload length before encrypt assertTrue(serialized.size <= TxMetadataDocument.MAX_ENCRYPTED_SIZE, "Payload too large")Or derive a constant for IV(16) + max padding(16) + version(1).
98-103: Assert detected version on decrypt.Strengthen the round-trip by asserting the parsed version.
- val decryptedItems = blockchainIdentity.decryptTxMetadata(txMetadataDocument, null) + val decryptedItems = blockchainIdentity.decryptTxMetadata(txMetadataDocument, null) + assertEquals(TxMetadataDocument.VERSION_PROTOBUF, txMetadataDocument.txMetadataVersion)
60-108: Consider adding a CBOR round-trip test.Add a mirror of this test using
TxMetadataDocument.VERSION_CBORto validate both paths.I can draft the CBOR variant if helpful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
dash-sdk-android/src/main/rust/Cargo.lockis excluded by!**/*.lockdash-sdk-bindings/Cargo.lockis excluded by!**/*.lockplatform-mobile/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
dash-sdk-java/src/test/java/org/dashj/platform/sdk/DataContractTest.java(2 hunks)dpp/src/main/java/org/dashj/platform/wallet/TxMetadataDocument.kt(3 hunks)dpp/src/main/java/org/dashj/platform/wallet/TxMetadataItem.kt(6 hunks)dpp/src/test/kotlin/org/dashj/platform/dashpay/BlockchainIdentityTest.kt(2 hunks)examples/src/main/kotlin/dashj/org/platform/CreateTxMetadataTest.kt(1 hunks)examples/src/main/kotlin/dashj/org/platform/DisplayIdentityKeys.kt(2 hunks)examples/src/main/kotlin/dashj/org/platform/PlatformExplorer.kt(1 hunks)examples/src/main/kotlin/dashj/org/platform/RegisteredNamesWithBalance.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- dash-sdk-java/src/test/java/org/dashj/platform/sdk/DataContractTest.java
- examples/src/main/kotlin/dashj/org/platform/RegisteredNamesWithBalance.kt
🔇 Additional comments (3)
examples/src/main/kotlin/dashj/org/platform/PlatformExplorer.kt (1)
549-550: LGTM! Necessary mutability for pagination.The change from
valtovaris correct and required for reassignment within the pagination loop.examples/src/main/kotlin/dashj/org/platform/DisplayIdentityKeys.kt (1)
9-10: LGTM!The new imports are used appropriately in the enhanced output statements.
examples/src/main/kotlin/dashj/org/platform/CreateTxMetadataTest.kt (1)
142-144: Off-by-one issue resolved.The previous off-by-one error has been correctly fixed by replacing
IntRange(0, count)withrepeat(count), ensuring exactlycountitems are added.
| // 2^16 + 2 | ||
| val childNumber = ChildNumber(2 shl 15 + 1, true) | ||
| const val MAX_ENCRYPTED_SIZE = 4096 - 32 // leave room for a partially filled block and the IV | ||
|
|
There was a problem hiding this comment.
Operator precedence bug in child number expression.
2 shl 15 + 1 is parsed as 2 shl (15 + 1) yielding 131072, not “2^16 + 2” as the comment states. Make the intent explicit.
Apply:
- // 2^16 + 2
- val childNumber = ChildNumber(2 shl 15 + 1, true)
+ // 2^16 + 2 = 65538
+ val childNumber = ChildNumber((1 shl 16) + 2, true)Also fix the same expression in tests (BlockchainIdentityTest.kt Lines 172–173).
🤖 Prompt for AI Agents
In dpp/src/main/java/org/dashj/platform/wallet/TxMetadataDocument.kt around
lines 23–26, the expression `2 shl 15 + 1` has wrong operator precedence and
does not match the comment "2^16 + 2"; replace it with an explicit calculation
such as `(1 shl 16) + 2` (or a literal 65538) to make the intent unambiguous,
and apply the same fix to the identical expression in tests at
BlockchainIdentityTest.kt lines 172–173.
dpp/src/test/kotlin/org/dashj/platform/dashpay/BlockchainIdentityTest.kt
Show resolved
Hide resolved
examples/src/main/kotlin/dashj/org/platform/CreateTxMetadataTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
dpp/src/main/java/org/dashj/platform/wallet/TxMetadataDocument.kt (1)
23-24: CRITICAL: Operator precedence bug still not fixed.This issue was flagged in previous reviews but remains unaddressed. The expression
2 shl 15 + 1evaluates to2 shl (15 + 1)= 131072, not 65538 (2^16 + 2) as the comment states.Apply this diff:
- // 2^16 + 2 - val childNumber = ChildNumber(2 shl 15 + 1, true) + // 2^16 + 2 = 65538 + val childNumber = ChildNumber((1 shl 16) + 2, true)Also fix the same bug in
BlockchainIdentityTest.kt(lines 172-173) as noted in previous reviews.
🧹 Nitpick comments (1)
examples/src/main/kotlin/dashj/org/platform/CreateTxMetadata.kt (1)
54-59: Consider logging when skipping documents with empty metadata.While the
isNotEmpty()check prevents an index-out-of-bounds exception, silently skipping documents with empty or null metadata could hide data integrity issues. Consider adding a log message to aid debugging.Apply this diff to improve observability:
if (txDoc.encryptedMetadata.isNotEmpty() && txDoc.encryptedMetadata[0] != 0.toByte()) { println(JSONObject(doc.toJSON()).toString(2)) val txList = blockchainIdentity.decryptTxMetadata(txDoc, null) println(" $txList") + } else { + println("Skipping document ${txDoc.document.id} with empty or zero metadata") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dpp/src/main/java/org/dashj/platform/wallet/TxMetadataDocument.kt(3 hunks)examples/src/main/kotlin/dashj/org/platform/CreateTxMetadata.kt(2 hunks)examples/src/main/kotlin/dashj/org/platform/CreateTxMetadataTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/src/main/kotlin/dashj/org/platform/CreateTxMetadataTest.kt
🔇 Additional comments (6)
examples/src/main/kotlin/dashj/org/platform/CreateTxMetadata.kt (2)
10-10: LGTM: Import path updated to match package reorganization.The import path correctly reflects the package move from
contracts.wallettowallet.
47-47: LGTM: API call updated to support versioned metadata.The additional parameters for batch number and version type align with the new versioned TxMetadata implementation.
dpp/src/main/java/org/dashj/platform/wallet/TxMetadataDocument.kt (4)
17-18: LGTM: Imports updated for versioned metadata support.The imports correctly reference the relocated
TxMetadataItemand the new protobuf-basedTxMetadataBatch.
27-29: LGTM: Version constants are well-defined.The constants provide clear versioning support for CBOR and Protobuf formats with an appropriate sentinel value for unknown versions.
38-39: LGTM: Proper encapsulation of version tracking.The property uses a private setter and sensible default, ensuring version state is only modified internally during decryption.
64-78: LGTM: Version-aware decryption correctly implemented.The decrypt logic properly handles both CBOR and Protobuf formats:
- Correctly extracts and masks the version byte
- Both branches now correctly skip the version byte when parsing (previous critical bug fixed)
- Error message is now informative (previous issue addressed)
- Consistent mapping to
TxMetadataItemin both branchesThe previous critical issues flagged in earlier reviews have been successfully resolved.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
dpp/src/test/kotlin/org/dashj/platform/dashpay/BlockchainIdentityTest.kt (1)
172-179: Use TxMetadataDocument.childNumber constant instead of duplicating calculation.Lines 172-179 calculate the same encryption key twice using different expressions for verification. This duplicates the derivation logic and reduces maintainability.
Apply this diff to use the existing constant:
val expectedEncryptionKey = parentEncryptionKey - .deriveChildKey(ChildNumber((2 shl 14) + 1, true)) + .deriveChildKey(TxMetadataDocument.childNumber) .derive(1) assertEquals(expectedEncryptionKey, actualEncryptionKey) - val expectedEncryptionKey2 = parentEncryptionKey - .deriveChildKey(ChildNumber(2.toDouble().pow(15).toInt() + 1, true)) - .derive(1) - assertEquals(expectedEncryptionKey2, actualEncryptionKey)Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
build.gradle(1 hunks)dpp/src/main/java/org/dashj/platform/wallet/TxMetadataDocument.kt(3 hunks)dpp/src/test/kotlin/org/dashj/platform/dashpay/BlockchainIdentityTest.kt(2 hunks)dpp/src/test/kotlin/org/dashj/platform/dashpay/PlatformNetwork.kt(1 hunks)
🔇 Additional comments (8)
build.gradle (1)
5-5: LGTM! Dependency version updated.The
dashj_versionbump from '21.1.7' to '21.1.9' aligns with the PR's goal to support tx metadata changes.dpp/src/test/kotlin/org/dashj/platform/dashpay/PlatformNetwork.kt (1)
26-26: LGTM! Test seed updated.The deterministic seed change is appropriate for test scenarios and aligns with the broader TxMetadata wallet integration testing.
dpp/src/main/java/org/dashj/platform/wallet/TxMetadataDocument.kt (3)
23-24: LGTM! Derivation path calculation fixed.The childNumber expression
(2 shl 14) + 1correctly evaluates to 32769 (2^15 + 1) with proper operator precedence, addressing the past review concern.
27-29: LGTM! Version constants added.The version constants provide clear semantic meaning for the supported metadata formats (CBOR and Protobuf).
64-78: LGTM! Versioned decryption logic implemented correctly.The decrypt method now:
- Reads the version byte from offset 0
- Correctly skips the version byte when parsing both CBOR (line 67:
copyOfRange(1, ...)) and Protobuf (line 73:parseFrom(..., 1, ...)) payloads- Sets
txMetadataVersionappropriately- Provides an informative error message for unknown versions
This addresses all past review concerns about version byte handling and offset bugs.
dpp/src/test/kotlin/org/dashj/platform/dashpay/BlockchainIdentityTest.kt (3)
62-108: LGTM! Basic TxMetadata test coverage.The test correctly validates:
- Document creation with two metadata items
- Document structure (type, required fields)
- Round-trip encryption/decryption
- Item equality after decryption
110-122: LGTM! Edge case coverage for empty list.Testing empty list behavior ensures the API handles this gracefully without creating unnecessary documents.
125-197: LGTM! Comprehensive large-data test.The test thoroughly validates:
- Multi-document splitting for large datasets
- Document size constraints (MAX_ENCRYPTED_SIZE)
- Key derivation path correctness
- Complete round-trip reconstruction of 50 items
| // @Test | ||
| // fun sendContactRequestTest() { | ||
| // | ||
| // blockchainIdentity.recoverIdentity(authenticationGroupExtension.identityKeyChain.getKey(0, true).pubKeyHash) | ||
| // | ||
| // val cr = ContactRequests(platform) | ||
| // | ||
| // // my-test-1 | ||
| // val toUserIdentity = platform.identities.get("6ffk46v6m79RPbiomNT1kLXX7unqgw5FkWhsTuPi4Aj7") | ||
| // | ||
| // val document = cr.create(blockchainIdentity, toUserIdentity!!, null) | ||
| // | ||
| // println(document) | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove or uncomment the dead test.
The sendContactRequestTest is commented out. Either remove it if obsolete or uncomment and update it if still needed.
Apply this diff to remove:
-// @Test
-// fun sendContactRequestTest() {
-//
-// blockchainIdentity.recoverIdentity(authenticationGroupExtension.identityKeyChain.getKey(0, true).pubKeyHash)
-//
-// val cr = ContactRequests(platform)
-//
-// // my-test-1
-// val toUserIdentity = platform.identities.get("6ffk46v6m79RPbiomNT1kLXX7unqgw5FkWhsTuPi4Aj7")
-//
-// val document = cr.create(blockchainIdentity, toUserIdentity!!, null)
-//
-// println(document)
-// }📝 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.
| // @Test | |
| // fun sendContactRequestTest() { | |
| // | |
| // blockchainIdentity.recoverIdentity(authenticationGroupExtension.identityKeyChain.getKey(0, true).pubKeyHash) | |
| // | |
| // val cr = ContactRequests(platform) | |
| // | |
| // // my-test-1 | |
| // val toUserIdentity = platform.identities.get("6ffk46v6m79RPbiomNT1kLXX7unqgw5FkWhsTuPi4Aj7") | |
| // | |
| // val document = cr.create(blockchainIdentity, toUserIdentity!!, null) | |
| // | |
| // println(document) | |
| // } |
🤖 Prompt for AI Agents
dpp/src/test/kotlin/org/dashj/platform/dashpay/BlockchainIdentityTest.kt lines
46-59: the sendContactRequestTest is commented out; either remove the entire
commented block if it's obsolete, or uncomment and convert it into a real
working test by adding the @Test annotation, restoring any required setup calls
(e.g. ensure blockchainIdentity is recovered with the correct pubKeyHash),
obtaining a valid toUserIdentity from platform.identities (with null checks or
test fixtures), and updating/asserting expected outcomes instead of printing —
apply one of these two options so no dead/commented test remains.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
build.gradle(1 hunks)dpp/src/main/java/org/dashj/platform/dashpay/BlockchainIdentity.kt(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- build.gradle
🧰 Additional context used
🪛 detekt (1.23.8)
dpp/src/main/java/org/dashj/platform/dashpay/BlockchainIdentity.kt
[warning] 2379-2379: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
dpp/src/main/java/org/dashj/platform/dashpay/BlockchainIdentity.kt (1)
2279-2339: Respect the requested tx-metadata version when encoding.This is the same critical issue flagged in the previous review. Line 2319 always serializes items with
TxMetadataDocument.VERSION_PROTOBUF, but the method advertises support for a caller-suppliedversionparameter and writes that value into the payload header at line 2326. If a caller passesVERSION_CBOR, the code emits protobuf bytes while tagging them as CBOR. WhendecryptTxMetadatareads the header, it will pick the CBOR deserialization path and fail parsing protobuf data, breaking backward compatibility.Apply this diff to branch on
versionwhen buildingmetadataBytesso the encoded format matches the header:log.info("publishing ${currentMetadataItems.size} items of ${txMetadataItems.size}") val txMetadata = TxMetadata(platform) - val metadataBytes = txMetadata.getBuffer(TxMetadataDocument.VERSION_PROTOBUF, currentMetadataItems)//Cbor.encode(currentMetadataItems.map { it.toObject() }) + val metadataBytes = when (version) { + TxMetadataDocument.VERSION_CBOR -> + Cbor.encode(currentMetadataItems.map { it.toObject() }) + TxMetadataDocument.VERSION_PROTOBUF -> + txMetadata.getBuffer(version, currentMetadataItems) + else -> error("invalid tx metadata version $version") + }
🧹 Nitpick comments (2)
dpp/src/main/java/org/dashj/platform/dashpay/BlockchainIdentity.kt (2)
2377-2381: Consider logging the exception details for debugging.The try-catch block swallows decryption failures with only a basic log message. While this provides graceful degradation when encountering corrupted documents, it may hide legitimate issues like wrong encryption keys, version mismatches, or bugs in the decryption logic.
Apply this diff to include exception details in the log:
try { results[doc] = decryptTxMetadata(TxMetadataDocument(it), keyParameter) } catch (e: Exception) { - log.info("txMetadata document could not be decrypted: {}", doc.id) + log.info("txMetadata document could not be decrypted: {} - {}", doc.id, e.message, e) }
2388-2388: Improve the precondition error message for clarity.The precondition check correctly ensures encryption state consistency, but the error message could be more descriptive to help developers understand what went wrong.
Apply this diff to improve the error message:
- checkArgument(wallet!!.isEncrypted == (keyParameter != null), "isEncrypted must match keyParameter") + checkArgument( + wallet!!.isEncrypted == (keyParameter != null), + "Wallet encryption state mismatch: wallet.isEncrypted=${wallet!!.isEncrypted} but keyParameter is ${if (keyParameter != null) "provided" else "null"}" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
dash-sdk-android/src/main/rust/Cargo.lockis excluded by!**/*.lockdash-sdk-bindings/Cargo.lockis excluded by!**/*.lockplatform-mobile/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
build.gradle(1 hunks)dpp/src/main/java/org/dashj/platform/dashpay/BlockchainIdentity.kt(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- build.gradle
🧰 Additional context used
🪛 detekt (1.23.8)
dpp/src/main/java/org/dashj/platform/dashpay/BlockchainIdentity.kt
[warning] 2379-2379: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (3)
dpp/src/main/java/org/dashj/platform/dashpay/BlockchainIdentity.kt (3)
52-52: LGTM!The updated imports correctly reflect the migration to the new tx metadata structure with protobuf support.
Also applies to: 70-70
2341-2366: LGTM with dependency oncreateTxMetadatafix.The method correctly passes the
versionparameter tocreateTxMetadataand provides reasonable progress tracking. However, it will inherit the version mismatch issue fromcreateTxMetadatauntil that method is fixed to respect the version parameter.
2408-2418: LGTM!The version dispatching logic correctly handles both CBOR (legacy) and protobuf formats, with appropriate error handling for invalid versions. This enables backward compatibility while supporting the new protobuf-based encoding.
Updates to support tx metadata changes
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores
Tests