-
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(trading-proto-upgrade): UTXO PoC + State machine refactor #1927
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.
Huge work!
First review(roughly) iteration from my side:
@ozkanonur Thanks for your review, all notes are fixed. |
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.
Thank you for the great work! Only 2 suggestions/questions at this stage as I couldn't find any major issues in this first review. Will do another review later but everything seems good after first look :)
pub async fn validate_dex_fee_spend_preimage<T: UtxoCommonOps + SwapOps>( | ||
coin: &T, | ||
gen_args: &GenDexFeeSpendArgs<'_>, | ||
preimage: &TxPreimageWithSig, |
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.
Isn't the signature enough to be shared in P2P messages? I think there is no need to share and validate the preimage since it can be constructed at the maker side. If that is the case, this can be done in next PRs.
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.
In UTXO case, maker can get a different transaction lock time and resulting premium amount (depending on miner fee). So, I had to add LocktimeSetting
and CalcPremiumBy
enums, and use the exact values from taker's preimage on maker's side. For other coin protocols, only signature might be sufficient.
komodo-defi-framework/mm2src/coins/utxo/utxo_common.rs
Lines 1324 to 1330 in ebe7e4f
let expected_preimage = gen_dex_fee_spend_preimage( | |
coin, | |
gen_args, | |
LocktimeSetting::UseExact(actual_preimage_tx.lock_time), | |
CalcPremiumBy::UseExactAmount(actual_preimage_tx.outputs[1].value), | |
) | |
.await?; |
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.
For other coin protocols, only signature might be sufficient.
Thanks for the explanation. I guess miner fee/gas discrepancy can also make signature not enough for other coins since they are part of the signed transaction. Can a different SIGHASH be used to allow bob decide the miner fee they want to pay instead of verifying the fee that alice calculated?
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.
For ETH/ERC-20, transaction preimage signing won't be required. Taker will sign a payload that will specify shares and destinations to which the payment should be spent. Maker will add the signature as an argument to smart contract call. For other protocols, additional research is required.
Can a different SIGHASH be used to allow bob decide the miner fee they want to pay instead of verifying the fee that alice calculated?
I will check it on next iterations, thanks for the 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.
Last review iteration:
mm2src/coins/utxo/utxo_common.rs
Outdated
coin, | ||
gen_args, | ||
LocktimeSetting::UseExact(actual_preimage_tx.lock_time), | ||
CalcPremiumBy::UseExactAmount(actual_preimage_tx.outputs[1].value), |
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 use get
to access indexes(including the other ones in this module)? It's panic-safe and allows us to handle out of bounds
situations.
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 have fixed this line but preferred to keep other cases as is for now to not make the diff even larger. Added to checklist for next iteration: #1895 (comment)
coin.as_ref().conf.signature_version, | ||
coin.as_ref().conf.fork_id | ||
)); | ||
let sig_hash_all_fork_id = 1 | coin.as_ref().conf.fork_id as 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.
Q: Are we 100% sure that fork_id
in coins configuration is always <= u8::MAX
? Otherwise sig_hash_all_fork_id
may have unexpected/random values.
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 is no such validation and coins repo even has asset using 2 bytes value for fork_id
:
https://github.com/KomodoPlatform/coins/blob/05eb26c9fdecb6168f4d900fa2db7464ddd6b675/coins#L7229.
I am now wondering whether this LTFN coin works at all 🙂
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.
UPD: at least it used to work at the moment of addition: KomodoPlatform/coins@676149e.
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 problem of using as
is sometimes it returns random values when overflow occurs which may result in unexpected behaviours and it's really hard to catch.
AFAIK fork_id
is currently u32
. If we update it to u8
, it will make more sense to use it fork_id as u32
which can't overflow so there will be no unexpected behaviour. And if a longer value than u8
was given to fork_id
in coins config, mm2 will validate and panic the process automatically on the initialization
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 trick with forementioned LTFN coin is it already uses 0x2A40
fork_id that doesn't fit into u8, and it somehow worked with as u8
conversion, which is actually used for a long time in our signing code:
sig_script.append(&mut Bytes::from(vec![1 | fork_id as u8])); |
I will need some time to do research on this, as it looks strange.
if dex_fee_tx.outputs[0] != expected_output { | ||
return MmError::err(ValidateDexFeeError::InvalidDestinationOrAmount(format!( | ||
"Expected {:?}, got {:?}", | ||
expected_output, dex_fee_tx.outputs[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.
if dex_fee_tx.outputs[0] != expected_output { | |
return MmError::err(ValidateDexFeeError::InvalidDestinationOrAmount(format!( | |
"Expected {:?}, got {:?}", | |
expected_output, dex_fee_tx.outputs[0] | |
))); | |
} | |
if dex_fee_tx.outputs.first() != Some(&expected_output) { | |
return MmError::err(ValidateDexFeeError::InvalidDestinationOrAmount(format!( | |
"Expected {:?}, got {:?}", | |
expected_output, dex_fee_tx.outputs.first() | |
))); | |
} |
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.
As outputs' length is checked previously, is this really required?
komodo-defi-framework/mm2src/coins/utxo/utxo_common.rs
Lines 4584 to 4586 in ebe7e4f
if dex_fee_tx.outputs.len() < 2 { | |
return MmError::err(ValidateDexFeeError::TxLacksOfOutputs); | |
} |
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.
As outputs' length is checked previously, is this really required?
I missed that on the review. At this point it's not required, but still safer way to follow in my opinion(e.g., when a dev makes changes here will likely forget updating the following parts of codes since it's not easy to catch). You can keep it or update it, I leave it to you
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 will update it in the next iteration - we can change the number of outputs in the future, and as you said, forget to update relevant parts of the code. Using get(index)
will be safer in general.
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.
I used this review mostly for studying code
SwapOpsV2
trait containing methods of the new protocol (WIP).SwapOpsV2
forUtxoStandardCoin
.StorableStateMachine
pattern extension.#1895