From 1bbee57c1c8414b6fb3fe409da02134df3741f56 Mon Sep 17 00:00:00 2001 From: Anirudha Bose Date: Wed, 30 Oct 2024 13:07:36 +0530 Subject: [PATCH] review(nuo-xu): reuse ZeroExQuote and drop ZeroExQuoteInfo struct --- .../browser/swap_response_parser.cc | 17 +++-- .../browser/swap_response_parser.h | 4 +- .../browser/swap_response_parser_unittest.cc | 69 ++++++++---------- .../brave_wallet/browser/swap_service.cc | 12 ++++ .../browser/swap_service_unittest.cc | 70 +++++++++++++------ .../brave_wallet/common/brave_wallet.mojom | 9 +-- .../page/screens/swap/hooks/useSwap.ts | 18 ++--- 7 files changed, 114 insertions(+), 85 deletions(-) diff --git a/components/brave_wallet/browser/swap_response_parser.cc b/components/brave_wallet/browser/swap_response_parser.cc index b51e9acfff39..0ff8efa859fd 100644 --- a/components/brave_wallet/browser/swap_response_parser.cc +++ b/components/brave_wallet/browser/swap_response_parser.cc @@ -141,8 +141,8 @@ mojom::ZeroExQuotePtr ParseQuote( } // namespace -mojom::ZeroExQuoteInfoPtr ParseQuoteResponse(const base::Value& json_value, - const std::string& chain_id) { +mojom::ZeroExQuotePtr ParseQuoteResponse(const base::Value& json_value, + const std::string& chain_id) { // { // "blockNumber": "20114692", // "buyAmount": "100037537", @@ -217,21 +217,20 @@ mojom::ZeroExQuoteInfoPtr ParseQuoteResponse(const base::Value& json_value, return nullptr; } - auto swap_response = mojom::ZeroExQuoteInfo::New(); - swap_response->allowance_target = - GetZeroExAllowanceHolderAddress(chain_id).value_or(""); + auto swap_response = mojom::ZeroExQuote::New(); + if (!swap_response_value->liquidity_available) { swap_response->liquidity_available = false; return swap_response; } - if (auto quote = ParseQuote(swap_response_value.value())) { - swap_response->quote = std::move(quote); - } else { + swap_response = ParseQuote(swap_response_value.value()); + if (!swap_response) { return nullptr; } - swap_response->liquidity_available = swap_response_value->liquidity_available; + swap_response->allowance_target = + GetZeroExAllowanceHolderAddress(chain_id).value_or(""); return swap_response; } diff --git a/components/brave_wallet/browser/swap_response_parser.h b/components/brave_wallet/browser/swap_response_parser.h index 36140791b4d1..0acb01656120 100644 --- a/components/brave_wallet/browser/swap_response_parser.h +++ b/components/brave_wallet/browser/swap_response_parser.h @@ -15,8 +15,8 @@ namespace brave_wallet { namespace zeroex { -mojom::ZeroExQuoteInfoPtr ParseQuoteResponse(const base::Value& json_value, - const std::string& chain_id); +mojom::ZeroExQuotePtr ParseQuoteResponse(const base::Value& json_value, + const std::string& chain_id); mojom::ZeroExTransactionPtr ParseTransactionResponse( const base::Value& json_value); mojom::ZeroExErrorPtr ParseErrorResponse(const base::Value& json_value); diff --git a/components/brave_wallet/browser/swap_response_parser_unittest.cc b/components/brave_wallet/browser/swap_response_parser_unittest.cc index 53ac73fdd90b..d701380cfae9 100644 --- a/components/brave_wallet/browser/swap_response_parser_unittest.cc +++ b/components/brave_wallet/browser/swap_response_parser_unittest.cc @@ -153,40 +153,37 @@ TEST(SwapResponseParserUnitTest, ParseZeroExQuoteResponse) { "zid": "0x111111111111111111111111" } )"); - auto quote_info = + auto quote = zeroex::ParseQuoteResponse(ParseJson(json), mojom::kMainnetChainId); - ASSERT_TRUE(quote_info); - ASSERT_TRUE(quote_info->quote); + ASSERT_TRUE(quote); - EXPECT_EQ(quote_info->quote->buy_amount, "100032748"); - EXPECT_EQ(quote_info->quote->buy_token, - "0xdac17f958d2ee523a2206206994597c13d831ec7"); + EXPECT_EQ(quote->buy_amount, "100032748"); + EXPECT_EQ(quote->buy_token, "0xdac17f958d2ee523a2206206994597c13d831ec7"); - ASSERT_TRUE(quote_info->quote->fees->zero_ex_fee); - EXPECT_EQ(quote_info->quote->fees->zero_ex_fee->amount, "0"); - EXPECT_EQ(quote_info->quote->fees->zero_ex_fee->token, "0xdeadbeef"); - EXPECT_EQ(quote_info->quote->fees->zero_ex_fee->type, "volume"); + ASSERT_TRUE(quote->fees->zero_ex_fee); + EXPECT_EQ(quote->fees->zero_ex_fee->amount, "0"); + EXPECT_EQ(quote->fees->zero_ex_fee->token, "0xdeadbeef"); + EXPECT_EQ(quote->fees->zero_ex_fee->type, "volume"); - EXPECT_EQ(quote_info->quote->gas, "288095"); - EXPECT_EQ(quote_info->quote->gas_price, "7062490000"); - EXPECT_EQ(quote_info->quote->liquidity_available, true); - EXPECT_EQ(quote_info->quote->min_buy_amount, "99032421"); + EXPECT_EQ(quote->gas, "288095"); + EXPECT_EQ(quote->gas_price, "7062490000"); + EXPECT_EQ(quote->liquidity_available, true); + EXPECT_EQ(quote->min_buy_amount, "99032421"); - ASSERT_EQ(quote_info->quote->route->fills.size(), 1UL); - EXPECT_EQ(quote_info->quote->route->fills.at(0)->from, + ASSERT_EQ(quote->route->fills.size(), 1UL); + EXPECT_EQ(quote->route->fills.at(0)->from, "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48"); - EXPECT_EQ(quote_info->quote->route->fills.at(0)->to, + EXPECT_EQ(quote->route->fills.at(0)->to, "0xdac17f958d2ee523a2206206994597c13d831ec7"); - EXPECT_EQ(quote_info->quote->route->fills.at(0)->source, "SolidlyV3"); - EXPECT_EQ(quote_info->quote->route->fills.at(0)->proportion_bps, "10000"); + EXPECT_EQ(quote->route->fills.at(0)->source, "SolidlyV3"); + EXPECT_EQ(quote->route->fills.at(0)->proportion_bps, "10000"); - EXPECT_EQ(quote_info->quote->sell_amount, "100000000"); - EXPECT_EQ(quote_info->quote->sell_token, - "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48"); - EXPECT_EQ(quote_info->quote->total_network_fee, "2034668056550000"); + EXPECT_EQ(quote->sell_amount, "100000000"); + EXPECT_EQ(quote->sell_token, "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48"); + EXPECT_EQ(quote->total_network_fee, "2034668056550000"); - EXPECT_EQ(quote_info->liquidity_available, true); - EXPECT_EQ(quote_info->allowance_target, + EXPECT_EQ(quote->liquidity_available, true); + EXPECT_EQ(quote->allowance_target, "0x0000000000001fF3684f28c67538d4D072C22734"); // Case 2: null zeroExFee @@ -253,12 +250,11 @@ TEST(SwapResponseParserUnitTest, ParseZeroExQuoteResponse) { "zid": "0x111111111111111111111111" } )"; - quote_info = - zeroex::ParseQuoteResponse(ParseJson(json), mojom::kMainnetChainId); - ASSERT_TRUE(quote_info); - EXPECT_FALSE(quote_info->quote->fees->zero_ex_fee); - EXPECT_EQ(quote_info->liquidity_available, true); - EXPECT_EQ(quote_info->allowance_target, + quote = zeroex::ParseQuoteResponse(ParseJson(json), mojom::kMainnetChainId); + ASSERT_TRUE(quote); + EXPECT_FALSE(quote->fees->zero_ex_fee); + EXPECT_EQ(quote->liquidity_available, true); + EXPECT_EQ(quote->allowance_target, "0x0000000000001fF3684f28c67538d4D072C22734"); // Case 3: malformed fees field @@ -330,13 +326,10 @@ TEST(SwapResponseParserUnitTest, ParseZeroExQuoteResponse) { "liquidityAvailable": false, } )"; - quote_info = - zeroex::ParseQuoteResponse(ParseJson(json), mojom::kMainnetChainId); - ASSERT_TRUE(quote_info); - EXPECT_FALSE(quote_info->liquidity_available); - EXPECT_EQ(quote_info->allowance_target, - "0x0000000000001fF3684f28c67538d4D072C22734"); - EXPECT_FALSE(quote_info->quote); + quote = zeroex::ParseQuoteResponse(ParseJson(json), mojom::kMainnetChainId); + ASSERT_TRUE(quote); + EXPECT_FALSE(quote->liquidity_available); + EXPECT_EQ(quote->buy_token, ""); // Case 5: other invalid cases json = R"({"totalNetworkFee": "2034668056550000"})"; diff --git a/components/brave_wallet/browser/swap_service.cc b/components/brave_wallet/browser/swap_service.cc index 13ca3d778c78..20a45b0dd746 100644 --- a/components/brave_wallet/browser/swap_service.cc +++ b/components/brave_wallet/browser/swap_service.cc @@ -439,6 +439,18 @@ void SwapService::OnGetZeroExQuote(const std::string& chain_id, if (auto swap_response = zeroex::ParseQuoteResponse( api_request_result.value_body(), chain_id)) { + if (!swap_response->liquidity_available) { + std::move(callback).Run( + nullptr, nullptr, + mojom::SwapErrorUnion::NewZeroExError(mojom::ZeroExError::New( + "INSUFFICIENT_LIQUIDITY", + l10n_util::GetStringUTF8( + IDS_BRAVE_WALLET_SWAP_INSUFFICIENT_LIQUIDITY), + true)), + ""); + return; + } + std::move(callback).Run( mojom::SwapQuoteUnion::NewZeroExQuote(std::move(swap_response)), std::move(swap_fee), nullptr, ""); diff --git a/components/brave_wallet/browser/swap_service_unittest.cc b/components/brave_wallet/browser/swap_service_unittest.cc index dc8ceb232545..0749d1932ae6 100644 --- a/components/brave_wallet/browser/swap_service_unittest.cc +++ b/components/brave_wallet/browser/swap_service_unittest.cc @@ -556,37 +556,36 @@ TEST_F(SwapServiceUnitTest, GetZeroExQuote) { } )"); - auto zero_ex_quote = mojom::ZeroExQuote::New(); - zero_ex_quote->buy_amount = "100032748"; - zero_ex_quote->buy_token = "0xdac17f958d2ee523a2206206994597c13d831ec7"; - zero_ex_quote->sell_amount = "100000000"; - zero_ex_quote->sell_token = "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48"; + auto expected_zero_ex_quote = mojom::ZeroExQuote::New(); + expected_zero_ex_quote->buy_amount = "100032748"; + expected_zero_ex_quote->buy_token = + "0xdac17f958d2ee523a2206206994597c13d831ec7"; + expected_zero_ex_quote->sell_amount = "100000000"; + expected_zero_ex_quote->sell_token = + "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48"; auto zero_ex_fee = mojom::ZeroExFee::New(); zero_ex_fee->type = "volume"; zero_ex_fee->token = "0xdeadbeef"; zero_ex_fee->amount = "0"; - zero_ex_quote->fees = mojom::ZeroExFees::New(); - zero_ex_quote->fees->zero_ex_fee = std::move(zero_ex_fee); + expected_zero_ex_quote->fees = mojom::ZeroExFees::New(); + expected_zero_ex_quote->fees->zero_ex_fee = std::move(zero_ex_fee); - zero_ex_quote->gas = "288095"; - zero_ex_quote->gas_price = "7062490000"; - zero_ex_quote->liquidity_available = true; - zero_ex_quote->min_buy_amount = "99032421"; - zero_ex_quote->total_network_fee = "2034668056550000"; + expected_zero_ex_quote->gas = "288095"; + expected_zero_ex_quote->gas_price = "7062490000"; + expected_zero_ex_quote->liquidity_available = true; + expected_zero_ex_quote->min_buy_amount = "99032421"; + expected_zero_ex_quote->total_network_fee = "2034668056550000"; auto fill = mojom::ZeroExRouteFill::New(); fill->from = "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48"; fill->to = "0xdac17f958d2ee523a2206206994597c13d831ec7"; fill->source = "SolidlyV3"; fill->proportion_bps = "10000"; - zero_ex_quote->route = mojom::ZeroExRoute::New(); - zero_ex_quote->route->fills.push_back(fill.Clone()); + expected_zero_ex_quote->route = mojom::ZeroExRoute::New(); + expected_zero_ex_quote->route->fills.push_back(fill.Clone()); - auto expected_zero_ex_quote_info = mojom::ZeroExQuoteInfo::New(); - expected_zero_ex_quote_info->quote = std::move(zero_ex_quote); - expected_zero_ex_quote_info->liquidity_available = true; - expected_zero_ex_quote_info->allowance_target = + expected_zero_ex_quote->allowance_target = "0x0000000000001fF3684f28c67538d4D072C22734"; auto expected_swap_fees = mojom::SwapFees::New(); @@ -598,7 +597,7 @@ TEST_F(SwapServiceUnitTest, GetZeroExQuote) { base::MockCallback callback; EXPECT_CALL(callback, Run(EqualsMojo(mojom::SwapQuoteUnion::NewZeroExQuote( - expected_zero_ex_quote_info.Clone())), + expected_zero_ex_quote.Clone())), EqualsMojo(expected_swap_fees.Clone()), EqualsMojo(mojom::SwapErrorUnionPtr()), "")); @@ -676,9 +675,9 @@ TEST_F(SwapServiceUnitTest, GetZeroExQuote) { } )"); - expected_zero_ex_quote_info->quote->fees->zero_ex_fee = nullptr; + expected_zero_ex_quote->fees->zero_ex_fee = nullptr; EXPECT_CALL(callback, Run(EqualsMojo(mojom::SwapQuoteUnion::NewZeroExQuote( - std::move(expected_zero_ex_quote_info))), + std::move(expected_zero_ex_quote))), EqualsMojo(expected_swap_fees.Clone()), EqualsMojo(mojom::SwapErrorUnionPtr()), "")); @@ -693,6 +692,7 @@ TEST_F(SwapServiceUnitTest, GetZeroExQuote) { } TEST_F(SwapServiceUnitTest, GetZeroExQuoteError) { + // Case 1: validation error std::string error = R"( { "name": "INPUT_INVALID", @@ -714,6 +714,34 @@ TEST_F(SwapServiceUnitTest, GetZeroExQuoteError) { mojom::SwapProvider::kZeroEx), callback.Get()); task_environment_.RunUntilIdle(); + testing::Mock::VerifyAndClearExpectations(&callback); + + // Case 2: insufficient liquidity + SetInterceptor(R"( + { + "liquidityAvailable": false, + "zid": "0x111111111111111111111111" + } + )"); + auto error_response = mojom::ZeroExError::New(); + error_response->name = "INSUFFICIENT_LIQUIDITY"; + error_response->message = + l10n_util::GetStringUTF8(IDS_BRAVE_WALLET_SWAP_INSUFFICIENT_LIQUIDITY); + error_response->is_insufficient_liquidity = true; + + EXPECT_CALL(callback, Run(EqualsMojo(mojom::SwapQuoteUnionPtr()), + EqualsMojo(mojom::SwapFeesPtr()), + EqualsMojo(mojom::SwapErrorUnion::NewZeroExError( + std::move(error_response))), + "")); + + swap_service_->GetQuote( + GetCannedSwapQuoteParams( + mojom::CoinType::ETH, mojom::kPolygonMainnetChainId, "DAI", + mojom::CoinType::ETH, mojom::kPolygonMainnetChainId, "ETH", + mojom::SwapProvider::kZeroEx), + callback.Get()); + task_environment_.RunUntilIdle(); } TEST_F(SwapServiceUnitTest, GetZeroExQuoteUnexpectedReturn) { diff --git a/components/brave_wallet/common/brave_wallet.mojom b/components/brave_wallet/common/brave_wallet.mojom index 6c43f9aa1e49..cc8fa0c4dd0c 100644 --- a/components/brave_wallet/common/brave_wallet.mojom +++ b/components/brave_wallet/common/brave_wallet.mojom @@ -448,7 +448,7 @@ union SwapTransactionParamsUnion { union SwapQuoteUnion { JupiterQuote jupiter_quote; - ZeroExQuoteInfo zero_ex_quote; + ZeroExQuote zero_ex_quote; LiFiQuote lifi_quote; SquidQuote squid_quote; }; @@ -501,18 +501,15 @@ struct ZeroExQuote { string sell_amount; string sell_token; string total_network_fee; -}; -struct ZeroExQuoteInfo { - // quote is undefined if liquidity_available is false - ZeroExQuote? quote; + // Custom field string allowance_target; - bool liquidity_available; }; struct ZeroExError { string name; string message; + bool is_insufficient_liquidity; }; union SwapErrorUnion { diff --git a/components/brave_wallet_ui/page/screens/swap/hooks/useSwap.ts b/components/brave_wallet_ui/page/screens/swap/hooks/useSwap.ts index d491d12f11c4..a0dab4ff549d 100644 --- a/components/brave_wallet_ui/page/screens/swap/hooks/useSwap.ts +++ b/components/brave_wallet_ui/page/screens/swap/hooks/useSwap.ts @@ -308,9 +308,9 @@ export const useSwap = () => { }) } - if (quoteUnion.zeroExQuote?.quote) { + if (quoteUnion.zeroExQuote) { return getZeroExQuoteOptions({ - quote: quoteUnion.zeroExQuote.quote, + quote: quoteUnion.zeroExQuote, fromNetwork, fromToken, toToken, @@ -565,18 +565,18 @@ export const useSwap = () => { } } - if (quoteResponse.response.zeroExQuote?.quote) { + if (quoteResponse.response.zeroExQuote) { if (params.editingFromOrToAmount === 'from') { setToAmount( getZeroExToAmount({ - quote: quoteResponse.response.zeroExQuote.quote, + quote: quoteResponse.response.zeroExQuote, toToken: params.toToken }).format(6) ) } else { setFromAmount( getZeroExFromAmount({ - quote: quoteResponse.response.zeroExQuote.quote, + quote: quoteResponse.response.zeroExQuote, fromToken: params.fromToken }).format(6) ) @@ -1007,11 +1007,11 @@ export const useSwap = () => { } // 0x specific validations - if (quoteUnion?.zeroExQuote?.liquidityAvailable === false) { - return 'insufficientLiquidity' - } - if (quoteErrorUnion?.zeroExError) { + if (quoteErrorUnion.zeroExError.isInsufficientLiquidity) { + return 'insufficientLiquidity' + } + return 'unknownError' }