-
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
[r2r] Ethereum swap watchers #1601
Conversation
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.
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.
Solid work!
First review iteration:
use common::executor::{AbortSettings, SpawnAbortable, Timer}; | ||
use common::log::{debug, error, info}; | ||
use common::state_machine::prelude::*; | ||
use common::{now_ms, DEX_FEE_ADDR_RAW_PUBKEY}; | ||
use futures::compat::Future01CompatExt; | ||
use mm2_core::mm_ctx::MmArc; | ||
use mm2_libp2p::{decode_signed, pub_sub_topic, TopicPrefix}; | ||
use rpc::v1::types::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.
@@ -94,6 +96,7 @@ pub struct TakerSwapWatcherData { | |||
pub maker_pub: Vec<u8>, | |||
pub maker_payment_hash: Vec<u8>, | |||
pub maker_coin_start_block: u64, | |||
pub swap_contract_address: Option<Vec<u8>>, |
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 use Option<BytesJson>
here. Right now we are doing extra mappings for no reason.
"urls": ["http://195.201.0.6:8565"], | ||
"swap_contract_address": "0xa09ad3cd7e96586ebd05a2607ee56b56fb2db8fd", |
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 use ETH_DEV_NODES
and ETH_DEV_SWAP_CONTRACT
consts here.
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.
Also eth_conf
for eth_coin_from_conf_and_request
"urls": ["http://195.201.0.6:8565"], | ||
"swap_contract_address": "0xa09ad3cd7e96586ebd05a2607ee56b56fb2db8fd", |
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.
Same here
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.
Please consider using eth_jst_conf
for eth_coin_from_conf_and_request
@@ -1243,55 +1245,348 @@ impl SwapOps for EthCoin { | |||
MmError::err(ValidateInstructionsErr::UnsupportedCoin(self.ticker().to_string())) | |||
} | |||
|
|||
fn is_supported_by_watchers(&self) -> bool { false } | |||
fn is_supported_by_watchers(&self) -> bool { true } |
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.
Q: Will this function have work flow in the future? If not, we don't need to register functions, it's better to have this functionality via coins configurations.
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 can be a case when an outdated mm2 alongside a newer coins config where supports_watchers
is true.
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.
Good point, I was going to remove it and use the coins config, but it's actually better this way then.
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 can be a case when an outdated mm2 alongside a newer coins config where supports_watchers is true.
Don't we use unwrap_or(DEFAULT_VALUE)
approach while parsing the config anyway?
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 believe all coins should support watchers in the future, so adding if a coin supports watchers to config will be an overkill, since we will add it to all or most coins configs eventually. We can implement a default implementation
fn is_supported_by_watchers(&self) -> bool { false }
in lp_coins.rs
and implement it to true
for Eth and Utxo only for now.
mm2src/coins/eth.rs
Outdated
let fut = async move { | ||
let tx = TransactionEnum::from(signed); | ||
Ok(tx) | ||
}; |
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.
let fut = async move { | |
let tx = TransactionEnum::from(signed); | |
Ok(tx) | |
}; | |
let fut = async move { Ok(TransactionEnum::from(signed)) }; |
mm2src/coins/eth.rs
Outdated
let fut = async move { | ||
let tx = TransactionEnum::from(signed); | ||
Ok(tx) | ||
}; |
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.
let fut = async move { | |
let tx = TransactionEnum::from(signed); | |
Ok(tx) | |
}; | |
let fut = async move { Ok(TransactionEnum::from(signed)) }; |
mm2src/coins/eth.rs
Outdated
.decode_input(&tx_from_rpc.input.0) | ||
.map_to_mm(|e| ValidatePaymentError::TxDeserializationError(e.to_string()))?; | ||
|
||
if decoded_input[0] != Token::Address(fee_addr) { |
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.
Direct access memory addresses might cause problems which are hard to debug. I recommend to use first()
function. It will return None
rather than panicking the mm2.
mm2src/coins/eth.rs
Outdated
let decoded = function | ||
.decode_input(&tx_from_rpc.input.0) | ||
.map_to_mm(|err| ValidatePaymentError::TxDeserializationError(err.to_string()))?; | ||
if decoded[0] != Token::FixedBytes(swap_id.clone()) { |
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.
Same here
mm2src/coins/eth.rs
Outdated
let decoded = function | ||
.decode_input(&tx_from_rpc.input.0) | ||
.map_to_mm(|err| ValidatePaymentError::TxDeserializationError(err.to_string()))?; | ||
if decoded[0] != Token::FixedBytes(swap_id.clone()) { |
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.
Same here
…time_locked_payment
…h_time_locked_payment
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.
Awesome work!
First review iteration
@@ -1243,55 +1245,348 @@ impl SwapOps for EthCoin { | |||
MmError::err(ValidateInstructionsErr::UnsupportedCoin(self.ticker().to_string())) | |||
} | |||
|
|||
fn is_supported_by_watchers(&self) -> bool { false } | |||
fn is_supported_by_watchers(&self) -> bool { true } |
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 can be a case when an outdated mm2 alongside a newer coins config where supports_watchers
is true.
} | ||
|
||
fn watcher_validate_taker_fee(&self, _input: WatcherValidateTakerFeeInput) -> ValidatePaymentFut<()> { | ||
unimplemented!(); | ||
fn watcher_validate_taker_fee(&self, validate_fee_args: WatcherValidateTakerFeeInput) -> ValidatePaymentFut<()> { |
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.
Is it possible to reuse EthCoin::validate_fee
?
I believe SwapOps::validate_fee
could be refactored to return Box<dyn Future<Item = (), Error = ValidateTakerFeeError> + Send>
or probably ValidatePaymentFut
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.
validate_fee
takes as parameter a copy of the taker fee, then requests the transaction from the chain using the hash. In the watcher, we don't have a copy of the transaction, we only have the hash. If we use the validate_fee
method, we'd have to request the transaction from the chain twice: once before calling the method and once in the method.
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 think for the watcher node it should be a problem to request the transaction twice. But on the other hand, it will simplify the code significantly.
Also as another solution, EthCoin::validate_fee
method could take an extended EthValidateFeeArgs
:
pub fee_tx_hash: &'a [u8], // This is the only difference between `ValidateFeeArgs` and `EthValidateFeeArgs`.
pub expected_sender: &'a [u8],
pub fee_addr: &'a [u8],
pub amount: &'a BigDecimal,
pub min_block_number: u64,
pub uuid: &'a [u8],
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.
Are you talking about writing another method that takes as parameter EthValidateFeeArgs
, which is then called both by EthCoin::validate_fee
and EthCoin::watcher_validate_taker_fee
?
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 also added this to the todo list in the issue
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.
Yes, exactly :)
Are you talking about writing another method that takes as parameter EthValidateFeeArgs, which is then called both by EthCoin::validate_fee and EthCoin::watcher_validate_taker_fee?
} | ||
|
||
fn watcher_validate_taker_payment(&self, _input: WatcherValidatePaymentInput) -> ValidatePaymentFut<()> { | ||
unimplemented!(); | ||
fn watcher_validate_taker_payment(&self, input: WatcherValidatePaymentInput) -> ValidatePaymentFut<()> { |
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.
Is it possible to reuse EthCoin::validate_payment
? Perhaps, it could look like:
fn watcher_validate_taker_payment(&self, input: WatcherValidatePaymentInput) -> ValidatePaymentFut<()> {
self.validate_payment(input.payment_tx, ...).await?;
// Here perform some additional checks:
}
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 one's possible, but there's some differences between the two validations. I added this to the TODO list in the issue, and I'll combine the methods if I can do it without making any important changes to the validate_payment
method.
"urls": ["http://195.201.0.6:8565"], | ||
"swap_contract_address": "0xa09ad3cd7e96586ebd05a2607ee56b56fb2db8fd", |
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.
Also eth_conf
for eth_coin_from_conf_and_request
"urls": ["http://195.201.0.6:8565"], | ||
"swap_contract_address": "0xa09ad3cd7e96586ebd05a2607ee56b56fb2db8fd", |
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.
Please consider using eth_jst_conf
for eth_coin_from_conf_and_request
…hereum-swap-watchers
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! I have just a comment for now.
) -> TransactionFut { | ||
let transaction: UtxoTx = | ||
try_tx_fus!(deserialize(taker_payment_refund_preimage).map_err(|e| TransactionErr::Plain(format!("{:?}", e)))); | ||
let transaction: UtxoTx = try_tx_fus!( |
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 taking T
as ref here and cloning once inside the function instead of cloning everywhere the function is used?
ub fn send_taker_payment_refund_preimage<T: UtxoCommonOps + SwapOps>(
coin: &T,
watcher_refunds_payment_args: SendWatcherRefundsPaymentArgs,
) -> TransactionFut {
let coin = coin.clone()
....
}
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.
Yes, this is better
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.
Looks good to me!
I have just a few non-critical suggestions
mm2src/coins/eth.rs
Outdated
if tx_from_rpc.to != Some(expected_swap_contract_address) { | ||
if tx_from_rpc.to == fallback_swap_contract { | ||
expected_swap_contract_address = fallback_swap_contract.ok_or_else(|| { | ||
ValidatePaymentError::InternalError("Fallback swap contract address not found".to_string()) | ||
})?; | ||
} else { | ||
return MmError::err(ValidatePaymentError::WrongPaymentTx(format!( | ||
"Payment tx {:?} was sent to wrong address, expected {:?}", | ||
tx_from_rpc, expected_swap_contract_address, | ||
))); | ||
} | ||
} |
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 think it can be simplified a bit. Please consider these changes
if tx_from_rpc.to != self.swap_contract_address && Some(tx_from_rpc.to) != fallback_swap {
return MmError::err(ValidatePaymentError::WrongPaymentTx(format!(
"Payment tx {tx_from_rpc:?} was sent to wrong address, expected either {:?} or the fallback {fallback_swap_contract:?}", self.swap_contract_address,
)));
}
let expected_swap_contract = tx_from_rpc.to;
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.
Nice, yes this is better
let swap_id_input = get_function_input_data(&decoded, function, 0) | ||
.map_to_mm(ValidatePaymentError::TxDeserializationError)?; |
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's not critical. In my opinion, the best way to handle each token step by step is to use Iterator::next
.
But only if the tokens are handled one after another.
For example,
https://github.com/KomodoPlatform/atomicDEX-API/blob/2e720274af8173b42a50fa54bbf36517096bbb51/mm2src/coins/qrc20/swap.rs#L681-L696
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.
We have too many decoded index accesses in the eth module, it would take hundreds of lines if we handle them all like this. Also in some cases not all sequential array items are required as you said, and I don't like the idea of using next() to skip unused items. I decided to use a helper function to simplify this as much as possible, and I think it'd be better if we use this and maybe improve it.
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 we need more detailed error messages, we could get e.g. the parameter names from the Function
input, or we could use a different helper for each contract function (e.g. erc20Payment) and write everything manually. But in general I'd prefer a solution that does all of this once, and not every time we use them. Maybe we can open an issue for this, there's still lots of direct index accesses in the eth module that I didn't fix.
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 meant that if you need to parse 1st, 2nd, 3rd, ... tokens, you can use Iterator::next
.
The best way to handle each token step by step is to use
Iterator::next
. But only if the tokens are handled one after another.
In other cases, I like your solution to use get_function_input_data
.
Anyway, the way you've chosen definitely can be
mm2src/coins/utxo/utxo_common.rs
Outdated
input: SendMakerPaymentSpendPreimageInput, | ||
) -> TransactionFut { | ||
let coin = coin.clone(); |
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 think it's better to clone the coin just before let fut = async move {}
mm2src/coins/utxo/utxo_common.rs
Outdated
maker_payment_tx: &[u8], | ||
time_lock: u32, | ||
maker_pub: &[u8], | ||
secret_hash: &[u8], | ||
swap_unique_data: &[u8], | ||
) -> TransactionFut { | ||
let coin = coin.clone(); |
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.
Same here
…hereum-swap-watchers
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! Two notes for the next iteration.
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.
Could you please consider two minor changes?
mm2src/coins/eth.rs
Outdated
} | ||
if tx_from_rpc.to != Some(expected_swap_contract_address) && tx_from_rpc.to != fallback_swap_contract { | ||
return MmError::err(ValidatePaymentError::WrongPaymentTx(format!( | ||
"Payment tx {tx_from_rpc:?} was sent to wrong address, expected either {expected_swap_contract_address:?} or the fallback {fallback_swap_contract:?}" |
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 guess you copied my suggestion as it is.
Could you please fix the tabulation?
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.
Yeah I copied your suggestion and made changes on it. Then I falsely assumed that cargo fmt would fix everything.
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.
LGTM! I have nothing to add :)
@caglaryucekaya Could you please also fix WASM CI stage? |
…hereum-swap-watchers
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 note related to the latest changes.
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.
Re-approving.
@ozkanonur @sergeyboyko0791 Could you complete the review of this PR, please? @caglaryucekaya If there are more notes, please add them to the next iteration TODO list. |
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 see the missing parts are now in the TODO list. So I can approve this.
Great work!
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.
LGTM, thanks for the fixes!
Extending the watcher spending
maker_payment_spend
and refundingtaker_payment
functionalities for Ethereum and ERC-20 tokens. ETH gas fee of watchers are not yet compensated in this PR.