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

feat(wallet): migrate 0x Swap API to v2 #26231

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

onyb
Copy link
Member

@onyb onyb commented Oct 25, 2024

Resolves brave/brave-browser#41891

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

See STR in linked issue

@onyb onyb requested review from a team as code owners October 25, 2024 15:55
@github-actions github-actions bot added CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet feature/web3/wallet/core chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch puLL-Merge labels Oct 25, 2024
Copy link
Contributor

@Douglashdaniel Douglashdaniel left a comment

Choose a reason for hiding this comment

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

++

@@ -448,74 +448,71 @@ union SwapTransactionParamsUnion {

union SwapQuoteUnion {
JupiterQuote jupiter_quote;
ZeroExQuote zero_ex_quote;
ZeroExQuoteInfo zero_ex_quote;
Copy link
Contributor

Choose a reason for hiding this comment

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

should we change this to zero_ex_quote_info to distinguish from ZeroExQuote? it seems ZeroExQuoteInfo now contains an optional of ZeroExQuote

Copy link
Member Author

Choose a reason for hiding this comment

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

The refactoring here should fix this.

Comment on lines 1009 to 1010
// 0x specific validations
if (quoteUnion?.zeroExQuote?.liquidityAvailable === false) {
return 'insufficientLiquidity'
}

if (quoteErrorUnion?.zeroExError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, shouldn't quoteErrorUnion.zeroExError. isInsufficientLiquidity == true?
checking both QuoteUnion and QuoteErrorUnion for errors seems odd to me. Also the quoteErrorUnion.zeroExError.isInsufficientAllowance case seems to be ignored.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was actually done because 0x API v2 now returns isInsufficientLiquidity as part of the quote instead of an error response. But now that I think about it, retaining the old interface is cleaner. You can find this change here: review(nuo-xu): reuse ZeroExQuote and drop ZeroExQuoteInfo struct

Also the quoteErrorUnion.zeroExError.isInsufficientAllowance case seems to be ignored

Allowance checks are no longer returned as part of the error response from the API. If this changes in future, will add this back. Meanwhile, the desktop code already handles allowance checks just like LiFi and Squid.

@github-actions github-actions bot removed the chromium-version-mismatch The Chromium version on the PR branch does not match the version on the target branch label Oct 30, 2024
Copy link
Contributor

[puLL-Merge] - brave/brave-core@26231

Description

This PR updates the implementation of the 0x (ZeroEx) swap functionality in the Brave Wallet. It adapts the code to use a new version of the 0x API, which includes changes in the request and response formats, as well as modifications to the supported networks and the way fees are handled.

Changes

Changes

  1. brave_wallet_constants.cc and brave_wallet_constants.h:

    • Added new constants for 0x API version and allowance holder addresses.
    • Removed individual network-specific API URLs in favor of a single base URL.
    • Added a function to get the allowance holder address for a given chain ID.
  2. swap_response_parser.cc and swap_response_parser.h:

    • Updated parsing logic for 0x quote and transaction responses to match the new API format.
    • Removed parsing for deprecated fields and added parsing for new fields like route and liquidityAvailable.
  3. swap_service.cc and swap_service.h:

    • Updated URL construction for 0x API calls to use the new endpoints and parameters.
    • Modified chain ID support logic to use the new allowance holder addresses.
    • Updated error handling to match the new API error format.
  4. swap_service_unittest.cc:

    • Updated test cases to reflect the changes in API responses and URL construction.
  5. brave_wallet.mojom:

    • Updated data structures to match the new 0x API response format.
  6. useSwap.ts and useZeroEx.ts:

    • Updated error handling and transaction processing to work with the new API response format.
  7. swap.utils.ts:

    • Modified the calculation of price impact and the handling of routing information to work with the new API response format.

Possible Issues

  • The changes to the API format and supported networks may require updates to the user interface to reflect new information or capabilities.
  • There might be a need for additional error handling or edge case considerations with the new API format.

Security Hotspots

  1. The addition of new allowance holder addresses (kZeroExAllowanceHolderCancun, etc.) should be verified to ensure they are correct and trusted.
  2. The removal of the skipValidation parameter in API calls might affect how allowance checks are performed. Ensure this doesn't introduce any security risks.
  3. The change in how fees are calculated and applied (swapFeeBps instead of buyTokenPercentageFee) should be carefully reviewed to ensure it doesn't introduce any unexpected behavior in fee calculations.

These changes represent a significant update to the 0x integration in the Brave Wallet. Careful testing across all supported networks and with various token combinations is recommended to ensure the swap functionality works as expected with the new API version.

@onyb onyb requested a review from nuo-xu October 30, 2024 08:27
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

Copy link
Collaborator

@supermassive supermassive left a comment

Choose a reason for hiding this comment

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

wallet core lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build feature/web3/wallet/core feature/web3/wallet puLL-Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate 0x Swap API to v2
5 participants