Xls 68 sponsored fees reserves updated#720
Conversation
…rp/lending-protocol
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #720 +/- ##
============================================
+ Coverage 93.50% 93.62% +0.11%
- Complexity 2606 2699 +93
============================================
Files 486 497 +11
Lines 6578 6790 +212
Branches 565 585 +20
============================================
+ Hits 6151 6357 +206
- Misses 270 274 +4
- Partials 157 159 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/ai-review |
|
|
||
| // Sponsor single-signing uses the same signable bytes as regular single-signing. | ||
| // The sponsor signs the complete transaction binary (including the sponsee's SigningPubKey). | ||
| final UnsignedByteArray signableTransactionBytes = this.signatureUtils.toSignableBytes(transaction); |
There was a problem hiding this comment.
sponsorSign reuses regular signable bytes — no domain separation from the account-owner path. sponsorMultiSign correctly uses toSponsorMultiSignableBytes; the single-sign path should mirror this with a dedicated method and distinct prefix to prevent signatures being cross-applicable between roles.
| final UnsignedByteArray signableTransactionBytes = this.signatureUtils.toSignableBytes(transaction); | |
| final UnsignedByteArray signableTransactionBytes = this.signatureUtils.toSponsorSignableBytes(transaction); |
There was a problem hiding this comment.
I have changed toSignableBytes to toSponsorSignableBytes
| */ | ||
| @Beta | ||
| @JsonProperty("Sponsor") | ||
| Optional<Address> sponsor(); |
There was a problem hiding this comment.
Even though other ledger objects are not listed here but some of them like MpTokenObject, MpTokenIssuanceObject etc do have reserve requirements. Should we add Sponsor field to such ledger objects as well?
There was a problem hiding this comment.
I think we should not add Sponsor to other ledger objects. There is some ambiguity in "Allowed Ledger Entry Types" ("Any other ledger entry type that contributes to an account's owner reserve, subject to implementation details") and I want to ensure that any added ledger object are truly supported
| */ | ||
| @JsonProperty("Flags") | ||
| @Value.Default | ||
| default Flags flags() { |
There was a problem hiding this comment.
Yes. Just added the file xrpl4j-core/src/main/java/org/xrpl/xrpl4j/model/flags/SponsorshipFlags.java which creates these sponsorship flags
| * | ||
| * @return An {@link Optional} {@link UnsignedInteger} containing the sponsor flags. | ||
| */ | ||
| @com.google.common.annotations.Beta |
There was a problem hiding this comment.
nit: Should we use just the class name instead of fully qualified name? This applied to other places in this PR.
| */ | ||
| @com.google.common.annotations.Beta | ||
| @JsonProperty("SponsorFlags") | ||
| Optional<UnsignedInteger> sponsorFlags(); |
There was a problem hiding this comment.
Should we create SponsorFlags class that extends TransactionFlags?
There was a problem hiding this comment.
Also, should we add the checks mentioned in the spec?
There was a problem hiding this comment.
I agree we should create SponsorFlags to extend another class, but we should extend Flags instead of TransactionFlags since other flag types do this. I added SponsorFlags
I added validation checks for SponsorFields here
There was a problem hiding this comment.
Should the return value use the flag type, like this?
Optional<SponsorFlags> sponsorFlags();
| */ | ||
| @JsonProperty("Flags") | ||
| @Value.Default | ||
| default TransactionFlags flags() { |
There was a problem hiding this comment.
Should we create SponsorshipSetFlags?
There was a problem hiding this comment.
Created xrpl4j-core/src/main/java/org/xrpl/xrpl4j/model/flags/SponsorshipSetFlags.java and associated tests xrpl4j-core/src/test/java/org/xrpl/xrpl4j/model/flags/SponsorshipSetFlagsTest.java
| * | ||
| * @throws JsonProcessingException if JSON is not valid. | ||
| */ | ||
| public String encodeForMultiSigningWithSigningPubKey( |
There was a problem hiding this comment.
Changes in this file can be reverted.
There was a problem hiding this comment.
This change has been removed from the PR. There are now no changes in xrpl4j-core/src/main/java/org/xrpl/xrpl4j/codec/
| * | ||
| * @see "https://github.com/XRPLF/XRPL-Standards/blob/master/XLS-0068-sponsored-fees-and-reserves/README.md" | ||
| */ | ||
| @Disabled("SponsorshipTransfer requires featureSponsorship amendment which may not be enabled on test networks") |
There was a problem hiding this comment.
Can we remove Disabled and instead use this image - legleux/xrpld:sponsor so that CI can pass for SponsorshipTransferIT? We can make a note to review the image back to develop once this rippled changed is merged and we have Docker image from develop branch.
There was a problem hiding this comment.
Added legleux/xrpld:sponsor to RippledContainer.java here
| * Scenario: tfSponsorshipCreate - Creating a new sponsorship where sponsor uses multi-sig. | ||
| */ | ||
| @Test | ||
| public void testSponsorshipTransferSingleSponseeMultiSponsor() throws JsonRpcClientErrorException, |
There was a problem hiding this comment.
Since we have not executed SignerListSet transaction to register signers for sponsor, I doubt this test would pass when run locally on standalone instance. Should we make sure to remove the Disabled and see if these tests run locally on standalone instance?
There was a problem hiding this comment.
Removed the disabled line for these tests but they still failed with the legleux/xrpld:sponsor rippled image because the featureSponsorship amendment is not registered in Feature.cpp
There was a problem hiding this comment.
The feature name is Sponsor. Can you give it a try?
Reviewing once the integration tests passes either locally or on the CI pipeline will avoid back and forth.
| @@ -0,0 +1,492 @@ | |||
| package org.xrpl.xrpl4j.tests; | |||
There was a problem hiding this comment.
- As per xrpl4j IT conventions we have one test file per amendment, we should merge these two IT files into one.
- It would be good to add lifecycle tests that tries to exercise as many fields and flags of various transactions and ledger objects introduced in this PR. For example:
- Tests for Fee sponsorship
- Alice sponsors Bob to pay for transaction fees
- Check ledger object that was modified/created and assert if Alice/Bob appears in it.
- Bob submits a Payment transaction.
- Assert that fees were paid by Alice and not Bob.
- Bob transfers ownership to Charlie.
- Assert that ledger objects involved are correctly reflected.
- Test for Ledger object sponsorship
- Similar idea as 2.1
- Tests for Fee sponsorship
There was a problem hiding this comment.
-
Changes to integration tests have been added to a single file, SponsorshipIT.java.
-
I have added each of these test cases here.
| * @return An {@link Optional} {@link List} of {@link Signer} objects. | ||
| */ | ||
| @JsonProperty("Signers") | ||
| Optional<List<SignerWrapper>> signers(); |
There was a problem hiding this comment.
The Singers needs to be sorted by AccountID in case of multi-signing. Batch and Lending Protocol have that sorting in-place which should be applied here as well.
There was a problem hiding this comment.
Added sorting in this file here
if (!sortedSigners()) {
return ImmutableSponsorSignature.builder()
.from(this)
.signers(signers().get().stream()
.sorted(Comparator.comparing(wrapper -> new BigInteger(
AddressCodec.getInstance().decodeAccountId(wrapper.signer().account()).hexValue(), 16
)))
.collect(Collectors.toList()))
.sortedSigners(true)
.build();
}
| @Value.Immutable | ||
| @JsonSerialize(as = ImmutableSponsorshipSet.class) | ||
| @JsonDeserialize(as = ImmutableSponsorshipSet.class) | ||
| public interface SponsorshipSet extends Transaction { |
There was a problem hiding this comment.
temMALFORMED errors checks are yet to be added across various transactions modified in this PR.
Swap the RippledContainer image from legleux/xrpld:sponsor (pulled from Docker Hub) to a locally-built xrpld:sponsor-local tag with PullPolicy.defaultPolicy(), so integration tests run against the locally-built rippled sponsor-branch image.
…sorshipIT Move the charlieAddress declaration in testSponsorshipTransferReassign from the top of the method to immediately before its first use in Step 3, satisfying the checkstyle VariableDeclarationUsageDistance rule (distance was 6, max allowed 3).
…sorshipIT Move the charlieKeyPair declaration in testSponsorshipTransferReassign from the top of the method to immediately before its first use in Step 3 (alongside charlieAddress), satisfying the checkstyle VariableDeclarationUsageDistance rule.
Restore the publicly-pullable legleux/xrpld:sponsor image and PullPolicy.alwaysPull() so integration tests run on GitHub Actions, which has no access to the locally-built xrpld:sponsor-local tag introduced in 2773b22.
Per XLS-0068, the Sponsorship ledger object carries a SponseeNode field (UInt64, nth=32). SponsorshipObject.java already serializes it via @JsonProperty("SponseeNode"), but the field was absent from definitions.json, so any binary-codec round-trip of a Sponsorship object emitted by rippled would fail to decode. Matches rippled source: sfields.macro line 155 TYPED_SFIELD(sfSponseeNode, UINT64, 32)
Merges main (Dynamic MPT XLS-94D, XLS-65 Vault, XLS-66 Lending Protocol, Issue polymorphism refactor, Q2 dep upgrades, bug fixes) while preserving XLS-68 sponsored fees changes (sponsorSign/sponsorMultiSign, SponsorshipObject, SponsorshipSet/Transfer, SponsorSignature, SponsorshipValidations). Resolved conflicts: - Vault/Loan/LoanBroker objects: took main's AssetAmount→Amount refactor - Transaction.java/TransactionType/LedgerObject: merged both sides' additions - AbstractTransactionSigner: removed duplicate counterpartySign methods, fixed signingHelper→signatureHelper, moved overloads adjacent for checkstyle - MetaMpTokenIssuanceObject: removed duplicate domainId() method - Deleted orphaned AssetAmountDeserializer/Serializer and AssetAmountTest - RippledContainer: kept legleux/xrpld:sponsor image for CI
…y check Replaced explicit (Consumer<CreateContainerCmd>) cast with an inferred lambda so that the docker-java-api transitive dependency is no longer directly referenced in source, fixing the dependency:analyze-only failure.
…devnet/testnet - Add docker-java-api as explicit test dependency in integration-tests pom.xml to satisfy dependency:analyze-only (RippledContainer directly uses CreateContainerCmd bytecode regardless of the source-level cast) - Annotate SponsorshipIT with @DisabledIf to skip on devnet/testnet/clio environments where the featureSponsorship amendment is not yet enabled, matching the pattern used by BatchTransactionIT, LendingProtocolIT, SingleAssetVaultIT, and XChainIT
| @JsonProperty("SponsorSignature") | ||
| Optional<SponsorSignature> sponsorSignature(); | ||
|
|
||
| // TODO: Add Granular Permission fields (SponsorFee, SponsorReserve) for sponsorship once #689 is merged. |
There was a problem hiding this comment.
TODO nees to be addressed.
| Objects.requireNonNull(transaction); | ||
|
|
||
| // Validate sponsorship fields per XLS-0068 | ||
| SponsorshipValidations.validateSponsorFields(transaction); |
There was a problem hiding this comment.
We shouldn't inject statically like this. If we have a dependency like SponsorshipValidations then we need to inject via the constructor. This will allow proper mocking for testing.
That said, I'm not convinced that this check should occur here in SignatureUtils. This function is merely serializing bytes. The correctness or non-correctness checks seem like they should be in the transaction itself, or possibly in the signing function itself (as opposed to here).
If we want to consider adding an @Check default function in Transaction.java that might be a good candidate. Transaction.java is not itself an immutable, but we do have some prior art here where we annotation transactionType() function with @Value.Default. The immutables codegen picks this up and applies it to all Transactions.
| * @return An {@link UnsignedByteArray}. | ||
| */ | ||
| @Beta | ||
| public UnsignedByteArray toSponsorSignableBytes(final Transaction transaction) { |
There was a problem hiding this comment.
This method doesn't seem any different from toSignableBytes(final Transaction transaction). Is there any reason we have it like this (i.e., with a different name)?
| Objects.requireNonNull(privateKeyable); | ||
| Objects.requireNonNull(transaction); | ||
|
|
||
| final UnsignedByteArray signableTransactionBytes = this.signatureUtils.toSponsorSignableBytes(transaction); |
There was a problem hiding this comment.
I might be missing something, but if privateKeyable is the Sponsor's private key, then couldn't this function simply call this.signatureUtils.toSignableBytes(transaction) instead?
If that answer is "yes", then do we need sponsorSign at all?
I see that we need a special function for sponsorMultiSign, so even if this is a redudant function, I kind of like the API part of it. In that case, could this just be implemented like this?
@Override
public <T extends Transaction> Signature sponsorSign(final P privateKeyable, final T transaction) {
Objects.requireNonNull(privateKeyable);
Objects.requireNonNull(transaction);
return this.sign(privateKeyable, transaction).signature();
}| } | ||
|
|
||
| @Override | ||
| public <T extends Transaction> Signature sponsorSign(final P privateKeyable, final T transaction) { |
There was a problem hiding this comment.
Should this return a SingleSignedTransaction<T> instead of a `Signature?
I'm inclined to say "no" because sign that returns a SingleSignedTransaction<T> appears to be an anomaly rather than a pattern, but this is still probably worth discussing.
| * Its API is subject to change.</p> | ||
| */ | ||
| @Beta | ||
| protected static final PaymentFlags SPONSOR_CREATED_ACCOUNT = new PaymentFlags(0x00080000L); |
There was a problem hiding this comment.
I don't see this change reflected in PaymentFlagsTests.
| */ | ||
| @Beta | ||
| @JsonProperty("Sponsor") | ||
| Optional<Address> sponsor(); |
There was a problem hiding this comment.
Let's be sure to update unit tests for this. In particular, we'll want to make sure serialization works with and without this field being present.
| */ | ||
| @Beta | ||
| @JsonProperty("Sponsor") | ||
| Optional<Address> sponsor(); |
There was a problem hiding this comment.
Let's be sure to update unit tests for this. In particular, we'll want to make sure serialization works with and without this field being present.
| */ | ||
| @Beta | ||
| @JsonProperty("Sponsor") | ||
| Optional<Address> sponsor(); |
There was a problem hiding this comment.
Let's be sure to update unit tests for this (e.g., EscrowObjectJsonTests). In particular, we'll want to make sure serialization works with and without this field being present.
We should do this for any ledger object that gets a sponsor.
| */ | ||
| @Beta | ||
| @JsonProperty("LowSponsor") | ||
| Optional<Address> lowSponsor(); |
There was a problem hiding this comment.
Let's be sure to update unit tests for this. In particular, we'll want to make sure serialization works with and without this field being present.
| @Value.Immutable | ||
| @JsonSerialize(as = ImmutableSponsorSignature.class) | ||
| @JsonDeserialize(as = ImmutableSponsorSignature.class) | ||
| public interface SponsorSignature { |
There was a problem hiding this comment.
I think this class needs to move to package org.xrpl.xrpl4j.crypto.signing;
This PR supports the new amendment for sponsored fees and reserves for the java library.
Changes here use XRPLF/rippled#5887