Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix support CALL moonpay sell #8323

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

sarneijim
Copy link
Contributor

@sarneijim sarneijim commented Nov 6, 2024

βœ… Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • ...

πŸ“ Description

Moonpay sell is rejected. Move from name to id.

Source of true CAL

❓ Context


🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

Copy link

vercel bot commented Nov 6, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Comments Updated (UTC)
web-tools βœ… Ready (Inspect) Visit Preview πŸ’¬ Add feedback Nov 7, 2024 4:05pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Nov 7, 2024 4:05pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Nov 7, 2024 4:05pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Nov 7, 2024 4:05pm

@live-github-bot live-github-bot bot added the common Has changes in live-common label Nov 6, 2024
@sarneijim sarneijim marked this pull request as ready for review November 6, 2024 16:57
@sarneijim sarneijim requested review from a team as code owners November 6, 2024 16:57
Comment on lines -40 to +47
return getFundProvider(providerName.toLowerCase());
return getFundProvider(provider.toLowerCase());

case ExchangeTypes.Sell:
case ExchangeTypes.SellNg:
return await getSellProvider(providerName.toLowerCase());
return await getSellProvider(provider.toLowerCase());

default:
throw new Error(`Unknown partner ${providerName} type`);
throw new Error(`Unknown partner ${provider} type`);
Copy link
Member

Choose a reason for hiding this comment

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

Why are you still lowerCasing here and why rename to provider? is it the providerName or now the providerId ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provider ID is derived from the old provider's name, converted to lowercase to match the ID format.
However, there are NEW cases where the provider's name varies slightly; for example, "moonpay-sell" has a different public key than "moonpay" (moonpay for swap").

Comment on lines -34 to +35
const [sellProvidersData, fundProviderData] = await Promise.all([
getProvidersData("sell"),
getProvidersData("fund"), // Mercuryo is currently treated as a fund provider
]);
providerDataCache = { ...sellProvidersData, ...fundProviderData };
return providerDataCache;
const sellProvidersData = await getProvidersData("sell");
return { ...sellProvidersData };
Copy link
Member

Choose a reason for hiding this comment

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

The cache was introduced because old swaps spammed this network call.
With Live apps I can understand you want to remove this. But if you stop using just remove all of its reference in the file

Copy link
Contributor Author

@sarneijim sarneijim Nov 7, 2024

Choose a reason for hiding this comment

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

Happy to delete it!

@@ -18,32 +18,23 @@ const testSellProvider: ExchangeProviderNameAndSignature = {
version: 2,
};

let providerDataCache: Record<string, ExchangeProviderNameAndSignature> | null = null;

/**
* The result is cached after the first successful fetch to avoid redundant network calls.
* - "fund" providers currently include Mercuryo, which is stored as a fund provider.
Copy link

Choose a reason for hiding this comment

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

This does not seem true anymore

Copy link
Member

Choose a reason for hiding this comment

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

what is not true anymore? it was not set in Sara's PR anymore so better remove it

Copy link

@fsamier fsamier Nov 7, 2024

Choose a reason for hiding this comment

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

Mercuryo is correctly identified as a sell provider, I think Sara update/removed the logic but didn't change the comment

Copy link
Member

Choose a reason for hiding this comment

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

ahhh yes removing also the comment ok got it

@@ -18,32 +18,23 @@ const testSellProvider: ExchangeProviderNameAndSignature = {
version: 2,
};

let providerDataCache: Record<string, ExchangeProviderNameAndSignature> | null = null;

/**
* The result is cached after the first successful fetch to avoid redundant network calls.
* - "fund" providers currently include Mercuryo, which is stored as a fund provider.
* If this behavior changes, this logic should be revisited.
* Reference: https://github.dev/LedgerHQ/crypto-assets/blob/main/assets/partners/mercuryo/common.json
Copy link

@fsamier fsamier Nov 7, 2024

Choose a reason for hiding this comment

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

Link is broken after CAL redesign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Has changes in live-common
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants