osmosis-frontend icon indicating copy to clipboard operation
osmosis-frontend copied to clipboard

feat(swap): adjust swap amount for gas fees, finalize swap tx data

Open Amosel opened this issue 1 year ago • 7 comments

Swap Transaction Enhancements.

  • Add useSendSwapTxMutation hook to encapsulate swap transaction sending logic
  • Implement finalizeSwapTxData function to create immutable SwapTransactionData object
    • Subtract gas fees from swap amount if amount left in balance can not cover gas fees
    • Set amountAdjustedForFees flag to indicate if adjustment occurred
  • Update SwapTool component to use useSendSwapTxMutation hook and finalizeSwapTxData function
  • Define SwapTransactionData interface for structured swap transaction data

What is the purpose of the change

This pull request introduces several enhancements to the swap transaction process, focusing on adjusting the swap amount for gas fees, encapsulating the transaction sending logic, ensuring data consistency, and improving code organization.

ClickUp Task

ClickUp Task URL

Changelog

  1. useSendSwapTxMutation Hook: A new hook has been added to handle the sending of swap transactions. This hook encapsulates the logic for preparing and sending the transaction, making it reusable across different components.

  2. finalizeSwapTxData Function: A new function has been implemented to finalize the data required for a swap transaction. It takes the current swap state and feature flags as input and returns an immutable SwapTransactionData object. This ensures data consistency and avoids potential issues related to mutable state.

    • If the amount left in the balance after the swap is below a certain threshold, the function subtracts the estimated gas fees from the swap amount. This ensures that the transaction can be successfully executed even if the remaining balance is low.
    • The function sets the amountAdjustedForFees flag to indicate whether the swap amount has been adjusted to account for gas fees. This flag can be utilized by the calling code to display appropriate messages to the user or handle the transaction differently.
  3. SwapTransactionData Interface: An interface has been defined to provide a structured representation of the finalized swap transaction data. This interface clearly defines the shape of the data required for a swap transaction, making it easier to understand and maintain.

Refactoring

In addition to the new features, this pull request includes various code refactoring changes to improve code organization and maintainability:

  • Swap-related hooks have been moved into a dedicated swap folder, providing a cleaner and more focused structure.
  • Certain hooks and functions have been renamed for clarity and consistency, improving code readability.
  • Import paths have been updated to reflect the new file structure, ensuring proper module resolution.

These refactoring changes aim to enhance the overall quality and maintainability of the codebase.

Importance of Immutable Swap Transaction Data

Converting the swap state into an immutable SwapTransactionData object is crucial for several reasons:

  1. Data Consistency: By creating an immutable object with all the necessary data for the swap transaction, we ensure that the data remains consistent throughout the transaction process. This eliminates the risk of inconsistencies and unexpected behavior caused by mutable state.

  2. Avoiding Race Conditions: Immutable data prevents multiple parts of the code from modifying the same data simultaneously, eliminating the possibility of race conditions. This ensures predictable behavior and reduces the chances of bugs related to concurrent modifications.

  3. Predictability: An immutable SwapTransactionData object provides predictability in the behavior of the code. Once created, the object's values won't change unexpectedly, making it easier to reason about the code and reducing the chances of bugs.

  4. Debugging and Reproducibility: If an issue or a bug occurs during the swap transaction process, having an immutable SwapTransactionData object makes it easier to debug and reproduce the problem. The state of the object can be logged or captured at different points, aiding in identifying the root cause of the issue.

  5. Auditing and Tracking: Immutable data allows for easier auditing and tracking of the swap transactions. The finalized data cannot be modified after the fact, providing a reliable record of the transaction details.

Please review the changes and provide any feedback or suggestions for further improvements.

Testing and Verifying

Documentation and Release Note

Amosel avatar Mar 26 '24 14:03 Amosel

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
osmosis-frontend ❌ Failed (Inspect) Mar 29, 2024 6:02pm
osmosis-frontend-dev ❌ Failed (Inspect) Mar 29, 2024 6:02pm
osmosis-frontend-edgenet ❌ Failed (Inspect) Mar 29, 2024 6:02pm
osmosis-testnet ❌ Failed (Inspect) Mar 29, 2024 6:02pm

vercel[bot] avatar Mar 26 '24 14:03 vercel[bot]

From our call @Amosel , I have an idea of how we can handle this fee issue for any amount being transferred in any tx. That way, we wouldn't hardcode the solution just for swaps, but also for anywhere inputs are being transferred by a tx.

Our frontend currently (though with legacy MobX stores) has a utility (called use fake fee config that handles subtracting the tx fee from the max input of only the staking token (OSMO). This is what we use throughout the app to make sure txs don't get blocked by transferring the max amount of OSMO. But 2 problems: only handles OSMO, and uses hardcoded gas values to estimate the subtraction.

With new chain features, such as multi fee tokens and the gas estimation query, we should be able to get the optimal FE UX that leverages those features, causing users to be blocked with fee issues in the minimal number of transactions.

We have a new hook that manages amount inputs without MobX, and using React query, called useAmountInput. It's used for managing the input of the in token in the swap tool.

We could extend it to allow the caller to pass in the list of messages being called, and perhaps an accessor callback for extracting the relevant amount that would be transferred by that tx.

Then, the useAmountInput hook could use those parameters to query the account store for the estimated fee value, and for checking the other user balances (that we already query in useBalances) to see if they can pay fees in any other token. What remains to be handled is 1) if the user's current wallet supports multiple fee tokens and 2) if we can specify the fee token to be used when we send the tx to the wallet.

Also, it could return a flag indicating if the fee is currently being subtracted. Then, we could use that to display a tooltip explaining why they're unable to enter in the full balance for that particular token. Thoughts @JoseRFelix ?

jonator avatar Mar 26 '24 19:03 jonator

@jonator Thank you, Jon! This presents an excellent chance to utilize the use-amount-input hook. Instead of directly passing messages, what if we introduce a parameter, gasFee: CoinPretty, to represent the gas fees? This way, the hook could automatically adjust the amount by deducting the gas fees. I prefer this approach as it assigns the responsibility of handling simulations to the React component or hook, accommodating more scenarios. Specifically, it supports cases where the gas token's estimation isn't derived from a simulation function

JoseRFelix avatar Mar 26 '24 19:03 JoseRFelix

True. That would be simple. Perhaps we can provide other hooks to help with getting the user's available gas tokens and for estimating the fee for the given tx in question.

jonator avatar Mar 26 '24 19:03 jonator

I believe allowing users to select the fee token is outside the scope of this PR, however, our estimateTxFee hook already returns the gas Coin so we only need to send in that parameter. It should be very straightforward

JoseRFelix avatar Mar 26 '24 20:03 JoseRFelix

@JoseRFelix @jonator What I understand is that the idea is to introduce a parameter gasFee: CoinPretty to useAmountInput, which comes from the result of the query in useEstimateTxFees. This will allow for amount adjustment derivation, for instance, it could subtract the network fees from the amount it returns, and add a flag or a value specifying the adjustment amount or zero upstream. Is that right?

Amosel avatar Mar 29 '24 15:03 Amosel

@JoseRFelix @jonator What I understand is that the idea is to introduce a parameter gasFee: CoinPretty to useAmountInput, which comes from the result of the query in useEstimateTxFees. This will allow for amount adjustment derivation, for instance, it could subtract the network fees from the amount it returns and add a flag or a value specifying the adjustment amount or zero upstream. Is that right?

This sounds right. By flag do you mean a method for toggling max that will subtract the balance and fee?

JoseRFelix avatar Mar 29 '24 18:03 JoseRFelix

@JoseRFelix @jonator By flag I mean an state that indicate if adjustment was made or not.

That said, such implementation creates a feedback loop. gasFee: Coin required by useAmountInput comes from useEstimateTxFees which is reliant on useAmountInput.

Amosel avatar Apr 02 '24 13:04 Amosel

@JoseRFelix @jonator By flag I mean an state that indicate if adjustment was made or not.

That said, such implementation creates a feedback loop. gasFee: Coin required by useAmountInput comes from useEstimateTxFees which is reliant on useAmountInput.

I think regardless, there is a loop. The message contains the amount to swap, and that's the same amount we need to modify if some of that amount is needed for fees. This wasn't a problem before simulation, since we just had a hardcoded cost per tx. @JoseRFelix thoughts?

jonator avatar Apr 02 '24 15:04 jonator

@JoseRFelix @jonator By flag I mean an state that indicate if adjustment was made or not.

That said, such implementation creates a feedback loop. gasFee: Coin required by useAmountInput comes from useEstimateTxFees which is reliant on useAmountInput.

We shouldn't use the current input amount but rather the total user balance to compute the routes. This approach accounts for the fact that available routes may vary due to the input amount, which can also affect the message we generate. Consequently, we need to derive a quote for the user's total balance and use that to estimate the transaction.

I suggest moving the message computation to the quote hook as I did for 1-Click Trading. Afterward, compute the maxBalance quote in a separate hook, simulate it, and set the gas amount. It would probably look like this:

 const [gasAmount, setGasAmount] = useState<CoinPretty>();

  const inAmountInput_ = useAmountInput({
    currency: swapAssets.fromAsset,
    gasFee: gasAmount.denom === swapAssets.fromasset.denom ? gasAmount : undefined
  });

  const {
    data: maxBalanceQuote,
    isLoading: isMaxBalanceQuoteLoading,
  } = useQueryRouterBestQuote(
    {
      tokenIn: swapAssets.fromAsset,
      tokenOut: swapAssets.toAsset,
      tokenInAmount: inAmountInput.balance?.toCoin().amount ?? "0", 
      forcePoolId: forceSwapInPoolId,
      maxSlippage,     
    },
    !!inAmountInput.balance && isToFromAssets
  );

  const {
    data: maxBalanceNetworkFee,
    isLoading: isLoadingMaxBalanceNetworkFee, // Disable the max button while this is loading
  } = useEstimateTxFees({
    chainId: chainStore.osmosis.chainId,
    messages: maxBalanceQuote?.messages,
    enabled:
      !!inAmountInput.balance &&
      featureFlags.swapToolSimulateFee,    
  });

  useEffect(() => {
    if (!maxBalanceNetworkFee?.gasAmount) return;
    setGasAmount(maxBalanceNetworkFee.gasAmount);
  }, [networkFee?.gasAmount]);

JoseRFelix avatar Apr 02 '24 17:04 JoseRFelix

closed and move to https://github.com/osmosis-labs/osmosis-frontend/pull/3081

Amosel avatar Apr 11 '24 21:04 Amosel