feat(LRAPI): add 1inch classic swap rpc #2222

dimxy · 2024-09-16T17:48:55Z

This PR adds:

initial code to connect to 1inch Liquidity Routing API (LRAPI) provider (see new trading_api crate)
two new RPCs to use 1inch classic swap API
'approve' and 'allowance' RPCs (for ERC20 tokens)

dimxy · 2024-09-17T18:01:48Z

mm2src/mm2_main/src/ext_api/one_inch/rpcs.rs

+}
+
+/// "1inch_classic_swap_create" rpc impl
+pub async fn one_inch_v6_0_classic_swap_create_rpc(


Note, that this rpc actually creates a transaction to call the swap aggregation contract. It is supposed that the GUI should sign it and send. We don't verify the tx in any way and trust the 1inch api.

Then lest add this clarification to doc comment

laruh

Great start! Here is the 1st review iteration

mm2src/mm2_main/src/ext_api/one_inch.rs

mm2src/mm2_main/src/ext_api.rs

mm2src/trading_api/src/one_inch_api.rs

mm2src/trading_api/src/one_inch_api/types.rs

mm2src/trading_api/src/lib.rs

mm2src/mm2_main/src/ext_api/one_inch/types.rs

mm2src/trading_api/src/one_inch_api/types.rs

laruh · 2024-09-18T07:22:00Z

mm2src/mm2_main/src/ext_api/one_inch/types.rs

+pub struct ClassicSwapQuoteRequest {
+    pub base: String,
+    pub rel: String,
+    pub amount: BigDecimal,


What do you think about using the U256 type for the "amount" from the beginning? We don’t have a 1inch coin, in wallet or our own trading protocol features, so we are free to use coin-specific types for external third-party trading

I used BigDecimal to make it like most our other rpcs do (for e.g. withdraw).
But now I see I probably should use MmNumber as those classic swap rpcs are close to our "buy" and "sell" methods

I made the swap amount as MmNumber (like in our own swap rpcs). I hope this is consistent

I made the swap amount as MmNumber (like in our own swap rpcs). I hope this is consistent

Hmm, I think yes. We use MmNumber type for atomic swaps and, as 1inch is a trading aggregator, then we can do the same for it.

I will leave comment as unresolved, so other people can find this.

mm2src/mm2_main/src/ext_api/one_inch/rpcs.rs

mariocynicys

Thanks :)
First review iteration.

mm2src/mm2_main/src/ext_api.rs

mm2src/mm2_main/src/ext_api/one_inch.rs

mm2src/mm2_main/src/ext_api/one_inch/types.rs

mm2src/mm2_main/src/ext_api/one_inch/rpcs.rs

mm2src/mm2_main/src/ext_api/one_inch/types.rs

mm2src/mm2_main/src/ext_api/one_inch/rpcs.rs

mm2src/trading_api/src/one_inch_api.rs

mm2src/trading_api/src/one_inch_api/types.rs

mm2src/mm2_main/src/ext_api/one_inch/errors.rs

mm2src/trading_api/src/one_inch_api/client.rs

onur-ozkan · 2024-09-19T10:30:05Z

mm2src/trading_api/src/one_inch_api/types.rs

+    pub fn with_fee(mut self, fee: Option<f32>) -> Self {
+        self.fee = fee;
+        self
+    }
+    pub fn with_protocols(mut self, protocols: Option<String>) -> Self {
+        self.protocols = protocols;
+        self
+    }
+    pub fn with_gas_price(mut self, gas_price: Option<String>) -> Self {
+        self.gas_price = gas_price;
+        self
+    }
+    pub fn with_complexity_level(mut self, complexity_level: Option<u32>) -> Self {
+        self.complexity_level = complexity_level;
+        self
+    }
+    pub fn with_parts(mut self, parts: Option<u32>) -> Self {
+        self.parts = parts;
+        self
+    }
+    pub fn with_main_route_parts(mut self, main_route_parts: Option<u32>) -> Self {
+        self.main_route_parts = main_route_parts;
+        self
+    }
+    pub fn with_gas_limit(mut self, gas_limit: Option<u128>) -> Self {
+        self.gas_limit = gas_limit;
+        self
+    }
+    pub fn with_include_tokens_info(mut self, include_tokens_info: Option<bool>) -> Self {
+        self.include_tokens_info = include_tokens_info;
+        self
+    }
+    pub fn with_include_protocols(mut self, include_protocols: Option<bool>) -> Self {
+        self.include_protocols = include_protocols;
+        self
+    }
+    pub fn with_include_gas(mut self, include_gas: Option<bool>) -> Self {
+        self.include_gas = include_gas;
+        self
+    }
+    pub fn with_connector_tokens(mut self, connector_tokens: Option<String>) -> Self {
+        self.connector_tokens = connector_tokens;
+        self
+    }


I am not a fan of this pattern personally, but if we really want to do this I guess the best way would be creating macro which implements this pattern automatically without taking too much space in the module.

I am not a fan of this pattern personally, but if we really want to do this I guess the best way would be creating macro which implements this pattern automatically without taking too much space in the module.

really like this idea

made a macro, great idea, ty

#2222 (comment)

borngraced

great work! couple notes for first review

mm2src/trading_api/src/one_inch_api/types.rs

mm2src/trading_api/src/one_inch_api/client.rs

onur-ozkan · 2024-09-24T08:44:20Z

mm2src/coins/eth.rs

@@ -866,6 +852,8 @@ impl EthCoinImpl {
        let guard = self.erc20_tokens_infos.lock().unwrap();
        (*guard).clone()
    }
+
+    pub fn chain_id(&self) -> u64 { self.chain_id }


Suggested change

pub fn chain_id(&self) -> u64 { self.chain_id }

#[inline(always)]

pub fn chain_id(&self) -> u64 { self.chain_id }

onur-ozkan · 2024-09-24T08:47:17Z

mm2src/coins/eth/eth_rpc.rs

    /// Get chain id
-    pub(crate) async fn chain_id(&self) -> Result<U256, web3::Error> {
+    pub(crate) async fn network_chain_id(&self) -> Result<U256, web3::Error> {


nit: I guess we should either remove the doc-comment (as function name is already clear) or update it with "Get network chain id".

onur-ozkan · 2024-09-24T08:50:04Z

mm2src/coins/lp_coins.rs

+    #[display(fmt = "No such coin {}", coin)]
+    NoSuchCoin { coin: String },
+    #[display(fmt = "Coin not supported {}", coin)]
+    CoinNotSupported { coin: String },
+    #[from_stringify("NumConversError")]
+    #[display(fmt = "Invalid param: {}", _0)]
+    InvalidParam(String),


nit: I would use the same pattern from NoSuchCoin and CoinNotSupported for InvalidParam (e.g., InvalidParam { param: String }) to make it more explicit. Right now it's not clear what this error takes as a String value.

onur-ozkan · 2024-09-24T08:50:35Z

mm2src/coins/lp_coins.rs

+/// Call allowance for ERC20 tokens
+/// Returns BigDecimal value


Suggested change

/// Call allowance for ERC20 tokens

/// Returns BigDecimal value

/// Call allowance for ERC20 tokens.

/// Returns BigDecimal value.

Can we add a ref link (like this) for "allowance" and "approve"?

onur-ozkan · 2024-09-24T08:51:33Z

mm2src/coins/lp_coins.rs

+/// Call approve for ERC20 coins
+/// Returns signed transaction to send to the chain


Suggested change

/// Call approve for ERC20 coins

/// Returns signed transaction to send to the chain

/// Call approve for ERC20 coins.

/// Returns signed transaction to send to the chain.

onur-ozkan · 2024-09-24T09:03:33Z

mm2src/coins/lp_coins.rs

+    let coin = lp_coinfind_or_err(&ctx, &req.coin)
+        .await
+        .mm_err(|_| Erc20CallError::NoSuchCoin { coin: req.coin.clone() })?;
+    match coin {
+        MmCoinEnum::EthCoin(eth_coin) => {
+            let wei = eth_coin.allowance(req.spender).compat().await?;
+            let amount = u256_to_big_decimal(wei, eth_coin.decimals())?;
+            Ok(amount)
+        },
+        _ => Err(MmError::new(Erc20CallError::CoinNotSupported { coin: req.coin })),
+    }


Suggested change

let coin = lp_coinfind_or_err(&ctx, &req.coin)

.await

.mm_err(|_| Erc20CallError::NoSuchCoin { coin: req.coin.clone() })?;

match coin {

MmCoinEnum::EthCoin(eth_coin) => {

let wei = eth_coin.allowance(req.spender).compat().await?;

let amount = u256_to_big_decimal(wei, eth_coin.decimals())?;

Ok(amount)

},

_ => Err(MmError::new(Erc20CallError::CoinNotSupported { coin: req.coin })),

}

match lp_coinfind_or_err(&ctx, &req.coin).await {

Ok(MmCoinEnum::EthCoin(eth_coin)) => {

let wei = eth_coin.allowance(req.spender).compat().await?;

let amount = u256_to_big_decimal(wei, eth_coin.decimals())?;

Ok(amount)

},

Ok(_) => Err(MmError::new(Erc20CallError::CoinNotSupported { coin: req.coin })),

Err(_) => Err(MmError::new(Erc20CallError::NoSuchCoin { coin: req.coin.clone() })),

}

onur-ozkan · 2024-09-24T09:04:03Z

mm2src/coins/lp_coins.rs

+    let coin = lp_coinfind_or_err(&ctx, &req.coin)
+        .await
+        .mm_err(|_| Erc20CallError::NoSuchCoin { coin: req.coin.clone() })?;


The suggestion above could be applied to here as well.

onur-ozkan · 2024-09-24T09:04:48Z

mm2src/coins/lp_coins.rs

+type Erc20AllowanceResult = MmResult<BigDecimal, Erc20CallError>;
+type Erc20ApproveResult = MmResult<BytesJson, Erc20CallError>;


Actual results are already quite clear, why is this needed?

mariocynicys

Thanks!
LGTM :)

dimxy added 3 commits September 16, 2024 21:15

change wei to big decimal macro to fn (for easier using in other crates)

14e695d

add chain_id from conf fn

aad3739

add code to connect 1inch api, add rpcs to use classic swap api

5df753b

dimxy added the in progress Changes will be made from the author label Sep 16, 2024

dimxy commented Sep 17, 2024

View reviewed changes

laruh requested changes Sep 18, 2024

View reviewed changes

dimxy added under review and removed in progress Changes will be made from the author labels Sep 18, 2024

mariocynicys reviewed Sep 18, 2024

View reviewed changes

onur-ozkan reviewed Sep 19, 2024

View reviewed changes

dimxy added 3 commits September 20, 2024 17:14

refactor on review notes

57192ca

added doc comment for swap params

1c31a7a

refactor on review notes (more)

3610147

borngraced requested changes Sep 21, 2024

View reviewed changes

mm2src/trading_api/src/one_inch_api/types.rs Outdated Show resolved Hide resolved

mm2src/trading_api/src/one_inch_api/client.rs Outdated Show resolved Hide resolved

mm2src/trading_api/src/one_inch_api/client.rs Outdated Show resolved Hide resolved

dimxy added 2 commits September 22, 2024 22:14

refactor on review notes (more)

23d8f1d

fix fmt

18466fc

onur-ozkan reviewed Sep 24, 2024

View reviewed changes

mariocynicys approved these changes Sep 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(LRAPI): add 1inch classic swap rpc #2222

feat(LRAPI): add 1inch classic swap rpc #2222

dimxy commented Sep 16, 2024

dimxy Sep 17, 2024

laruh Sep 20, 2024

dimxy Sep 20, 2024

laruh left a comment

laruh Sep 18, 2024

dimxy Sep 18, 2024

dimxy Sep 20, 2024

laruh Sep 22, 2024 •

edited

Loading

mariocynicys left a comment

onur-ozkan Sep 19, 2024

laruh Sep 20, 2024

dimxy Sep 20, 2024

borngraced Sep 21, 2024

borngraced left a comment

onur-ozkan Sep 24, 2024

onur-ozkan Sep 24, 2024

onur-ozkan Sep 24, 2024

onur-ozkan Sep 24, 2024

onur-ozkan Sep 24, 2024

onur-ozkan Sep 24, 2024

onur-ozkan Sep 24, 2024

onur-ozkan Sep 24, 2024

onur-ozkan Sep 24, 2024

mariocynicys left a comment

	pub fn chain_id(&self) -> u64 { self.chain_id }
	#[inline(always)]
	pub fn chain_id(&self) -> u64 { self.chain_id }

		/// Call allowance for ERC20 tokens
		/// Returns BigDecimal value

		/// Call approve for ERC20 coins
		/// Returns signed transaction to send to the chain

		type Erc20AllowanceResult = MmResult<BigDecimal, Erc20CallError>;
		type Erc20ApproveResult = MmResult<BytesJson, Erc20CallError>;

feat(LRAPI): add 1inch classic swap rpc #2222

Are you sure you want to change the base?

feat(LRAPI): add 1inch classic swap rpc #2222

Conversation

dimxy commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laruh Sep 22, 2024 • edited Loading

Choose a reason for hiding this comment

mariocynicys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borngraced left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariocynicys left a comment

Choose a reason for hiding this comment

laruh Sep 22, 2024 •

edited

Loading