Base the EscrowFinish inner fee on the encoded fulfillment blob size rather than the preimage size

Author: kuan121

xrpl4j-core/src/main/java/org/xrpl/xrpl4j/model/client/fees/FeeUtils.java

@@ -27,7 +27,6 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.primitives.UnsignedInteger;
 import com.google.common.primitives.UnsignedLong;
-import com.ripple.cryptoconditions.PreimageSha256Fulfillment;
 import org.immutables.value.Value.Derived;
 import org.immutables.value.Value.Immutable;
 import org.xrpl.xrpl4j.model.immutables.FluentCompareTo;
@@ -46,7 +45,6 @@
 import java.math.MathContext;
 import java.math.RoundingMode;
 import java.util.Arrays;
-import java.util.Base64;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
@@ -313,18 +311,14 @@ private static long surchargeUnits(final FeeParams feeParams, final Transaction
    * @return A number of extra base fees.
    */
   private static long fulfillmentUnits(final EscrowFinish escrowFinish) {
-    return escrowFinish.fulfillment()
-      .map(fulfillment -> {
-        Preconditions.checkArgument(
-          PreimageSha256Fulfillment.class.isAssignableFrom(fulfillment.getClass()),
-          "Only PreimageSha256Fulfillment is supported, but the fulfillment was a %s.",
-          fulfillment.getClass().getSimpleName()
-        );
-        final long fulfillmentByteSize = Base64.getUrlDecoder()
-          .decode(((PreimageSha256Fulfillment) fulfillment).getEncodedPreimage()).length;
-        // See rippled's EscrowFinish::calculateBaseFee: extraFee = base * (32 + (fulfillment size / 16)).
-        return 32L + (fulfillmentByteSize / 16L);
-      })
+    // rippled charges by the on-wire size of the sfFulfillment blob:
+    //   extraFee = base * (32 + fulfillmentBytes / 16)   (EscrowFinish::calculateBaseFee)
+    // fulfillmentRawValue() is that exact blob, hex-encoded — the DER-encoded crypto-condition fulfillment that
+    // EscrowFinish#normalizeFulfillment always populates. It is NOT the decoded preimage: for a PREIMAGE-SHA-256
+    // fulfillment the DER wrapper adds a few bytes, which can change the /16 term near a boundary. Measuring the
+    // blob (not the preimage) is what matches rippled.
+    return escrowFinish.fulfillmentRawValue()
+      .map(fulfillmentHex -> 32L + ((fulfillmentHex.length() / 2L) / 16L))
       .orElse(0L);
   }
 

xrpl4j-core/src/test/java/org/xrpl/xrpl4j/model/client/fees/FeeUtilsTest.java

@@ -656,20 +656,29 @@ void allThreeFeeLevelsAreScaled() {
 
   @Test
   void escrowFinishWithoutFulfillmentCostsOneBaseFee() {
-    assertFeeUnits(paramsFor(escrowFinish(false)), 1);
+    assertFeeUnits(paramsFor(escrowFinish(0)), 1);
   }
 
   @Test
   void escrowFinishWithFulfillmentAddsTheSizeSurcharge() {
-    // 1 + (32 + 32/16) = 35
-    assertFeeUnits(paramsFor(escrowFinish(true)), 35);
+    // rippled charges by the on-wire fulfillment blob, not the preimage. A 32-byte preimage DER-encodes to a
+    // 36-byte blob, so the surcharge is 32 + 36/16 = 34: total 1 + 34 = 35.
+    assertFeeUnits(paramsFor(escrowFinish(32)), 35);
+  }
+
+  @Test
+  void escrowFinishSurchargeUsesTheEncodedBlobSizeNotThePreimageSize() {
+    // Boundary case exposing the difference: a 28-byte preimage DER-encodes to a 32-byte blob. Measuring the blob
+    // (correct, matches rippled) gives 32 + 32/16 = 34 -> total 35. Measuring the preimage (the old, buggy
+    // behavior) would give 32 + 28/16 = 33 -> total 34, underpaying by one base fee.
+    assertFeeUnits(paramsFor(escrowFinish(28)), 35);
   }
 
   @Test
   void escrowFinishSurchargeIsAddedToTheSignatureTerms() {
     // (1 + 2) + (32 + 32/16) = 37 — the signer terms EscrowFinish.computeFee omits.
     assertFeeUnits(
-      FeeParams.builder().feeResult(feeResultBuilder().build()).transaction(escrowFinish(true))
+      FeeParams.builder().feeResult(feeResultBuilder().build()).transaction(escrowFinish(32))
         .signersCount(UnsignedInteger.valueOf(2)),
       37
     );
@@ -1052,16 +1061,20 @@ private AccountDelete accountDelete() {
       .build();
   }
 
-  private EscrowFinish escrowFinish(final boolean withFulfillment) {
+  /**
+   * An {@link EscrowFinish} carrying a PREIMAGE-SHA-256 fulfillment built from a {@code preimageBytes}-byte preimage,
+   * or no fulfillment when {@code preimageBytes} is 0.
+   */
+  private EscrowFinish escrowFinish(final int preimageBytes) {
     ImmutableEscrowFinish.Builder builder = EscrowFinish.builder()
       .account(ALICE)
       .owner(BOB)
       .offerSequence(UnsignedInteger.ONE)
       .fee(XrpCurrencyAmount.ofDrops(0))
       .sequence(UnsignedInteger.ONE)
       .signingPublicKey(PUBLIC_KEY);
-    return withFulfillment ?
-      builder.fulfillment(PreimageSha256Fulfillment.from(new byte[32])).build() : builder.build();
+    return preimageBytes == 0 ?
+      builder.build() : builder.fulfillment(PreimageSha256Fulfillment.from(new byte[preimageBytes])).build();
   }
 
   private Batch batch(final Address outerAccount, final Transaction... inners) {