fix swap claim bug for multi-fees #1421
Conversation
🦋 Changeset detectedLatest commit: af561e9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Can you write a test for swap claims as well?
There was a problem hiding this comment.
logic has changed with the inclusion of indexDB, so we should change these tests accordingly
| if (swapCommitment) { | ||
| const swaps = await indexedDb.getSwapByCommitment(swapCommitment); | ||
| if (swaps?.swap?.tradingPair?.asset1) { | ||
| return swaps.swap.tradingPair.asset1; | ||
| } | ||
| } |
There was a problem hiding this comment.
I believe this is saying "If a user is making a swap claim, attempt to find the assetIn they originally swapped with for an alt fee".
But given swap claims happen in different blocks than swaps, isn't it possible the user doesn't have that original asset anymore? Like what if they swapped the entire balance of it out? Is this logic we can't really depend on?
There was a problem hiding this comment.
as discussed, the original asset value balances are irrelevant since the swap pre-pays the swap claim. The assetId however must be consistent between the swap / swap claim and can be retrieved by querying the swap table in the database or by calling the SwapByCommitment service implementation using the swap commitment as the key.
| @@ -14,11 +14,13 @@ export const transactionPlanner: Impl['transactionPlanner'] = async (req, ctx) = | |||
| const { indexedDb } = await services.getWalletServices(); | |||
|
|
|||
| // Query IndexedDB directly to check for the existence of staking token | |||
| const nativeToken = await indexedDb.hasStakingAssetBalance(req.source); | |||
| const nativeToken = await indexedDb.hasStakingAssetBalance(); | |||
There was a problem hiding this comment.
What's the thinking around removing source here? Is the expectation that paying fees should not be scoped to one account and can be paid by any of your accounts? Is this consistent with the core repo? At the moment, in minifront, we've kinda been scoping all activity to a single account in a tx.
There was a problem hiding this comment.
for some reason I was noticing stateless check failures. When performing a swap, the AddressIndex in hasStakingAssetBalance properly corresponds to the account performing the trade, and returns the proper boolean. For swap claims, AddressIndex is an undefined value. Removing this allowed the swap claim to proceed. cc @Valentine1898
There was a problem hiding this comment.
I think the issue is that source should always be set, and we should not create transaction plans that are not linked to any account (address index)
There was a problem hiding this comment.
The fact that hasStakingAssetBalance() now doesn't take AddressIndex into account leads to a bug where the planner thinks the user has a native token for the commission but can't spend it. This bug will be reproduced quite often
|
reproduced edge case discovered during pairing:
|
| const outputAsset = request.outputs.map(o => o.value?.assetId).find(Boolean); | ||
| expect(outputAsset!.equals(umAssetId)).toBeTruthy(); |
There was a problem hiding this comment.
🚨 This (and the three changed tests below) are no longer testing extractAltFee(), since they don't call it. They should be changed to actually call extractAltFee(), or else they're essentially useless tests.
There was a problem hiding this comment.
we're passing in an indexDB now, not sure how we'd simulate this in the test since MockIndexDB has different interface?
There was a problem hiding this comment.
Un-resolving this because this is a pretty important issue: the tests here are literally not testing anything. It's the equivalent of writing an add() function, then writing a test like this:
it('adds', () => {
const result = 2 + 2;
expect(result).toBe(4);
})In this test, we haven't called the add() function, so if we break the add() function later, the tests won't catch it.
Happy to pair on updating MockedIndexedDB if that would help!
There was a problem hiding this comment.
Talked offline. Given this blocks deployment, we want to move forward. However, we should absolutely follow up with this with a separate PR.
|
@grod220 The |
|
Pairing together w/ @TalDerei, merging! |
references #1407