Conversation
|
|
||
| UnsignedByteArray byteArray = new CurrencyType().fromJson(issue.currency()).value(); | ||
| if (issue.mptIssuanceId().isPresent()) { | ||
| // MPTokenIssuanceID binary format: issuer (20 bytes) + ACCOUNT_ONE (20 bytes) + sequence (4 bytes) = 44 bytes |
There was a problem hiding this comment.
Remind me why rippled adds an additional ACCOUNT_ONE into the binary encoding? I feel like there was a good reason, but I can't remember it.
Also, we should update the comment description to properly describe the two different formats. Currently, the only descriptor is binary vs hex, which is confusing (generally binary and hex should be the same, but in this case they're not, which means they're describing two different values)
There was a problem hiding this comment.
why rippled adds an additional ACCOUNT_ONE into the binary encoding?
IOU - currency (20 bytes) || issuer (20 bytes)
MPT - issuer (20 bytes) || ACCOUNT_ONE (20 bytes) || sequence (4 bytes)
I think ACCOUNT_ONE acts as a discriminator between IOU and MPT for deserializer. Reading first 20 bytes helps us to distinguish between XRP (all zeros) and IOU/MPT, but it won't be possible to distinguish between IOU and MPT if ACCOUNT_ONE is not present.
| // After rounding, mantissa may exceed MAX_INT64 again | ||
| if (absMantissa.compareTo(MAX_INT64) > 0) { | ||
| if (exponent >= MAX_EXPONENT) { | ||
| throw new IllegalArgumentException("Exponent overflow: value too large to represent"); |
There was a problem hiding this comment.
This line seems uncovered by the unit test.
| lastDigit = absMantissa.mod(BigInteger.TEN); | ||
| absMantissa = absMantissa.divide(BigInteger.TEN); | ||
| exponent++; | ||
| if (lastDigit.compareTo(BigInteger.valueOf(5)) >= 0) { |
There was a problem hiding this comment.
This line seems uncovered by the unit test.
| * @return An optionally present {@link JsonNode}. | ||
| */ | ||
| @JsonProperty("mpt_issuance_id") | ||
| Optional<JsonNode> mptIssuanceId(); |
There was a problem hiding this comment.
This is technically a breaking change (though I doubt any external software is using this object, but you never know).
Can we capture somewhere a list of all breaking changes in this PR? We'll want to use that to draft release notes. We should probably have (as part of this PR) something that outlines info that will eventually make its way into a migration guide, sort of like this one.
There was a problem hiding this comment.
I think since mptIssuanceId is Optional, it won't be a breaking change. Any user using Issue object present in binary.types package would still be able to create Issue object the way they have been creating it, ever after the upgrade. Did I understand it incorrectly?
There is a breaking change in Issue interface present in model.ledger package this PR, and I am planning to document it.
| transactionWithSignature = PermissionedDomainDelete.builder().from((PermissionedDomainDelete) transaction) | ||
| .transactionSignature(signature) | ||
| .build(); | ||
| } else if (VaultCreate.class.isAssignableFrom(transaction.getClass())) { |
|
|
||
| @Override | ||
| public VaultData deserialize(JsonParser jsonParser, DeserializationContext ctxt) throws IOException { | ||
| return VaultData.of(jsonParser.getText()); |
There was a problem hiding this comment.
I'm not sure VaultData should be a String, but maybe it should? It's going to be binary data, so if it's a String it will be Hex, right?
(maybe this is fine, but I'm trying to think if VaultData should actually just hold an array (e.g., List of UnsignedByte).
That said, perhaps there's prior art in xrpl4j for this kind of thing?
There was a problem hiding this comment.
VaultData is a wrapper type containing String and it has checks to make sure that the content is valid Hex.
I followed the convention that we have in xrpl4j for such metadata (MpTokenMetadata, DidData etc).
We could make this type wrap List<UnsignedByte> or UnsignedByteArray and update the serializer and deserialize to do bytes <-> String (Hex) conversion, but I think doing that won't make much difference as we will anyway provide static convenience methods like fromHex for users to create an instance of VaultData from off-chain binary data that they would have stored in Hex format.
| * @return An optionally-present {@link AssetScale}. | ||
| */ | ||
| @JsonProperty("Scale") | ||
| Optional<AssetScale> scale(); |
There was a problem hiding this comment.
In the spec, scale and most of the other values have are required (but default to 0).
Looks like only data and assetsMaximum are actually optional.
See https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0065-single-asset-vault#212-fields
There was a problem hiding this comment.
Thanks for highlighting this. I was marking all thesoeDEFAULT fields as optional just to be on safer side. But now have updated them to default to zero value of their type. From ledger_entries.macro only
Data is soeOPTIONAL other fields are either soeREQUIRED or soeDEFAULT.
Addressed in 5256ce8
| }, | ||
| iouIssue -> { | ||
| long scaleVal = scaleValue.value().longValue(); | ||
| if (scaleVal < 0 || scaleVal > 18) { |
There was a problem hiding this comment.
I think this line needs some more coverage.
There was a problem hiding this comment.
AssetScale wraps UnsignedInteger, so I have updated the check accordingly and now this line has full coverage.
Addressed in 5256ce8
| .issuer(Address.of("rPZtDb6ZHtfYzMmzyUGvehoi2vYhrNAPhy")) | ||
| .build() | ||
| ) | ||
| .asset2(Issue.builder() |
There was a problem hiding this comment.
This is a breaking change - see my other comment about how we should document breakage?
| * ========================LICENSE_START================================= | ||
| * xrpl4j :: core | ||
| * %% | ||
| * Copyright (C) 2020 - 2023 XRPL Foundation and its contributors |
There was a problem hiding this comment.
Minor nit - If you're adding new files and adding a license, can you make the date correct, like this:
| * Copyright (C) 2020 - 2023 XRPL Foundation and its contributors | |
| * Copyright (C) 2020 - 2026 XRPL Foundation and its contributors |
(Alternatively, don't add this license and it will get taken care of, eventually, by #669)
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #713 +/- ##
============================================
+ Coverage 92.38% 92.70% +0.32%
- Complexity 2115 2259 +144
============================================
Files 437 459 +22
Lines 5594 5910 +316
Branches 430 479 +49
============================================
+ Hits 5168 5479 +311
Misses 279 279
- Partials 147 152 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // xrpld implementation: src/libxrpl/basics/Number.cpp -> doNormalize | ||
| @Override | ||
| public NumberType fromJson(JsonNode node) { | ||
| String value = node.isInt() || node.isLong() ? String.valueOf(node.asLong()) : node.asText(); |
There was a problem hiding this comment.
Can you add a test case for when node.isInt() returns true and node.isLong() returns true? Looks like just the node.asText() true case is tested
There was a problem hiding this comment.
I think instead I should remove these checks as the type would always be string since AssetAmount wraps String type.
| @Override | ||
| public Issue deserialize(JsonParser jsonParser, DeserializationContext ctxt) throws IOException { | ||
| JsonNode node = jsonParser.getCodec().readTree(jsonParser); | ||
|
|
There was a problem hiding this comment.
Would there be an instance where mpt_issuance_id and currency are present? If so, maybe include something like
boolean hasMpt = node.has("mpt_issuance_id");
boolean hasCurrency = node.has("currency");
// Validate mutual exclusivity
if (hasMpt && hasCurrency) {
throw new IOException(
"Invalid Issue JSON: cannot have both 'mpt_issuance_id' and 'currency' fields"
);
}
There was a problem hiding this comment.
This codepath would get exercised when parsing the transaction/ledger object from rippled. The object returned would never have both mpt_issuance_id and currency present at the same time.
xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/SingleAssetVaultIT.java
Fixed
Show fixed
Hide fixed
xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/SingleAssetVaultIT.java
Fixed
Show fixed
Hide fixed
xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/SingleAssetVaultIT.java
Fixed
Show fixed
Hide fixed
xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/SingleAssetVaultIT.java
Fixed
Show fixed
Hide fixed
xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/SingleAssetVaultIT.java
Fixed
Show fixed
Hide fixed
xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/SingleAssetVaultIT.java
Fixed
Show fixed
Hide fixed
xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/SingleAssetVaultIT.java
Fixed
Show fixed
Hide fixed
xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/SingleAssetVaultIT.java
Fixed
Show fixed
Hide fixed
xrpl4j-integration-tests/src/test/java/org/xrpl/xrpl4j/tests/SingleAssetVaultIT.java
Fixed
Show fixed
Hide fixed
|
/ai-review |
1 similar comment
|
/ai-review |
There was a problem hiding this comment.
ℹ️ Note: This is a large diff (403,683 chars). Complex issues deep in the diff may receive less attention.
Four issues flagged inline: a typo in IssueType.java, an NPE risk in IssueDeserializer, a validation gap in VaultInfoRequestParams.check() that produces misleading errors, and missing codec fixtures for five of the six new vault transaction types.
Review by Claude Opus 4.6 · Prompt: V12
Single Asset Vault - XLS-65
Single Asset Vault had multiple PRs, so rippled/xrpld release 3.1.0 was considered as source of truth for Transactions, ledger objects, RPC and binary codec changes.
Transactions added/updated:
Ledger objects added/updated:
RPC added/updated:
Binary codec added/updated:
MPT DEX PR and this PR both have IssueType binary codec change and Issue object refactoring change in common. Since, Single Asset Vault is up for voting, this PR would likely get merged first. So, I have kept these suggestions in mind when making changes: