Skip to content

Add option to buy or sell eth#730

Merged
ebma merged 49 commits into
stagingfrom
719-add-option-to-buy-or-sell-eth
Jun 27, 2025
Merged

Add option to buy or sell eth#730
ebma merged 49 commits into
stagingfrom
719-add-option-to-buy-or-sell-eth

Conversation

@ebma
Copy link
Copy Markdown
Member

@ebma ebma commented Jun 17, 2025

This pull request introduces several updates across controllers, routes, services, and phase handlers to improve error handling, logging, and parameter consistency. The most significant changes include enhancing error messages, updating route definitions, standardizing token details parameters, and improving logging for better debugging and traceability.

Error Handling Improvements:

  • Enhanced error logging in quote.controller.ts to include detailed error messages when creating quotes.
  • Improved error handling in squid-router-pay-phase-handler.ts by replacing console.error with logger.error for better logging consistency.

Route and Controller Updates:

  • Updated the updateRamp route to remove the rampId parameter from the URL and moved it to the request body for consistency. [1] [2]
  • Adjusted the updateRamp controller to reflect the removal of rampId from the URL, simplifying the request structure.

Token Details Standardization:

  • Replaced PendulumDetails with PendulumTokenDetails throughout outAmount.ts and related handlers for more precise parameter naming. [1] [2] [3] [4] [5]
  • Updated token detail references in multiple phase handlers (e.g., currencyId instead of pendulumCurrencyId) to align with the new naming convention. [1] [2] [3]

Logging Enhancements:

  • Added detailed logging in pendulum-moonbeam-phase-handler.ts to trace XCM transfer progress and state transitions.
  • Improved logging in squid-router-pay-phase-handler.ts to provide better insights into Axelar gas funding and route fetching. [1] [2]

Miscellaneous Fixes:

  • Renamed functions (e.g., didInputTokenArrivedOnPendulum to didInputTokenArriveOnPendulum) for grammatical correctness and clarity. [1] [2]
  • Removed redundant checks in initial-phase-handler.ts to simplify error conditions.

Closes #719.

@ebma ebma linked an issue Jun 17, 2025 that may be closed by this pull request
@netlify
Copy link
Copy Markdown

netlify Bot commented Jun 17, 2025

Deploy Preview for pendulum-pay ready!

Name Link
🔨 Latest commit 6051710
🔍 Latest deploy log https://app.netlify.com/projects/pendulum-pay/deploys/685ebafdc04a370008dfc916
😎 Deploy Preview https://deploy-preview-730--pendulum-pay.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

ebma added 25 commits June 17, 2025 17:14
# Conflicts:
#	apps/api/src/api/services/ramp/quote.service/gross-output.ts
#	apps/api/src/api/services/ramp/quote.service/index.ts
#	apps/api/src/api/services/transactions/squidrouter/route.ts
#	apps/frontend/src/components/AssetNumericInput/index.tsx
#	apps/frontend/src/components/NumericInput/index.tsx
#	packages/shared/src/tokens/evm/config.ts
#	packages/shared/src/tokens/types/evm.ts
#	packages/shared/src/tokens/utils/helpers.ts
@ebma ebma requested a review from a team June 26, 2025 15:55
Comment thread apps/frontend/src/services/api/ramp.service.ts
Comment thread apps/api/src/api/services/xcm/send.ts
Comment thread apps/api/src/api/services/ramp/quote.service/gross-output.ts
} catch (error) {
logger.error("Error creating quote:", error);
logger.error(
`Error creating quote: ${error && typeof error === "object" && "message" in error ? error.message : String(error)}`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use here error instanceof Error ? instead of error && typeof error === "object" && "message" in error ?

Copy link
Copy Markdown
Contributor

@gianfra-t gianfra-t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that you tested this already on the routes, seems all good!

}
}

export async function getEvmBridgeQuote(request: EvmBridgeQuoteRequest) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the function handles two distinct responsibilities:

  1. Preparing the route request (gathering token details, formatting the amount, building params)
  2. Processing the route response (extracting output amounts, calculating fees)

We could extract them into separate functions to make the context more clear.
For example:

export async function getEvmBridgeQuote(request: EvmBridgeQuoteRequest) {
  const routeParams = buildRouteRequest(request);
  return await getSquidrouterRouteData(routeResponse);
}
async function getSquidrouterRouteData(routeParams) {
 const result = await getSquidrouterRouteData(routeParams);
  const outputTokenDecimals = routeResponse.route.estimate.toToken.decimals;
  const outputAmountRaw = routeResponse.route.estimate.toAmount;
  const outputAmountDecimal = parseContractBalanceResponse(outputTokenDecimals, BigInt(outputAmountRaw)).preciseBigDecimal;
  const networkFeeUSD = await calculateSquidrouterNetworkFee(routeResponse);

  return {
    networkFeeUSD,
    outputAmountDecimal,
  };
}
function buildRouteRequest(request: EvmBridgeQuoteRequest) {
  const token = getTokenDetailsForEvmDestination(request.inputOrOutputCurrency, request.sourceOrDestination);
  const amountRaw = multiplyByPowerOfTen(request.amountDecimal, token.decimals).toFixed(0, 0);
  return prepareSquidrouterRouteParams(request.rampType, amountRaw, token, request.sourceOrDestination);
}

wdyt?

);

const inputAmountForNablaSwap = new Big(request.inputAmount).minus(preNablaDeductibleFeeInInputCurrency);
let inputAmountForNablaSwap = new Big(request.inputAmount).minus(preNablaDeductibleFeeInInputCurrency);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking of moving the inputAmountForNablaSwap logic into its own function so we can use const instead of let. It should make the code a bit cleaner and safer - wdyt?

async function calculateInputAmountForNablaSwap(request: RequestType, preNablaDeductibleFeeInInputCurrency: Big, preNablaDeductibleFeeAmount: Big) {
  if (request.rampType === "off" && request.from !== "assethub") {
    // Check squidrouter rate and adjust the input amount accordingly
    const bridgeQuote = await getEvmBridgeQuote({
      amountDecimal: request.inputAmount,
      inputOrOutputCurrency: request.inputCurrency as OnChainToken,
      rampType: request.rampType,
      sourceOrDestination: request.from
    });
    return new Big(bridgeQuote.outputAmountDecimal).minus(preNablaDeductibleFeeAmount);
  } else {
    return new Big(request.inputAmount).minus(preNablaDeductibleFeeInInputCurrency);
  }
}

// Usage:
const inputAmountForNablaSwap = await calculateInputAmountForNablaSwap(
  request,
  preNablaDeductibleFeeInInputCurrency,
  preNablaDeductibleFeeAmount
);

if (!isNetworkEVM(network)) {
throwInvalidNetworkError(network);
}
network = network as EvmNetworks;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we do the type assertion here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing it out. Made me realize that we have two types for this, EVMNetworks and EvmNetworks. I cleaned it up now.

Comment thread apps/frontend/src/components/AssetNumericInput/index.tsx
}));
}

selectedNetwork = selectedNetwork as EvmNetworks;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we do the type assertion here?
we could just do it like: evmTokenConfig[selectedNetwork as EvmNetworks]

onChange?: (e: ChangeEvent) => void;
}

function trimToMaxDecimals(value: string, decimals: number) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already defined in the NumericInput/helpers.ts

export function trimToMaxDecimals(value: string, maxDecimals: number): string {
const [integer, decimal] = value.split(".");
return decimal ? ${integer}.${decimal.slice(0, maxDecimals)} : value;
}

}

function handleOnPaste(e: ClipboardEvent): void {
function handleOnPaste(e: ClipboardEvent<HTMLInputElement>): void {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these functions are already defined in the NumericInput/helpers.ts

Copy link
Copy Markdown
Member Author

@ebma ebma Jun 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but interesting we weren't even using them before 😅


// Helper function to format exchange rate strings
function formatExchangeRateString(rate: number, input: string, output: string) {
return `1 ${input} ≈ ${rate.toFixed(4)} ${output}`;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using many conditions, could we just use roundDownToSignificantDecimals?

// Round a number to a specified number of decimals
// If the number is small, ensure that the required number of significant
// decimals is retained
// e.g., if decimals = 2, then
// - 12345 -> 12345
// - 12.345 -> 12.34
// - 1.2345 -> 1.23
// - 0.012345 -> 0.012
// - 0.00012345 -> 0.00012
export function roundDownToSignificantDecimals(big: BigNumber, decimals: number) {
  return big.prec(Math.max(0, big.e + 1) + decimals, 0);
}

Copy link
Copy Markdown
Member

@Sharqiewicz Sharqiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it looks good 👍 Just in some files I was wondering if we can optimize some things

return balances[0];

// Filter out native tokens since this hook specifically expects OnChainTokenDetails
const onChainBalance = balances.find(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we add these checks if we are sure in the array - there is only one token we requested for?

import { useVortexAccount } from "./useVortexAccount";

// Hook to get EVM native token balance
export const useEvmNativeBalance = (nativeToken: EvmTokenDetails | undefined): EvmTokenDetailsWithBalance | null => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering:
Why do we create these two new functions instead of just using useEvmBalances and useAssetHubBalances
Or if we want to have the native functions, why do we support passing tokens if there is always only one native token? wdyt

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to pass the token because the functions itself return XXXTokenDetails and not just a balance. And i didn't want to duplicate the filtering we are already doing in the useOnchainTokenBalances function. I can change it though, so that the functions for the native balances can be called in isolation.

}

// This type is used to represent all networks that can be used as a source or destination in the system.
export type EvmNetworks =
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have this EvmNetworks type but it was not exported from the file:
type EVMNetworks = Exclude<Networks, Networks.AssetHub>;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I missed that. I decided to stick to the other type though where the EvmNetworks are handpicked. This way, we can easily add more other networks to the Networks enum without them accidentally becoming available on the EvmNetworks enum too (which would be the case if we use an Omit for this type).

Copy link
Copy Markdown
Member Author

@ebma ebma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I addressed all your comments @Sharqiewicz, please have another look.

}

// This type is used to represent all networks that can be used as a source or destination in the system.
export type EvmNetworks =
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I missed that. I decided to stick to the other type though where the EvmNetworks are handpicked. This way, we can easily add more other networks to the Networks enum without them accidentally becoming available on the EvmNetworks enum too (which would be the case if we use an Omit for this type).

@ebma ebma merged commit 80c89a4 into staging Jun 27, 2025
5 checks passed
@ebma ebma deleted the 719-add-option-to-buy-or-sell-eth branch June 27, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add option to Buy or Sell ETH

3 participants