chore: remove fee compute for swap transaction and muliplier base inclusion fee for simulation#139
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates swap-transaction validation so that fee computation is no longer applied when decoding/validating a swap envelope from XDR, and adjusts the related unit test expectations.
Changes:
- Update
TransactionService.createValidatedSwapTransactionto stop using thecomputingFeereturn value and return the original decoded transaction. - Update
TransactionServiceswap-validation test expectations around Soroban invoke swap simulation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/snap/src/services/transaction/TransactionService.ts | Stops propagating the computingFee result through swap validation/return path. |
| packages/snap/src/services/transaction/TransactionService.test.ts | Updates Soroban swap validation test assertions to match the new return behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+399
to
+401
| // Bridge API swap transaction already include the fee — we trust it and do not recalculate. | ||
| // For Soroban invokes, computingFee simulates the transaction to validate it. | ||
| await this.computingFee(transaction); |
Collaborator
Author
There was a problem hiding this comment.
this is expected, as we only using compute fee method to simulate if the txn is get though or not
Julink-eth
previously approved these changes
Jun 26, 2026
stanleyyconsensys
changed the title
| ); | ||
| } | ||
|
|
||
| // Get the min resource fee from the simulation response. |
Collaborator
Author
There was a problem hiding this comment.
further to Stellar Team suggest, we should no need to change the resource fee
instead we should change the base fee (inclusion fee)
| timeout: this.#getTimeout(), | ||
| scope, | ||
| // Base fee is a placeholder until RPC simulation. | ||
| fee: BASE_FEE.toString(), |
Collaborator
Author
There was a problem hiding this comment.
dont use fix fee here
| 'Invalid fee amount, falling back to configured inclusion fee', | ||
| ); | ||
| fee = BASE_FEE; | ||
| fee = baseInclusionFee(); |
Collaborator
Author
There was a problem hiding this comment.
for fallback case, we should also do multiplier with the base fee
Comment on lines
121
to
129
| /** | ||
| * Fetches the current base fee per operation from the Stellar network. | ||
| * Fetches the current Stellar network base fee and applies | ||
| * {@link AppConfig.transaction.baseFeeMultiplier} to produce the per-operation inclusion fee. | ||
| * | ||
| * @param scope - The CAIP-2 chain ID. | ||
| * @returns A Promise that resolves to the base fee as BigNumber. | ||
| * @returns A Promise that resolves to the inclusion fee in stroops. | ||
| * @throws {NetworkServiceException} If the fee cannot be fetched. | ||
| */ | ||
| async getBaseFee(scope: KnownCaip2ChainId): Promise<BigNumber> { |
Collaborator
Author
There was a problem hiding this comment.
we keep getBaseFee for now, otherwise, it will be a lot of changes
Comment on lines
1204
to
1208
| isSimErrorSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it('applies simulationFeeMultiplier to minResourceFee before assembling', async () => { | ||
| const { simulateTransactionSpy } = getRpcServerSpies(); | ||
| const mockInvoke = createMockInvokeHostFunctionTransaction(); | ||
| const minResourceFee = '1000'; | ||
| const transactionData = new SorobanDataBuilder(); | ||
| const setResourceFeeSpy = jest.spyOn(transactionData, 'setResourceFee'); | ||
| simulateTransactionSpy.mockResolvedValue({ | ||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| _parsed: true, | ||
| id: '1', | ||
| latestLedger: 1, | ||
| events: [], | ||
| minResourceFee, | ||
| transactionData, | ||
| result: { auth: [] }, | ||
| } as never); | ||
|
|
||
| const result = await networkService.simulateTransaction( | ||
| mockInvoke, | ||
| scope, | ||
| ); | ||
|
|
||
| expect(result).toBeInstanceOf(Transaction); | ||
| expect(setResourceFeeSpy).toHaveBeenCalledWith( | ||
| new BigNumber(minResourceFee) | ||
| .multipliedBy(AppConfig.transaction.simulationFeeMultiplier) | ||
| .toString(), | ||
| ); | ||
| }); | ||
|
|
||
| it('calls RPC simulateTransaction with the wrapped envelope getRaw()', async () => { | ||
| const { simulateTransactionSpy } = getRpcServerSpies(); |
Collaborator
Author
There was a problem hiding this comment.
this is no need to add a regression test
Julink-eth
approved these changes
Jun 29, 2026
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.
Explanation
References
Checklist