-
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(utxo-swap): add utxo burn output for non-kmd taker fee #2112
base: dev
Are you sure you want to change the base?
Conversation
* dev: docs(README): remove outdated information from the README (#2097) fix(sia): fix sia compilation after hd wallet PR merge (#2103) feat(hd_wallet): utxo and evm hd wallet and trezor (#1962) feat(sia): initial Sia integration (#2086) fix(BCH): deserialize BCH header that uses KAWPOW version correctly (#2099) fix(eth_tests): remove ETH_DEV_NODE from tests (#2101)
fix is_kmd in TestCoin
* dev: feat(tendermint): pubkey-only activation and unsigned tx (#2088) fix(tests): set txfee for some tbtc tests (#2116) fix(eth): remove my_address from sign_and_send_transaction_with_keypair (#2115) fix(utxo-swap): apply events occurred while taker down (#2114) refactor(memory): memory usage improvements (#2098) feat(app-dir): implement root application dir `.kdf` (#2102) fix tendermint fee calculation (#2106) update dockerfile (#2104)
fix zhtlc send and spend tests
add should_burn_dex_fee method to indicate which coin has burn output
* dev: fix(indexeddb): window usage in worker env (#2131) feat(tx-history): handle encoded transaction values (#2133) fix(core): tendermint withdraws on hd accounts (#2130) fix(core): improve validation rules for table names (#2123) fix(test): improve log wait condition to fix taker restart test (#2125)
…er test add timeout in wait for fee in qrc20 docker test
… docker test failed due to different dex fee values if trait default impl was used)
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 progress! Here is the first review iteration
use same ovk for dex fee and burn outputs
add sanity check for dex_fee in calc_burn_amount_for_op_return
* dev: chore(RPCs): rename `get_peers_info` RPC to `get_directly_connected_peers` (#2195) chore(WASM-builds): remove `wasm-opt` overriding (#2200) fix(coins): add p2p feature to mm2_net dependency (#2210) chore(test): turn on debug assertion (#2204) feat(sia): extract sia lib to external repo (#2167) feat(eth-swap): eth tpu v2 methods, eth docker test enhancements (#2169) fix(cors): allow OPTIONS request to KDF server (#2191) docs(README): update commit badges to use dev branch (#2193) use default value for `komodo_proxy` (#2192) feat(cosmos): komodo-defi-proxy support (#2173)
* dev: 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)
…de was refactored to enable building with mockable)
- use only dex_fee_from_taker_coin() in swap code - make burn_active param optional (to allow None when only total fee amount is needed) - add DexFeeOption enum to better indicate requested dex fee kind - more dex fee calc tests
* dev: 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)
…ddress' error when syncing wallet db
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 feat :)
first review iter. gonna need more iteration(s) to draw the full picture.
I get that we use env-vars since we run mm2 as a process in tests thus have less control over it. i think we should invest in a framework to standardize how we do this since i can imagine this will be hard to maintain.
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 is not related, no? or is it a mis-merge with dev?
mm2src/coins/solana/spl.rs
Outdated
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.
... and this?
mm2src/coins/z_coin/z_htlc.rs
Outdated
let mut outputs = vec![dex_fee_out]; | ||
if let Some(dex_burn_amount) = dex_fee.burn_amount() { | ||
let dex_burn_amount_sat = | ||
sat_from_big_decimal(&Into::<BigDecimal>::into(dex_burn_amount), coin.utxo_arc.decimals)?; |
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 seem to allow DexFee::NoFee
here. Is this intentional?
also nit: Into::<BigDecimal>::into(dex_burn_amount)
-> dex_burn_amount.to_decimals()
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 thanks
@@ -72,7 +72,7 @@ mm2_number = { path = "../mm2_number"} | |||
mm2_p2p = { path = "../mm2_p2p" } | |||
mm2_rpc = { path = "../mm2_rpc" } | |||
mm2_state_machine = { path = "../mm2_state_machine" } | |||
mocktopus = "0.8.0" | |||
mocktopus = { version = "0.8.0", optional = 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.
maybe we could add it as a for-test
feature.
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 the way like it is now is convenient as we may use mocktopus both from tests inside the crate and from other crates, without extra features, and at the same time avoiding building it into releases
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.
from tests inside the crate -> dev-deps section
from tests from outside/integration -> for-test
flag to activate it, this has the downside though as u said will enable all and every extra test features we have, but it's a for-test build anyway so i wouldn't worry much about that and instead prioritize having less complex cfg
s around the code base.
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.
from tests from outside/integration -> for-test flag to activate it
I think, in this case mocktopus would be built into release what is not recommended for performance reasons.
The optional flag is a way to avoid this.
mm2src/coins/lp_coins.rs
Outdated
#[cfg(feature = "for-tests")] | ||
{ | ||
unsafe { | ||
if let Some(ref test_pk) = TEST_DEX_FEE_ADDR_RAW_PUBKEY { | ||
return test_pk.as_slice(); | ||
} | ||
} | ||
} |
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.
why have this if we can mock 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.
I think this would not work in this case, as this code is executed in another mm2 process (iirc I even tried this)
#[cfg(feature = "run-docker-tests")] | ||
if let Ok(env_pubkey) = std::env::var("TEST_DEX_FEE_ADDR_RAW_PUBKEY") { | ||
unsafe { | ||
TEST_DEX_FEE_ADDR_RAW_PUBKEY = Some(hex::decode(env_pubkey).expect("valid hex")); | ||
} | ||
} |
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.
can we have these only where we happen to use the variable TEST_DEX_FEE_ADDR_RAW_PUBKEY
? and maybe remove the variable then.
we might also want to group these test-related env vars in some file and assign them to constants to not lose track of them or have them dangling.
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, this var is needed because in docker swap tests we start new taker and maker mm2 processes and in couple tests I need to run taker with a test dex pubkey. I guess this is okay as there are other cases in the codebase using env vars to configure tests.
(I considered creating a feature for that but then we would need an extra cargo test call to run such tests in CI)
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 the static variable not the environment one
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.
Ofc I'd like to get rid of TEST_DEX_FEE_ADDR_RAW_PUBKEY static var too but for that I'd have to change dex_pubkey() return type (it's static ref now), did not like to do it just for tests sake
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.
huge progress! I would like to clarify some moments
/// Determine version from negotiation data if saved swap data does not store version | ||
/// (if the swap started before the upgrade to versioned negotiation message) | ||
/// In any case it is very undesirable to upgrade mm2 when any swaps are active | ||
fn fix_taker_version(negotiation_data: &TakerNegotiationData) -> u16 { negotiation_data.taker_version.unwrap_or(0) } |
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.
may be get_taker_version
? fix_
in the naming feels like it should solve some issue. But as I can see the function returns taker version from data
Same note for
fn fix_maker_version(negotiation_data: &MakerNegotiationData) -> u16 { negotiation_data.maker_version.unwrap_or(0) }
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.
changed both names, thanks
let mut msgs = vec![]; | ||
|
||
// Important to try the new versioned negotiation message first | ||
if cfg!(not(feature = "for-tests")) || !env_var_as_bool("USE_NON_VERSIONED_MAKER") { | ||
let maker_versioned_negotiation_msg = SwapMsgExt::NegotiationVersioned(NegotiationDataMsgVersion { | ||
version: SWAP_PROTOCOL_VERSION, | ||
msg: negotiation_data.clone(), | ||
}); | ||
msgs.push(( | ||
swap_ext_topic(&self.uuid), | ||
SwapMsgWrapper::Ext(maker_versioned_negotiation_msg), | ||
)); | ||
} | ||
|
||
let maker_old_negotiation_msg = SwapMsg::Negotiation(negotiation_data); | ||
msgs.push(( | ||
swap_topic(&self.uuid), | ||
SwapMsgWrapper::Legacy(maker_old_negotiation_msg), | ||
)); |
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 clarify, are you pushing both versioned negotiation msg and legacy negotiation msg in production code?
could you tell why do we need 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.
We need this for the upgrade period: first try if the remote node understands the old (default) version, if yes, fallback to older negotiation protocol and turn off new features
let burn_amount = dex_fee - min_tx_amount; | ||
(min_tx_amount.clone(), burn_amount) | ||
} else { | ||
(dex_fee.clone(), MmNumber::from(0)) |
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.
shouldn't this be (min_tx_amount.clone(), 0)
, if we reach this case then we know dex_fee
is dust already.
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.
dex_fee is not dust in both calc_burn_amount_for_burn_account() and calc_burn_amount_for_op_return(): it was checked in the caller fn.
// Default case where burn_amount is considered dust | ||
(dex_fee.clone(), 0.into()) |
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.
similar to the case above, we don't know for sure here if dex_fee
is dust or not though.
let new_fee = dex_fee * &MmNumber::from(Self::DEX_FEE_SHARE); | ||
let burn_amount = dex_fee - &new_fee; | ||
if &new_fee >= min_tx_amount && &burn_amount >= min_tx_amount { | ||
// Use the max burn value, which is 25%. Ensure burn_amount is not dust | ||
return (new_fee, burn_amount); | ||
} | ||
// If the new dex fee is dust set it to min_tx_amount and check the updated burn_amount is not dust. | ||
let burn_amount = dex_fee - min_tx_amount; | ||
if &new_fee < min_tx_amount && &burn_amount >= min_tx_amount { | ||
// actually currently burn_amount (25%) < new_fee (75%) so this never happens. Added for a case if 25/75 will ever change | ||
return (min_tx_amount.clone(), burn_amount); | ||
} |
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 find match
es usually good for when the conditions get complicated:
match (dex_cut > min_tx_amount, burn_cut > min_tx_amount) {
(true, true) => (dex_cut, burn_cut),
(true, false) => (both_cuts, 0),
(false, true) => (both_cuts, 0),
(false, false) => (max(both_cuts, min_tx_amount), 0)
}
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.
Hmm, looks a bit unusual for me.
I guess, expressions like traditional boolean algebra logic are more comprehensible?
#[cfg(feature = "run-docker-tests")] | ||
if let Ok(env_pubkey) = std::env::var("TEST_DEX_FEE_ADDR_RAW_PUBKEY") { | ||
unsafe { | ||
TEST_DEX_FEE_ADDR_RAW_PUBKEY = Some(hex::decode(env_pubkey).expect("valid hex")); | ||
} | ||
} |
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 the static variable not the environment one
|
||
if taker_coin.is_kmd() { | ||
// use a special dex fee option for kmd | ||
let (fee_amount, burn_amount) = Self::calc_burn_amount_for_op_return(&dex_fee, &min_tx_amount); |
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 came to find the burn_amount = 0
in this branch we should return dex fee standard (just like the non-kmd counterpart), otherwise the code down the call tree will generate a tx with a zero op_return burn and spike the miner fee (tx size) for no reason.
tbh i think we should merge Standard
& WithBurn
to something general that has both dex and burn fee and to always check the burn (and dex) fee aren't zero before generating their output (legacy standard & dust burn would both have 0 burn 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.
We can also use non-zero-u* for the burn field to make sure it's always set (if we don't wanna go to the route of merging both variants & checking for 0 at tx generation).
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 came to find the burn_amount = 0 in this branch we should return dex fee standard
This changes already working code. I guess we could do this only as version activated feature
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 think we should merge Standard & WithBurn to something general
Eventually we will eliminate the Standard option (when all nodes are upgraded), so in fact this will be done.
For now I suggest to retain it as it would be easier to eliminated the code related to Standard.
mm2src/coins/swap_features.rs
Outdated
|
||
pub fn is_active(feature: SwapFeature, version: u16) -> bool { | ||
if let Some(found) = Self::SWAP_FEATURE_ACTIVATION.iter().find(|fv| fv.1 == feature) { | ||
return version >= found.0; |
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 always assumes that a remote version greater than activation version will always activation such a feature (but might we not disable it?). this is not the most extensible.
e.g.: what if we activated some feature in version 3
and disabled it in version 5
, a version 3/4
peer will think the feature is active since 5 >= 3
.
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.
added deactivation in 05feb72 (may be of use indeed)
let remote_version = self | ||
.r() | ||
.data | ||
.taker_version | ||
.ok_or("No swap protocol version".to_owned())?; | ||
let is_burn_active = SwapFeature::is_active(SwapFeature::SendToPreBurnAccount, remote_version); | ||
let dex_fee = dex_fee_from_taker_coin( | ||
self.taker_coin.deref(), | ||
&self.r().data.maker_coin, | ||
&taker_amount, | ||
Some(self.r().other_taker_coin_htlc_pub.to_vec().as_ref()), | ||
Some(is_burn_active), | ||
); |
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 think this would be dev-friendly to upgrade to the next feature set. I like the versioning mechanism and think we should have one, but also think it should be abstracted away in each entities impl (here DexFee).
so how i imagine something like this would be is: taker and maker settle on a version together (deterministically, pick the min version for e.g. or have a deterministic mapping rules), and we pass that version to whoever needs it to apply some version specific rules (in here this is the DexFee, so something like DexFee::using_version(version, taker_coin, maker_coin, etc...)
and inside we match against the version and apply the appropriate rule),
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 started to do this but realised there is a problem here:
We support both legacy and TPU protocols, each of both may have own versioning. So it looks like it's better to deal with versions outside the lower level components like DexFee (which is called both from legacy and TPU), don't you think?
.r() | ||
.data | ||
.taker_version | ||
.ok_or("No swap protocol version".to_owned())?; |
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.
shouldn't this be unwrap_or(0)
? or maybe just not let this be an option and default it to 0
right away.
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.
The reason why this taker_version is optional is because it is discovered on the negotiation phase.
I think it is not correct to set it to some specific version on the swap start.
Also, if it is not defined at this stage (wait_taker_fee) - I think this is a clear logic error and probably also not very good to default it to 0 or another version (we should determine it explicitly on the negotiation phase).
TakerPayment(Vec<u8>), | ||
} | ||
|
||
// Extension to SwapMsg with version added to negotiation exchange | ||
#[derive(Clone, Debug, Eq, Deserialize, PartialEq, Serialize)] | ||
pub enum SwapMsgExt { | ||
NegotiationVersioned(NegotiationDataMsgVersion), | ||
NegotiationReplyVersioned(NegotiationDataMsgVersion), | ||
} |
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.
why not merge the new msgs into the old struct (and have a comment separating this is version x)?
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 did this on purpose: the old struct should be intact for old nodes to be able to parse it.
(I tried to test this though, in principle it could be tweaked to allow the merged struct to be parsed but I chose not to do this cause it involves knowledge about internal lib behaviour)
@@ -72,7 +72,7 @@ mm2_number = { path = "../mm2_number"} | |||
mm2_p2p = { path = "../mm2_p2p" } | |||
mm2_rpc = { path = "../mm2_rpc" } | |||
mm2_state_machine = { path = "../mm2_state_machine" } | |||
mocktopus = "0.8.0" | |||
mocktopus = { version = "0.8.0", optional = 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.
from tests inside the crate -> dev-deps section
from tests from outside/integration -> for-test
flag to activate it, this has the downside though as u said will enable all and every extra test features we have, but it's a for-test build anyway so i wouldn't worry much about that and instead prioritize having less complex cfg
s around the code base.
revert preburn_account_active var (fix burning kmd coins by non-upgraded nodes)
Adds a burn output sending 25% of the taker utxo DEX fee to a dedicated pre-burn address. Funds collected on the pre-burn address will be traded for KMD to burn them (thus additionally burning KMD supply).
This PR partially closes #2010
The requirements for this PR: #2269
In this PR:
NOTE: As mocktopus now is marked 'optional = true' in coins Cargo.toml and activated from the mm2_main crate by adding features = ["mocktopus"] in [dev-dependencies] section, you also need to mark your mockable code, called from other crates, this way:
#[cfg_attr(feature = "mocktopus", mockable)]
, otherwise mocks won't work (see samples in code)TODO: