-
Notifications
You must be signed in to change notification settings - Fork 94
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(LRAPI): add 1inch classic swap rpc #2222
base: dev
Are you sure you want to change the base?
Conversation
} | ||
|
||
/// "1inch_classic_swap_create" rpc impl | ||
pub async fn one_inch_v6_0_classic_swap_create_rpc( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then lest add this clarification to doc comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start! Here is the 1st review iteration
pub struct ClassicSwapQuoteRequest { | ||
pub base: String, | ||
pub rel: String, | ||
pub amount: BigDecimal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the swap amount as MmNumber (like in our own swap rpcs). I hope this is consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks :)
First review iteration.
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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made a macro, great idea, ty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work! couple notes for first review
@@ -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 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub fn chain_id(&self) -> u64 { self.chain_id } | |
#[inline(always)] | |
pub fn chain_id(&self) -> u64 { self.chain_id } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
LGTM :)
@dimxy please fix branch conflicts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! First review iteration from my side where I gave the types.rs
a first look.
* dev: fix(cosmos): fix tx broadcasting error (#2238) chore(solana): remove solana implementation (#2239) chore(cli): remove leftover subcommands from help message (#2235) fix(orders): fix cancel order race condition using time-based cache (#2232) fix(legacy-swap): taker failed spend maker payment marked as failed (#2199) chore(adex-cli): deprecate adex-cli (#2234) feat(new-RPC): connection healthcheck implementation for peers (#2194) fix(proxy-signature): add message lifetime overflows (#2233) feat(CI): handle remote files in a safer way (#2217) chore(doc): update issue address in README (#2227) fix(merge): remove duplicated db_root function (#2229) feat(wallets): add `get_wallet_names` rpc (#2202) chore(tests): don't use `.wait()` and use `block_on` instead (#2220) fix(native-rpc): remove escaped response body (#2219) fix(clippy): fix coins mod clippy warnings in wasm (#2224) feat(core): handling CTRL-C signal with graceful shutdown (#2213) docs(README): fix typos (#2212) remove the non-sense arguments (#2216) fix(db): stop creating the all-zeroes dir on KDF start (#2218)
In the recent 5 commits I fixed most review notes above, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last notes from me
impl TxFields { | ||
pub(crate) fn from_api_value( | ||
tx_fields: one_inch_api::types::TxFields, | ||
decimals: u8, | ||
) -> MmResult<Self, FromApiValueError> { | ||
Ok(Self { | ||
from: tx_fields.from, | ||
to: tx_fields.to, | ||
data: BytesJson::from(hex::decode(str_strip_0x!(tx_fields.data.as_str()))?), | ||
value: u256_to_big_decimal(U256::from_dec_str(&tx_fields.value)?, decimals)?, | ||
gas_price: wei_to_gwei_decimal(U256::from_dec_str(&tx_fields.gas_price)?)?, | ||
gas: tx_fields.gas, | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn from_api_tx_fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
fn validate_slippage(slippage: f32) -> MmResult<(), ApiClientError> { | ||
if !(0.0..=ONE_INCH_MAX_SLIPPAGE).contains(&slippage) { | ||
return Err(ApiClientError::OutOfBounds { | ||
param: "slippage".to_owned(), | ||
value: slippage.to_string(), | ||
min: 0.0.to_string(), | ||
max: ONE_INCH_MAX_SLIPPAGE.to_string(), | ||
} | ||
.into()); | ||
} | ||
Ok(()) | ||
} | ||
|
||
fn validate_fee(fee: &Option<f32>) -> MmResult<(), ApiClientError> { | ||
if let Some(fee) = fee { | ||
if !(0.0..=ONE_INCH_MAX_FEE_SHARE).contains(fee) { | ||
return Err(ApiClientError::OutOfBounds { | ||
param: "fee".to_owned(), | ||
value: fee.to_string(), | ||
min: 0.0.to_string(), | ||
max: ONE_INCH_MAX_FEE_SHARE.to_string(), | ||
} | ||
.into()); | ||
} | ||
} | ||
Ok(()) | ||
} | ||
|
||
fn validate_gas_limit(gas_limit: &Option<u128>) -> MmResult<(), ApiClientError> { | ||
if let Some(gas_limit) = gas_limit { | ||
if gas_limit > &ONE_INCH_MAX_GAS { | ||
return Err(ApiClientError::OutOfBounds { | ||
param: "gas_limit".to_owned(), | ||
value: gas_limit.to_string(), | ||
min: 0.to_string(), | ||
max: ONE_INCH_MAX_GAS.to_string(), | ||
} | ||
.into()); | ||
} | ||
} | ||
Ok(()) | ||
} | ||
|
||
fn validate_parts(parts: &Option<u32>) -> MmResult<(), ApiClientError> { | ||
if let Some(parts) = parts { | ||
if parts > &ONE_INCH_MAX_PARTS { | ||
return Err(ApiClientError::OutOfBounds { | ||
param: "parts".to_owned(), | ||
value: parts.to_string(), | ||
min: 0.to_string(), | ||
max: ONE_INCH_MAX_PARTS.to_string(), | ||
} | ||
.into()); | ||
} | ||
} | ||
Ok(()) | ||
} | ||
|
||
fn validate_main_route_parts(main_route_parts: &Option<u32>) -> MmResult<(), ApiClientError> { | ||
if let Some(main_route_parts) = main_route_parts { | ||
if main_route_parts > &ONE_INCH_MAX_MAIN_ROUTE_PARTS { | ||
return Err(ApiClientError::OutOfBounds { | ||
param: "main route parts".to_owned(), | ||
value: main_route_parts.to_string(), | ||
min: 0.to_string(), | ||
max: ONE_INCH_MAX_MAIN_ROUTE_PARTS.to_string(), | ||
} | ||
.into()); | ||
} | ||
} | ||
Ok(()) | ||
} | ||
|
||
fn validate_complexity_level(complexity_level: &Option<u32>) -> MmResult<(), ApiClientError> { | ||
if let Some(complexity_level) = complexity_level { | ||
if complexity_level > &ONE_INCH_MAX_COMPLEXITY_LEVEL { | ||
return Err(ApiClientError::OutOfBounds { | ||
param: "complexity level".to_owned(), | ||
value: complexity_level.to_string(), | ||
min: 0.to_string(), | ||
max: ONE_INCH_MAX_COMPLEXITY_LEVEL.to_string(), | ||
} | ||
.into()); | ||
} | ||
} | ||
Ok(()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can move validation logics inside fn validate_params
if you don't plan to reuse them. This will also reduce boilerplate code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh I'd like to have an option to break functions like that (even if those functions are not reused), for code structuring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its all good. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this feat covers WASM? no test coverage 🙂 ... otherwise, LGTM 🚀
it works in native and wasm, basically this is forward to 3party api. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes! Next review iteration!
#[serde(rename = "isFoT", default)] | ||
pub is_fot: bool, | ||
#[serde(rename = "logoURI")] | ||
pub logo_uri: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is to be used by GUI in anyway, it should be checked for phishing links, same for any field that returns a url. I suppose it will not be used as the token logo will be retrieved when adding it as a custom token using the custom token import APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in d457bee
pub decimals: u32, | ||
pub eip2612: bool, | ||
#[serde(rename = "isFoT", default)] | ||
pub is_fot: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To think about in future PRs as well, should deduct this fee from the amount when routing a swap (1inch then cross chain atomic swap) so the atomic swap will use the amount minus this fee.
mm2src/coins/lp_coins.rs
Outdated
let eth_coin = find_erc20_eth_coin(&ctx, &req.coin).await?; | ||
let amount = wei_from_big_decimal(&req.amount, eth_coin.decimals())?; | ||
let tx = eth_coin.approve(req.spender, amount).compat().await?; | ||
Ok(tx.tx_hash_as_bytes()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be formatted
in a right way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would return a result like that
{
"mmrpc": "2.0",
"result": "5136701f11060010841c9708c3eb26f6606a070b8ae43f4b98b6d7b10a545258",
"id": null
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should include the 0x
prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not fixed yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 65c4853
I can see couple places where 0x is not added though: send_raw_tx in eth and TransactionNftDetails
We apparently need a dedicated to_string conversion fn for tx_hash.
(But is it convenient for GUI when we return tx hash in different formats for different coins?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But is it convenient for GUI when we return tx hash in different formats for different coins
No, we should fix those cases (in a different PR of course). tx hashes and addresses should always be displayed/sent to GUIs with 0x
prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes! Last review from my side, please also fix any unfixed comment in previous review.
const ONE_INCH_V6_0_SUPPORTED_CHAINS: &[(&str, u64)] = &[ | ||
("Ethereum", 1), | ||
("Optimism", 10), | ||
("BSC", 56), | ||
("Gnosis", 100), | ||
("Polygon", 137), | ||
("Fantom", 250), | ||
("ZkSync", 324), | ||
("Klaytn", 8217), | ||
("Base", 8453), | ||
("Arbitrum", 42161), | ||
("Avalanche", 43114), | ||
("Aurora", 1313161554), | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this in case they add support for new chains, in next PRs you should find a way to make this configurable or look for an API that provides this info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought of adding this into a config file (but did not find suitable one).
For the other hand, if the QA team has to add another chain they would still need to make a PR to the KDF, so maybe they could modify this const as well(?)
(Besides, I think if 1inch ever adds a new chain they will probably have to make a new version of their API, so we would need to modify the code)
… check for same chain for 1inch swap, move 1inch to 'lp_commands/rpc')
* dev: fix(foot-shooting): remove leftover code that panics via RPC (#2270) refactor(MarketCoinOps): make `wait_for_htlc_tx_spend` async (#2265) feat(eth-swap): maker tpu v2 implementation (#2211) fix(nft): add token_id field to the tx history primary key, fix balance (#2209) feat(cosmos): support IBC types in tx history implementation (#2245) fix(hd-wallet): use `CoinBalanceMap` for UTXO and QTUM (#2259) fix(tests): add more sepolia endpoints in tests (#2262) fix(legacy-swap): check for confirmations on recover taker (#2242) fix(legacy-swap): remove the need for takers to confirm their payment (#2249) refactor(P2P): types and modules (#2256) fix(evm): correctly display eth addr in iguana v2 activation result (#2254) feat(utxo): prioritize electrum connections (#1966) refactor(SwapOps): make all methods async (#2251) refactor(SwapOps): make `send_maker_payment` async (#2250) remove old p2p implementation (#2248) feat(cosmos-offline-tests): prepare IBC channels inside the container (#2246)
Ok(()) | ||
} else { | ||
Err(MmError::new(ApiIntegrationRpcError::ChainNotSupported)) | ||
fn api_supports_coin(base: &EthCoin, rel: &EthCoin) -> MmResult<(), ApiIntegrationRpcError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should be called api_supports_pair
mm2src/coins/lp_coins.rs
Outdated
let eth_coin = find_erc20_eth_coin(&ctx, &req.coin).await?; | ||
let amount = wei_from_big_decimal(&req.amount, eth_coin.decimals())?; | ||
let tx = eth_coin.approve(req.spender, amount).compat().await?; | ||
Ok(tx.tx_hash_as_bytes()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But is it convenient for GUI when we return tx hash in different formats for different coins
No, we should fix those cases (in a different PR of course). tx hashes and addresses should always be displayed/sent to GUIs with 0x
prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fixes! LGTM!
Please resolve conflicts, I also have one small comment :)
@dimxy please open docs PR for this features |
This PR adds:
(This is an ongoing work to add features requested in #1287 in the part regarding "AMM" tech adoption. We need a dedicated issue for that)
TODO: