Add support for Lending Protocol - XLS-66#719
Conversation
…rp/lending-protocol
|
General note: There are multiple places where |
There was a problem hiding this comment.
Create a constant for 79E25403E9FC010A277D80410EED5494FDD033A09FD4C1432335A1734A1D099D at the beginning of the file since this value is used multiple times here
There was a problem hiding this comment.
I think moving this to a class level constant would be appropriate if these were unit tests and had specific meaning across the tests. However, these are JSON tests and the value has no particular meaning. I think it's better to leave it here.
There was a problem hiding this comment.
Consider creating constants for ED1F863E4E0957C6965B7B0563D56C77E4F68D571E8026251834F0ADBB0411D6FD and 79E25403E9FC010A277D80410EED5494FDD033A09FD4C1432335A1734A1D099D
There was a problem hiding this comment.
Looks like these values appear in other files as well. Maybe create this constant in some place where multiple files can easily reuse this value?
There was a problem hiding this comment.
These have no particular significance and are readable in the current form along side the JSON string.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| flagCount <= 1, | ||
| "Only one of tfLoanDefault, tfLoanImpair, or tfLoanUnimpair may be set." |
There was a problem hiding this comment.
Should this be flagCount == 1? The string on line 67 implies that a flagCount of 0 is not expected
There was a problem hiding this comment.
Let's make sure we have unit test coverage for whatever the final decision is.
There was a problem hiding this comment.
See the comment in LoanPay. It applies here as well.
| flagCount <= 1, | ||
| "Only one of tfLoanLatePayment, tfLoanFullPayment, or tfLoanOverpayment may be set." |
There was a problem hiding this comment.
Same issue: should this be flagCount==1? Either this check should be updated or the string on line 80 should be updated to account for a possible flagCount of 0
There was a problem hiding this comment.
Technically the transaction passes at rippled even if none of the flags are set. So I have updated the error message to depict the same in 0f523c5. Also, there are unit tests present to test this.
| * @return An optionally-present {@link Amount}. | ||
| */ | ||
| @JsonProperty("DebtTotal") | ||
| Optional<Amount> debtTotal(); |
There was a problem hiding this comment.
In the spec, the only two truly optional fields are ManagementFeeRate and Data. All other fields, including this field, are marked as required=yes with a default value of 0. So, it seems like we should change all of the non-optional fields to be like this:
@JsonProperty("DebtMaximum")
default Amount debtTotal(){
return Amount.ZERO;
}The one exclusion to this (that I can see in the spec) is DebtMaximum, which has the comment "The default value of 0 means there is no limit to the debt."
Because of that one comment, DebtMaximum seems like the only truly Optional field, and should be left alone.
Thoughts?
There was a problem hiding this comment.
LoanBroker object has either soeDEFAULT or soeREQUIRED fields, as per ledger_entries.macro. So, I defaulted to Zero value for most of the soeDEFAULT fields.
Even though Data as marked as soeDEFAULT, it makes sense to keep it optional as it doesn't make sense to have return an empty string as Metadata if present cannot be empty.
I agree to keep DebtMaximum Optional in Java for the reason you mentioned.
Addressed 0f523c5
| * @return An optionally-present {@link Amount}. | ||
| */ | ||
| @JsonProperty("PrincipalOutstanding") | ||
| Optional<Amount> principalOutstanding(); |
There was a problem hiding this comment.
In the spec, most of these fields (except LoanScale) are non-optional. Seems like these should just be default values with Amount.ZERO as the default.
Thoughts?
There was a problem hiding this comment.
Loan object has either soeDEFAULT or soeREQUIRED fields, as per ledger_entries.macro. So I have given default zero value for each type accordingly.
LoanScale is also soeDEFAULT, so I have defaulted it's value to 0. Does this sound right for LoanScale?
| * @return An optionally-present {@link String}. | ||
| */ | ||
| @JsonProperty("SigningPubKey") | ||
| Optional<String> signingPubKey(); |
There was a problem hiding this comment.
I think we should use PublicKey here, like we do in BatchSigner. That way, developers are guided around how exactly to assemble bytes (no String), and serde does all the work.
Thoughts?
There was a problem hiding this comment.
Yes makes sense to use PublicKey and Signature
0f523c5
| * @return An optionally-present {@link String}. | ||
| */ | ||
| @JsonProperty("TxnSignature") | ||
| Optional<String> txnSignature(); |
There was a problem hiding this comment.
Can we use Signature here?
| flagCount <= 1, | ||
| "Only one of tfLoanDefault, tfLoanImpair, or tfLoanUnimpair may be set." |
There was a problem hiding this comment.
Let's make sure we have unit test coverage for whatever the final decision is.
| @JsonDeserialize(as = VaultData.class, using = VaultDataDeserializer.class) | ||
| @Beta | ||
| abstract static class _VaultData extends Wrapper<String> implements Serializable { | ||
| abstract static class Metadata extends Wrapper<String> implements Serializable { |
There was a problem hiding this comment.
We need a unit test that covers Metadata directly (not implicitly, such as VaultDataTest or otherwise)
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| public String encodeForMultiSigningWithSigningPubKey( | ||
| String json, String xrpAccountId | ||
| ) throws JsonProcessingException { | ||
| JsonNode node = BINARY_CODEC_OBJECT_MAPPER.readTree(json); | ||
| if (!node.isObject()) { | ||
| throw new IllegalArgumentException("JSON object required for signing"); | ||
| } | ||
| // NOTE: Unlike encodeForMultiSigning, we do NOT clear SigningPubKey here. | ||
| String suffix = new AccountIdType().fromJson(new TextNode(xrpAccountId)).toHex(); | ||
| return TRX_MULTI_SIGNATURE_PREFIX + encode(removeNonSigningFields(node)) + suffix; |
There was a problem hiding this comment.
⚪ Severity: LOW
encodeForMultiSigningWithSigningPubKey does not validate that SigningPubKey is present and non-empty. If the transaction retains its default empty SigningPubKey, the output is identical to encodeForMultiSigning, eliminating the intended domain separation between regular multi-sign and counterparty multi-sign contexts.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Add a validation check after the JSON object check to ensure that the SigningPubKey field is present and non-empty. This prevents the method from silently producing output identical to encodeForMultiSigning when SigningPubKey is missing or empty, which would eliminate the intended domain separation between regular multi-sign and counterparty multi-sign contexts.
After the if (!node.isObject()) check (around line 233), add:
JsonNode signingPubKeyNode = node.get("SigningPubKey");
if (signingPubKeyNode == null || signingPubKeyNode.asText().isEmpty()) {
throw new IllegalArgumentException("SigningPubKey must be present and non-empty for counterparty multi-signing");
}
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| public String encodeForMultiSigningWithSigningPubKey( | |
| String json, String xrpAccountId | |
| ) throws JsonProcessingException { | |
| JsonNode node = BINARY_CODEC_OBJECT_MAPPER.readTree(json); | |
| if (!node.isObject()) { | |
| throw new IllegalArgumentException("JSON object required for signing"); | |
| } | |
| // NOTE: Unlike encodeForMultiSigning, we do NOT clear SigningPubKey here. | |
| String suffix = new AccountIdType().fromJson(new TextNode(xrpAccountId)).toHex(); | |
| return TRX_MULTI_SIGNATURE_PREFIX + encode(removeNonSigningFields(node)) + suffix; | |
| public String encodeForMultiSigningWithSigningPubKey( | |
| String json, String xrpAccountId | |
| ) throws JsonProcessingException { | |
| JsonNode node = BINARY_CODEC_OBJECT_MAPPER.readTree(json); | |
| if (!node.isObject()) { | |
| throw new IllegalArgumentException("JSON object required for signing"); | |
| } | |
| JsonNode signingPubKeyNode = node.get("SigningPubKey"); | |
| if (signingPubKeyNode == null || signingPubKeyNode.asText().isEmpty()) { | |
| throw new IllegalArgumentException("SigningPubKey must be present and non-empty for counterparty multi-signing"); | |
| } | |
| // NOTE: Unlike encodeForMultiSigning, we do NOT clear SigningPubKey here. | |
| String suffix = new AccountIdType().fromJson(new TextNode(xrpAccountId)).toHex(); | |
| return TRX_MULTI_SIGNATURE_PREFIX + encode(removeNonSigningFields(node)) + suffix; | |
| } |
| public UnsignedByteArray toCounterpartyMultiSignableBytes(final LoanSet transaction, final Address signerAddress) { | ||
| Objects.requireNonNull(transaction); | ||
| Objects.requireNonNull(signerAddress); | ||
|
|
||
| try { | ||
| final String unsignedJson = objectMapper.writeValueAsString(transaction); | ||
| final String unsignedBinaryHex = binaryCodec.encodeForMultiSigningWithSigningPubKey( |
There was a problem hiding this comment.
⚪ Severity: LOW
toCounterpartyMultiSignableBytes does not verify transaction.signingPublicKey() is non-empty before encoding. A LoanSet with default empty SigningPubKey will silently produce multi-sign bytes indistinguishable from toMultiSignableBytes, breaking domain separation guarantees.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Add a precondition check after the null checks (line 188) to verify that transaction.signingPublicKey() is non-empty before encoding. When signingPublicKey has an empty value (i.e., it equals PublicKey.MULTI_SIGN_PUBLIC_KEY), the output of encodeForMultiSigningWithSigningPubKey becomes indistinguishable from encodeForMultiSigning, breaking domain separation between regular multi-signing and counterparty multi-signing. Add: if (transaction.signingPublicKey().value().length() == 0) { throw new IllegalArgumentException("LoanSet.signingPublicKey must be non-empty for counterparty multi-signing"); }
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| public UnsignedByteArray toCounterpartyMultiSignableBytes(final LoanSet transaction, final Address signerAddress) { | |
| Objects.requireNonNull(transaction); | |
| Objects.requireNonNull(signerAddress); | |
| try { | |
| final String unsignedJson = objectMapper.writeValueAsString(transaction); | |
| final String unsignedBinaryHex = binaryCodec.encodeForMultiSigningWithSigningPubKey( | |
| public UnsignedByteArray toCounterpartyMultiSignableBytes(final LoanSet transaction, final Address signerAddress) { | |
| Objects.requireNonNull(transaction); | |
| Objects.requireNonNull(signerAddress); | |
| if (transaction.signingPublicKey().value().length() == 0) { | |
| throw new IllegalArgumentException( | |
| "LoanSet.signingPublicKey must be non-empty for counterparty multi-signing to ensure domain separation" | |
| ); | |
| } | |
| try { | |
| final String unsignedJson = objectMapper.writeValueAsString(transaction); | |
| final String unsignedBinaryHex = binaryCodec.encodeForMultiSigningWithSigningPubKey( | |
| unsignedJson, signerAddress.value() | |
| ); | |
| return UnsignedByteArray.fromHex(unsignedBinaryHex); | |
| } catch (JsonProcessingException e) { | |
| throw new RuntimeException(e.getMessage(), e); | |
| } | |
| } |
3 participants
XLS spec - https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0066-lending-protocol
Transactions added/updated:
Ledger objects added/updated:
Flags added:
Signing added/updated:
counterpartySign()andcounterpartyMultiSign()for counterparty signing)Binary codec added/updated:
encodeForMultiSigningWithSigningPubKey()— preservesSigningPubKeyunlikeencodeForMultiSigning)Fees added/updated:
computeLoanSetNetworkFees()— accounts for extra counterparty signer in fee calculation)