-
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): swap UTXO PoC using Storable State Machine. #1958
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.
Great work!
Left comments to add doc coms and asked one question
pub type GenAndSignDexFeeSpendResult = MmResult<TxPreimageWithSig, TxGenError>; | ||
pub type ValidateDexFeeResult = MmResult<(), ValidateDexFeeError>; | ||
pub type GenTakerPaymentSpendResult = MmResult<TxPreimageWithSig, TxGenError>; | ||
pub type ValidateDexFeeResult = MmResult<(), ValidateTakerPaymentError>; | ||
pub type ValidateDexFeeSpendPreimageResult = MmResult<(), ValidateDexFeeSpendPreimageError>; | ||
|
||
pub type IguanaPrivKey = Secp256k1Secret; |
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.
doc comment
@laruh I added as many doc comments as I could 🙂 Can you review the PR again, please? |
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 iteration, Will do another last review iteration before the end of the week.
For this review, only minor issues and opening some points for discussion.
I looked over the whole swap v2 code for better understanding. Do I understand it correct that in swap v2 we eliminate step1 (taker sends dex fee) and now taker sends all amounts in his tx: dex fee, premium, trading amount as step1? |
Hey @dimxy, thanks for your feedback! 🙂
Yes, it's correct.
Yes, it does, but we are looking for ways to solve this problem or at least reduce attack surface. |
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.
🔥
@KomodoPlatform/qa: This comment #1958 (comment) has a test plan to test only the success case of utxo/utxo swap using the new trading protocol. |
@kivqa I've setup 3 seednodes for use in testing this PR. Add the following to your MM2.json:
|
Taker
Maker
Seednode
|
Added this PR to v1.1.0-beta since it's a WIP PR and doesn't require any changes for GUIs yet. |
#1895
What's done:
broadcast_swap_v2_message
can (and likely should) be used for other protocols utilizing protobuf, but I preferred to keep them inlp_swap
for now to not overlap with P2P upgrade PR.Important notes:
Test plan:
"use_trading_proto_v2": true
in MM2.conf."protocol": { "type": "UTXO" }
.setprice
andbuy/sell
calls.