-
Notifications
You must be signed in to change notification settings - Fork 98
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(tpu-v2): provide swap protocol versioning #2324
base: fix-tpu-v2-wait-for-payment-spend
Are you sure you want to change the base?
Changes from 4 commits
7387740
124057a
998e75a
53d4b40
d4c5a16
e37c74e
a62f56a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -134,6 +134,8 @@ const TRIE_STATE_HISTORY_TIMEOUT: u64 = 3; | |||||
const TRIE_ORDER_HISTORY_TIMEOUT: u64 = 300; | ||||||
#[cfg(test)] | ||||||
const TRIE_ORDER_HISTORY_TIMEOUT: u64 = 3; | ||||||
/// Current swap protocol version | ||||||
const SWAP_VERSION: u32 = 2; | ||||||
|
||||||
pub type OrderbookP2PHandlerResult = Result<(), MmError<OrderbookP2PHandlerError>>; | ||||||
|
||||||
|
@@ -1169,6 +1171,9 @@ pub struct TakerRequest { | |||||
#[serde(default)] | ||||||
#[serde(skip_serializing_if = "Option::is_none")] | ||||||
pub rel_protocol_info: Option<Vec<u8>>, | ||||||
#[serde(default = "legacy_swap_version")] | ||||||
#[serde(skip_serializing_if = "is_legacy_swap_version")] | ||||||
pub swap_version: u32, | ||||||
} | ||||||
|
||||||
impl TakerRequest { | ||||||
|
@@ -1189,6 +1194,7 @@ impl TakerRequest { | |||||
conf_settings: Some(message.conf_settings), | ||||||
base_protocol_info: message.base_protocol_info, | ||||||
rel_protocol_info: message.rel_protocol_info, | ||||||
swap_version: message.swap_version, | ||||||
} | ||||||
} | ||||||
|
||||||
|
@@ -1234,6 +1240,7 @@ impl From<TakerOrder> for new_protocol::OrdermatchMessage { | |||||
conf_settings: taker_order.request.conf_settings.unwrap(), | ||||||
base_protocol_info: taker_order.request.base_protocol_info, | ||||||
rel_protocol_info: taker_order.request.rel_protocol_info, | ||||||
swap_version: taker_order.request.swap_version, | ||||||
}) | ||||||
} | ||||||
} | ||||||
|
@@ -1244,6 +1251,9 @@ impl TakerRequest { | |||||
fn get_rel_amount(&self) -> &MmNumber { &self.rel_amount } | ||||||
} | ||||||
|
||||||
const fn legacy_swap_version() -> u32 { 1 } | ||||||
fn is_legacy_swap_version(swap_version: &u32) -> bool { *swap_version == legacy_swap_version() } | ||||||
|
||||||
pub struct TakerOrderBuilder<'a> { | ||||||
base_coin: &'a MmCoinEnum, | ||||||
rel_coin: &'a MmCoinEnum, | ||||||
|
@@ -1259,6 +1269,7 @@ pub struct TakerOrderBuilder<'a> { | |||||
min_volume: Option<MmNumber>, | ||||||
timeout: u64, | ||||||
save_in_history: bool, | ||||||
swap_version: u32, | ||||||
} | ||||||
|
||||||
pub enum TakerOrderBuildError { | ||||||
|
@@ -1338,6 +1349,7 @@ impl<'a> TakerOrderBuilder<'a> { | |||||
order_type: OrderType::GoodTillCancelled, | ||||||
timeout: TAKER_ORDER_TIMEOUT, | ||||||
save_in_history: true, | ||||||
swap_version: SWAP_VERSION, | ||||||
} | ||||||
} | ||||||
|
||||||
|
@@ -1401,6 +1413,17 @@ impl<'a> TakerOrderBuilder<'a> { | |||||
self | ||||||
} | ||||||
|
||||||
/// When a new [TakerOrderBuilder::new] is created, it sets [SWAP_VERSION] by default. | ||||||
/// However, if user has not specified in the config to use TPU V2, | ||||||
/// the TakerOrderBuilder's swap_version is changed to legacy. | ||||||
/// In the future alls users will be using TPU V2 by default without "use_trading_proto_v2" configuration. | ||||||
pub fn use_trading_proto_v2(mut self, use_trading_proto_v2: bool) -> Self { | ||||||
if !use_trading_proto_v2 { | ||||||
self.swap_version = legacy_swap_version(); | ||||||
} | ||||||
self | ||||||
} | ||||||
|
||||||
/// Validate fields and build | ||||||
#[allow(clippy::result_large_err)] | ||||||
pub fn build(self) -> Result<TakerOrder, TakerOrderBuildError> { | ||||||
|
@@ -1489,6 +1512,7 @@ impl<'a> TakerOrderBuilder<'a> { | |||||
conf_settings: self.conf_settings, | ||||||
base_protocol_info: Some(base_protocol_info), | ||||||
rel_protocol_info: Some(rel_protocol_info), | ||||||
swap_version: self.swap_version, | ||||||
}, | ||||||
matches: Default::default(), | ||||||
min_volume, | ||||||
|
@@ -1529,6 +1553,7 @@ impl<'a> TakerOrderBuilder<'a> { | |||||
conf_settings: self.conf_settings, | ||||||
base_protocol_info: Some(base_protocol_info), | ||||||
rel_protocol_info: Some(rel_protocol_info), | ||||||
swap_version: self.swap_version, | ||||||
}, | ||||||
matches: HashMap::new(), | ||||||
min_volume: Default::default(), | ||||||
|
@@ -1690,6 +1715,9 @@ pub struct MakerOrder { | |||||
/// A custom priv key for more privacy to prevent linking orders of the same node between each other | ||||||
/// Commonly used with privacy coins (ARRR, ZCash, etc.) | ||||||
p2p_privkey: Option<SerializableSecp256k1Keypair>, | ||||||
#[serde(default = "legacy_swap_version")] | ||||||
#[serde(skip_serializing_if = "is_legacy_swap_version")] | ||||||
pub swap_version: u32, | ||||||
} | ||||||
|
||||||
pub struct MakerOrderBuilder<'a> { | ||||||
|
@@ -1702,6 +1730,7 @@ pub struct MakerOrderBuilder<'a> { | |||||
rel_orderbook_ticker: Option<String>, | ||||||
conf_settings: Option<OrderConfirmationsSettings>, | ||||||
save_in_history: bool, | ||||||
swap_version: u32, | ||||||
} | ||||||
|
||||||
pub enum MakerOrderBuildError { | ||||||
|
@@ -1851,6 +1880,7 @@ impl<'a> MakerOrderBuilder<'a> { | |||||
price: 0.into(), | ||||||
conf_settings: None, | ||||||
save_in_history: true, | ||||||
swap_version: SWAP_VERSION, | ||||||
} | ||||||
} | ||||||
|
||||||
|
@@ -1889,6 +1919,17 @@ impl<'a> MakerOrderBuilder<'a> { | |||||
self | ||||||
} | ||||||
|
||||||
/// When a new [MakerOrderBuilder::new] is created, it sets [SWAP_VERSION] by default. | ||||||
/// However, if user has not specified in the config to use TPU V2, | ||||||
/// the MakerOrderBuilder's swap_version is changed to legacy. | ||||||
/// In the future alls users will be using TPU V2 by default without "use_trading_proto_v2" configuration. | ||||||
pub fn use_trading_proto_v2(mut self, use_trading_proto_v2: bool) -> Self { | ||||||
if !use_trading_proto_v2 { | ||||||
self.swap_version = legacy_swap_version(); | ||||||
} | ||||||
self | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above |
||||||
} | ||||||
|
||||||
/// Build MakerOrder | ||||||
#[allow(clippy::result_large_err)] | ||||||
pub fn build(self) -> Result<MakerOrder, MakerOrderBuildError> { | ||||||
|
@@ -1945,6 +1986,7 @@ impl<'a> MakerOrderBuilder<'a> { | |||||
base_orderbook_ticker: self.base_orderbook_ticker, | ||||||
rel_orderbook_ticker: self.rel_orderbook_ticker, | ||||||
p2p_privkey, | ||||||
swap_version: self.swap_version, | ||||||
}) | ||||||
} | ||||||
|
||||||
|
@@ -1969,6 +2011,7 @@ impl<'a> MakerOrderBuilder<'a> { | |||||
base_orderbook_ticker: None, | ||||||
rel_orderbook_ticker: None, | ||||||
p2p_privkey: None, | ||||||
swap_version: self.swap_version, | ||||||
} | ||||||
} | ||||||
} | ||||||
|
@@ -2099,6 +2142,7 @@ impl From<TakerOrder> for MakerOrder { | |||||
base_orderbook_ticker: taker_order.base_orderbook_ticker, | ||||||
rel_orderbook_ticker: taker_order.rel_orderbook_ticker, | ||||||
p2p_privkey: taker_order.p2p_privkey, | ||||||
swap_version: taker_order.request.swap_version, | ||||||
}, | ||||||
// The "buy" taker order is recreated with reversed pair as Maker order is always considered as "sell" | ||||||
TakerAction::Buy => { | ||||||
|
@@ -2121,6 +2165,7 @@ impl From<TakerOrder> for MakerOrder { | |||||
base_orderbook_ticker: taker_order.rel_orderbook_ticker, | ||||||
rel_orderbook_ticker: taker_order.base_orderbook_ticker, | ||||||
p2p_privkey: taker_order.p2p_privkey, | ||||||
swap_version: taker_order.request.swap_version, | ||||||
} | ||||||
}, | ||||||
} | ||||||
|
@@ -2173,6 +2218,9 @@ pub struct MakerReserved { | |||||
#[serde(default)] | ||||||
#[serde(skip_serializing_if = "Option::is_none")] | ||||||
pub rel_protocol_info: Option<Vec<u8>>, | ||||||
#[serde(default = "legacy_swap_version")] | ||||||
#[serde(skip_serializing_if = "is_legacy_swap_version")] | ||||||
pub swap_version: u32, | ||||||
} | ||||||
|
||||||
impl MakerReserved { | ||||||
|
@@ -2200,6 +2248,7 @@ impl MakerReserved { | |||||
conf_settings: Some(message.conf_settings), | ||||||
base_protocol_info: message.base_protocol_info, | ||||||
rel_protocol_info: message.rel_protocol_info, | ||||||
swap_version: message.swap_version, | ||||||
} | ||||||
} | ||||||
} | ||||||
|
@@ -2216,6 +2265,7 @@ impl From<MakerReserved> for new_protocol::OrdermatchMessage { | |||||
conf_settings: maker_reserved.conf_settings.unwrap(), | ||||||
base_protocol_info: maker_reserved.base_protocol_info, | ||||||
rel_protocol_info: maker_reserved.rel_protocol_info, | ||||||
swap_version: maker_reserved.swap_version, | ||||||
}) | ||||||
} | ||||||
} | ||||||
|
@@ -2996,8 +3046,10 @@ fn lp_connect_start_bob(ctx: MmArc, maker_match: MakerMatch, maker_order: MakerO | |||||
}, | ||||||
}; | ||||||
|
||||||
// TODO add alice swap protocol version check once protocol versioning is implemented | ||||||
if !ctx.use_trading_proto_v2() { | ||||||
let alice_swap_v = maker_match.request.swap_version; | ||||||
|
||||||
// Start legacy swap if taker uses legacy protocol (version 1) or if conditions for trading_proto_v2 aren't met. | ||||||
if alice_swap_v == 1u32 || !ctx.use_trading_proto_v2() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done d4c5a16 |
||||||
let params = LegacySwapParams { | ||||||
maker_coin: &maker_coin, | ||||||
taker_coin: &taker_coin, | ||||||
|
@@ -3131,6 +3183,7 @@ async fn start_maker_swap_state_machine< | |||||
lock_duration: *params.locktime, | ||||||
taker_p2p_pubkey: *taker_p2p_pubkey, | ||||||
require_taker_payment_spend_confirm: true, | ||||||
swap_version: maker_order.swap_version, | ||||||
}; | ||||||
#[allow(clippy::box_default)] | ||||||
maker_swap_state_machine | ||||||
|
@@ -3216,8 +3269,10 @@ fn lp_connected_alice(ctx: MmArc, taker_order: TakerOrder, taker_match: TakerMat | |||||
uuid | ||||||
); | ||||||
|
||||||
// TODO add bob swap protocol version check once protocol versioning is implemented | ||||||
if !ctx.use_trading_proto_v2() { | ||||||
let bob_swap_v = taker_match.reserved.swap_version; | ||||||
|
||||||
// Start legacy swap if maker uses legacy protocol (version 1) or if conditions for trading_proto_v2 aren't met. | ||||||
if bob_swap_v == 1u32 || !ctx.use_trading_proto_v2() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done d4c5a16 |
||||||
let params = LegacySwapParams { | ||||||
maker_coin: &maker_coin, | ||||||
taker_coin: &taker_coin, | ||||||
|
@@ -3366,6 +3421,7 @@ async fn start_taker_swap_state_machine< | |||||
maker_p2p_pubkey: *maker_p2p_pubkey, | ||||||
require_maker_payment_confirm_before_funding_spend: true, | ||||||
require_maker_payment_spend_confirm: true, | ||||||
swap_version: taker_order.request.swap_version, | ||||||
}; | ||||||
#[allow(clippy::box_default)] | ||||||
taker_swap_state_machine | ||||||
|
@@ -3872,6 +3928,7 @@ async fn process_taker_request(ctx: MmArc, from_pubkey: H256Json, taker_request: | |||||
}), | ||||||
base_protocol_info: Some(base_coin.coin_protocol_info(None)), | ||||||
rel_protocol_info: Some(rel_coin.coin_protocol_info(Some(rel_amount.clone()))), | ||||||
swap_version: order.swap_version, | ||||||
}; | ||||||
let topic = order.orderbook_topic(); | ||||||
log::debug!("Request matched sending reserved {:?}", reserved); | ||||||
|
@@ -4104,7 +4161,8 @@ pub async fn lp_auto_buy( | |||||
.with_sender_pubkey(H256Json::from(our_public_id.bytes)) | ||||||
.with_save_in_history(input.save_in_history) | ||||||
.with_base_orderbook_ticker(ordermatch_ctx.orderbook_ticker(base_coin.ticker())) | ||||||
.with_rel_orderbook_ticker(ordermatch_ctx.orderbook_ticker(rel_coin.ticker())); | ||||||
.with_rel_orderbook_ticker(ordermatch_ctx.orderbook_ticker(rel_coin.ticker())) | ||||||
.use_trading_proto_v2(ctx.use_trading_proto_v2()); | ||||||
if let Some(timeout) = input.timeout { | ||||||
order_builder = order_builder.with_timeout(timeout); | ||||||
} | ||||||
|
@@ -4849,7 +4907,8 @@ pub async fn create_maker_order(ctx: &MmArc, req: SetPriceReq) -> Result<MakerOr | |||||
.with_conf_settings(conf_settings) | ||||||
.with_save_in_history(req.save_in_history) | ||||||
.with_base_orderbook_ticker(ordermatch_ctx.orderbook_ticker(base_coin.ticker())) | ||||||
.with_rel_orderbook_ticker(ordermatch_ctx.orderbook_ticker(rel_coin.ticker())); | ||||||
.with_rel_orderbook_ticker(ordermatch_ctx.orderbook_ticker(rel_coin.ticker())) | ||||||
.use_trading_proto_v2(ctx.use_trading_proto_v2()); | ||||||
|
||||||
let new_order = try_s!(builder.build()); | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -706,6 +706,8 @@ mod tests { | |
|
||
wasm_bindgen_test_configure!(run_in_browser); | ||
|
||
const LEGACY_SWAP_V: u32 = 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Done d4c5a16 |
||
|
||
fn maker_order_for_test() -> MakerOrder { | ||
MakerOrder { | ||
base: "BASE".to_owned(), | ||
|
@@ -724,6 +726,7 @@ mod tests { | |
base_orderbook_ticker: None, | ||
rel_orderbook_ticker: None, | ||
p2p_privkey: None, | ||
swap_version: LEGACY_SWAP_V, | ||
} | ||
} | ||
|
||
|
@@ -742,6 +745,7 @@ mod tests { | |
conf_settings: None, | ||
base_protocol_info: None, | ||
rel_protocol_info: None, | ||
swap_version: LEGACY_SWAP_V, | ||
}, | ||
matches: HashMap::new(), | ||
created_at: now_ms(), | ||
|
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 this?
or, at least rename it to something like
maybe_**
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.
u suggested to change version to legacy protocol calling
use_trading_proto_v2
please re read 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.
What I suggest is, either remove this boolean from the function and check it on the caller side or declare that in the function name with "maybe" 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.
I dont get it, on the caller side it is where? Right now it looks like, you call
use_trading_proto_v2
and set legacyThere 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 updated function name e37c74e
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 didn't pay attention to the logic, the usage was just unclear. Looking at the logic, it's even more unclear.. Function is called
use_trading_proto_v2
but it downgrades version to legacy if given boolean is false.Why not just this:
and call it only when
use_trading_proto_v2
is false?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 thing is that use_trading_proto_v2 initially is a config boolean flag, so use_trading_proto_legacy is supposed to be a question
ok I refactored function, now its purpose is only to set legacy protocol a62f56a