fix(swap): forward the quoted buy fee into the existing-currency swap transaction - #1277
Merged
Conversation
… transaction The `ExistingCurrency` branch of `IntentStatefulSwap.transaction()` never passed `request.feeAmount` to `TransactionBuilder.swap`, so it silently took the old `feeAmount = 0` default. The client quoted a fee to the server in the initiate request, the server built `VM::TransferForSwapWithFee` (opcode 0x14, 8 accounts, 18 bytes) — and the client built the plain `VM::TransferForSwap` (0x11, 7 accounts, 10 bytes). The missing fee-destination account also shifted every downstream account index by one, so the two transactions could never match and every fee-bearing "Get" failed to sign. The `NewCurrency` and `Stablecoin` branches directly below it both already forwarded the fee, which is why only existing-currency buys broke. `TransactionBuilder.swap` loses its `feeAmount = 0` default so the same omission becomes a compile error rather than a silent no-fee transaction. `feeAmount` is consumed only by the Buy route; sells and cross-currency swaps ignore it, so this is a no-op for them. Adds a regression test that drives the real `IntentStatefulSwap` and fails without the fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
Every fee-bearing Get (buy) failed.
DifferKtshows the server's transaction and the client's diverging on the VM instruction:0x14VM::TransferForSwapWithFee0, 17, 3, 8, 7, 5,16, 1214+500000+5000+ bump (18 bytes)0x11VM::TransferForSwap0, 16, 3, 8, 7, 5, 1211+500000+ bump (10 bytes)The server's account list carries one extra entry — the fee destination at index 16 — so every client index at or above 16 is shifted down by one. That accounts for every other mismatched line in the diff; the signatures can never match.
Root cause
The
ExistingCurrencybranch ofIntentStatefulSwap.transaction()never forwardedrequest.feeAmounttoTransactionBuilder.swap, so it silently took that function'sfeeAmount = 0default. The client quoted the fee to the server in the initiate request (LocalToProtobuf.setFeeAmount), then built the no-fee instruction anyway.Everything downstream was already correct and tested — the proto parsing of
feeDestination, the with-fee instruction builder, and the selection logic inbuildExistingCurrencyBuyInstructions. Only the one call site was wrong. TheNewCurrencyandStablecoinbranches immediately below it both forward the fee, which is why only existing-currency buys broke.The change
request.feeAmount?.underlyingTokenAmount?.quarks ?: 0in theExistingCurrencybranch.TransactionBuilder.swap'sfeeAmount = 0default. It has exactly one non-test caller, so the same omission is now a compile error instead of a silently fee-less transaction.feeAmountis consumed only by theSwapRoute.Buybranch insideswap—buildSellInstructionsandbuildCrossCurrencyExistingSwapInstructionstake no fee parameter — so this is a no-op for sells and cross-currency swaps.Testing
ExistingCurrencyBuyFeeTestis a JVM unit test, so it runs in CI — the existing swap coverage lives inandroidTest, which does not. The key case drives the realIntentStatefulSwapwith a realStatefulSwapRequestat the amounts from the failing log (500000 swap / 5000 fee) and asserts the transaction carries opcode0x14with both amounts encoded.