Skip to content
This repository was archived by the owner on Mar 23, 2021. It is now read-only.

Refactor State Machine and Events have correct types for Alice AND Bob #427

Merged
merged 10 commits into from
Nov 19, 2018
2 changes: 1 addition & 1 deletion application/comit_node/src/comit_client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use std::{io, net::SocketAddr, sync::Arc};
use std::{fmt::Debug, panic::RefUnwindSafe};
use swap_protocols::{bam_types, rfc003};

pub trait Client: Send + Sync {
pub trait Client: Send + Sync + 'static {
fn send_swap_request<
SL: rfc003::Ledger,
TL: rfc003::Ledger,
Expand Down
19 changes: 11 additions & 8 deletions application/comit_node/src/http_api/rfc003/swap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ use swap_protocols::{
ledger::{Bitcoin, Ethereum},
rfc003::{
self, bitcoin,
roles::{Alice, Bob},
state_store::{self, StateStore},
Ledger, Secret, SecretHash,
Ledger, Secret,
},
Assets, Ledgers, Metadata, MetadataStore, Roles,
};
Expand Down Expand Up @@ -290,14 +291,16 @@ fn handle_state_for_get_swap<T: MetadataStore<SwapId>, S: state_store::StateStor
target_asset: Assets::Ether,
role,
}) => match role {
Roles::Alice => match state_store
.get::<Bitcoin, Ethereum, BitcoinQuantity, EtherQuantity, Secret>(id)
{
Err(e) => error!("Could not retrieve state: {:?}", e),
Ok(state) => info!("Here is the state we have retrieved: {:?}", state),
},
Roles::Alice => {
match state_store
.get::<Alice<Bitcoin, Ethereum, BitcoinQuantity, EtherQuantity>>(id)
{
Err(e) => error!("Could not retrieve state: {:?}", e),
Ok(state) => info!("Here is the state we have retrieved: {:?}", state),
}
}
Roles::Bob => match state_store
.get::<Bitcoin, Ethereum, BitcoinQuantity, EtherQuantity, SecretHash>(id)
.get::<Bob<Bitcoin, Ethereum, BitcoinQuantity, EtherQuantity>>(id)
{
Err(e) => error!("Could not retrieve state: {:?}", e),
Ok(state) => info!("Here is the state we have retrieved: {:?}", state),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,12 @@ use swap_protocols::{
ethereum::EtherRedeem,
Action, StateActions,
},
bitcoin::{bitcoin_htlc, bitcoin_htlc_address},
roles::Alice,
state_machine::*,
Secret,
},
};

impl StateActions for SwapStates<Bitcoin, Ethereum, BitcoinQuantity, EtherQuantity, Secret> {
impl StateActions for SwapStates<Alice<Bitcoin, Ethereum, BitcoinQuantity, EtherQuantity>> {
type Accept = ();
type Decline = ();
type Fund = BitcoinFund;
Expand All @@ -26,7 +25,7 @@ impl StateActions for SwapStates<Bitcoin, Ethereum, BitcoinQuantity, EtherQuanti
match *self {
SS::Start { .. } => vec![],
SS::Accepted(Accepted { ref swap, .. }) => vec![Action::Fund(BitcoinFund {
address: bitcoin_htlc_address(swap),
address: swap.source_htlc_params().compute_address(),
value: swap.source_asset,
})],
SS::SourceFunded { .. } => vec![],
Expand All @@ -44,7 +43,7 @@ impl StateActions for SwapStates<Bitcoin, Ethereum, BitcoinQuantity, EtherQuanti
}),
Action::Refund(BitcoinRefund {
outpoint: *source_htlc_location,
htlc: bitcoin_htlc(swap),
htlc: swap.source_htlc_params().into(),
value: swap.source_asset,
transient_keypair: swap.source_ledger_refund_identity,
}),
Expand All @@ -60,7 +59,7 @@ impl StateActions for SwapStates<Bitcoin, Ethereum, BitcoinQuantity, EtherQuanti
..
}) => vec![Action::Refund(BitcoinRefund {
outpoint: *source_htlc_location,
htlc: bitcoin_htlc(swap),
htlc: swap.source_htlc_params().into(),
value: swap.source_asset,
transient_keypair: swap.source_ledger_refund_identity,
})],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,13 @@ use swap_protocols::{
ethereum::{EtherDeploy, EtherRefund},
Accept, Action, Decline, StateActions,
},
bitcoin::bitcoin_htlc,
ethereum::ethereum_htlc,
ethereum::{EtherHtlc, Htlc},
roles::Bob,
state_machine::*,
SecretHash,
},
};

impl StateActions for SwapStates<Bitcoin, Ethereum, BitcoinQuantity, EtherQuantity, SecretHash> {
impl StateActions for SwapStates<Bob<Bitcoin, Ethereum, BitcoinQuantity, EtherQuantity>> {
type Accept = Accept;
type Decline = Decline;
type Fund = EtherDeploy;
Expand All @@ -28,7 +27,7 @@ impl StateActions for SwapStates<Bitcoin, Ethereum, BitcoinQuantity, EtherQuanti
SS::Start { .. } => vec![Action::Accept(Accept), Action::Decline(Decline)],
SS::Accepted { .. } => vec![],
SS::SourceFunded(SourceFunded { ref swap, .. }) => {
let htlc = ethereum_htlc(swap);
let htlc: EtherHtlc = swap.target_htlc_params().into();
vec![Action::Fund(EtherDeploy {
data: htlc.compile_to_hex().into(),
value: swap.target_asset,
Expand Down Expand Up @@ -68,51 +67,13 @@ impl StateActions for SwapStates<Bitcoin, Ethereum, BitcoinQuantity, EtherQuanti
..
}) => vec![Action::Redeem(BitcoinRedeem {
outpoint: *source_htlc_location,
htlc: bitcoin_htlc(swap),
htlc: swap.source_htlc_params().into(),
value: swap.source_asset,
transient_keypair: swap.source_ledger_refund_identity,
transient_keypair: swap.source_ledger_success_identity,
secret: *secret,
})],
SS::Error(_) => vec![],
SS::Final(_) => vec![],
}
}
}

#[cfg(test)]
mod tests {

use super::*;
use bitcoin_support;
use hex;
use secp256k1_support;
use swap_protocols::rfc003::Secret;

#[test]
fn given_state_instance_when_calling_actions_should_not_need_to_specify_type_arguments() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Can we have this test or a similar one back please? One that proofs that we don't need type arguments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do need the type arguments :^)

There is no way to infer the role from the arguments (unless you made the role an argument but I don't even know if this will fix it as I doubt it will be able to infer the type parameters to Alice or Bob).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please read the test name again!

I was talking about the "type" arguments that are now associated types on the StateActions. Since they are associated, you shouldn't need to specify them when calling the actions method.

The test would ensure that we can use this API in the way we intend. (similar to how a test that instantiates the state machine for Alice and Bob verifies that we can actually do that.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wups. Fixed. Sorry I was lazy.

let swap_state = SwapStates::from(Start {
source_ledger_refund_identity: secp256k1_support::KeyPair::from_secret_key_slice(
&hex::decode("18e14a7b6a307f426a94f8114701e7c8e774e7f9a47e2c2035db29a206321725")
.unwrap(),
)
.unwrap(),
target_ledger_success_identity: "8457037fcd80a8650c4692d7fcfc1d0a96b92867"
.parse()
.unwrap(),
source_ledger: Bitcoin::regtest(),
target_ledger: Ethereum::default(),
source_asset: BitcoinQuantity::from_bitcoin(1.0),
target_asset: EtherQuantity::from_eth(10.0),
source_ledger_lock_duration: bitcoin_support::Blocks::from(144),
secret: Secret::from(*b"hello world, you are beautiful!!").hash(),
});

let actions = swap_state.actions();

assert_eq!(
actions,
vec![Action::Accept(Accept), Action::Decline(Decline)]
);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@ use swap_protocols::{
self,
alice::SwapRequests,
messages::Request,
roles::Alice,
state_machine::{Start, SwapStates},
state_store::StateStore,
ExtractSecret, Ledger, Secret,
Ledger, Secret,
},
};
use swaps::{alice_events, common::SwapId};
Expand Down Expand Up @@ -123,11 +124,9 @@ impl<

fn spawn_state_machine<SL: Ledger, TL: Ledger, SA: Asset, TA: Asset, S: StateStore<SwapId>>(
id: SwapId,
start_state: Start<SL, TL, SA, TA, Secret>,
start_state: Start<Alice<SL, TL, SA, TA>>,
state_store: &S,
) where
TL::Transaction: ExtractSecret,
{
) {
let state = SwapStates::Start(start_state);

// TODO: spawn state machine from state here
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
use bitcoin_support::Transaction;
use swap_protocols::rfc003::secret::{ExtractSecret, Secret, SecretHash};
use swap_protocols::{
ledger::Bitcoin,
rfc003::{
secret::{Secret, SecretHash},
ExtractSecret,
},
};

impl ExtractSecret for Transaction {
fn extract_secret(&self, secret_hash: &SecretHash) -> Option<Secret> {
self.input.iter().find_map(|txin| {
impl ExtractSecret for Bitcoin {
fn extract_secret(transaction: &Transaction, secret_hash: &SecretHash) -> Option<Secret> {
transaction.input.iter().find_map(|txin| {
txin.witness
.iter()
.find_map(|script_item| match Secret::from_vec(&script_item) {
Expand Down Expand Up @@ -50,7 +56,7 @@ mod test {
let secret = Secret::from(*b"This is our favourite passphrase");
let transaction = setup(&secret);

assert_that!(transaction.extract_secret(&secret.hash()))
assert_that!(Bitcoin::extract_secret(&transaction, &secret.hash()))
.is_some()
.is_equal_to(&secret);
}
Expand All @@ -65,7 +71,7 @@ mod test {
bfbfbfbfbfbfbfbfbfbfbfbfbfbfbfbf",
)
.unwrap();
assert_that!(transaction.extract_secret(&secret_hash)).is_none();
assert_that!(Bitcoin::extract_secret(&transaction, &secret_hash)).is_none();
}

#[test]
Expand All @@ -77,7 +83,7 @@ mod test {
.unwrap();
let secret = Secret::from_vec(&hex_secret).unwrap();

assert_that!(transaction.extract_secret(&secret.hash()))
assert_that!(Bitcoin::extract_secret(&transaction, &secret.hash()))
.is_some()
.is_equal_to(&secret);
}
Expand Down
36 changes: 18 additions & 18 deletions application/comit_node/src/swap_protocols/rfc003/bitcoin/mod.rs
Original file line number Diff line number Diff line change
@@ -1,39 +1,39 @@
use bitcoin_support::{Address, BitcoinQuantity, Blocks, OutPoint};
use secp256k1_support::KeyPair;
use swap_protocols::{ledger::Bitcoin, rfc003::Ledger};
use swap_protocols::{
ledger::Bitcoin,
rfc003::{state_machine::HtlcParams, Ledger},
};

mod extract_secret;
mod htlc;
mod queries;
mod validation;

pub use self::{
htlc::{Htlc, UnlockingError},
queries::*,
};
use swap_protocols::{
asset::Asset,
rfc003::{state_machine::OngoingSwap, IntoSecretHash},
};

impl Ledger for Bitcoin {
type LockDuration = Blocks;
type HtlcLocation = OutPoint;
type HtlcIdentity = KeyPair;
}

pub fn bitcoin_htlc<TL: Ledger, TA: Asset, S: IntoSecretHash>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NICE! 👍

swap: &OngoingSwap<Bitcoin, TL, BitcoinQuantity, TA, S>,
) -> Htlc {
Htlc::new(
swap.source_ledger_success_identity,
swap.source_ledger_refund_identity,
swap.secret.clone().into(),
swap.source_ledger_lock_duration.into(),
)
impl From<HtlcParams<Bitcoin, BitcoinQuantity>> for Htlc {
fn from(htlc_params: HtlcParams<Bitcoin, BitcoinQuantity>) -> Self {
Htlc::new(
htlc_params.success_identity,
htlc_params.refund_identity,
htlc_params.secret_hash,
htlc_params.lock_duration.into(),
)
}
}

pub fn bitcoin_htlc_address<TL: Ledger, TA: Asset, S: IntoSecretHash>(
swap: &OngoingSwap<Bitcoin, TL, BitcoinQuantity, TA, S>,
) -> Address {
bitcoin_htlc(swap).compute_address(swap.source_ledger.network)
impl HtlcParams<Bitcoin, BitcoinQuantity> {
pub fn compute_address(&self) -> Address {
Htlc::from(self.clone()).compute_address(self.ledger.network)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 It would be nice if we could avoid this clone. Either re-instantiate the Htlc here or create a private fn to reduce the duplication between this and the From impl.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really know what you mean as a the solution. My preferred solution would be to replace HtlcParams with the actual HTLCs. Lucas and I tried this at some point and it's tricker than adding an associated type on ledger bound to an HTLC trait because the ethereum HTLC depends on the asset.

Now that we have Role which has the asset and the ledger it should be possible to require with a bound that (SL, SA) is able to produce an HTLC together (and generate it generically in state machine and pass it through).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we can optimize later.
It is not as easy as I thought since SecretHash is not Copy and thus, we actually need to clone it. However, SecretHash should actually be just Copy because it can be a [u8; 32].

}
}
61 changes: 16 additions & 45 deletions application/comit_node/src/swap_protocols/rfc003/bitcoin/queries.rs
Original file line number Diff line number Diff line change
@@ -1,73 +1,44 @@
use bitcoin_support::{BitcoinQuantity, OutPoint};
use ledger_query_service::BitcoinQuery;
use swap_protocols::{
asset::Asset,
ledger::Bitcoin,
rfc003::{
bitcoin::bitcoin_htlc_address,
events::{
NewSourceHtlcFundedQuery, NewSourceHtlcRedeemedQuery, NewSourceHtlcRefundedQuery,
},
state_machine::OngoingSwap,
IntoSecretHash, Ledger,
events::{NewHtlcFundedQuery, NewHtlcRedeemedQuery, NewHtlcRefundedQuery},
state_machine::HtlcParams,
},
};

impl<TL, TA, S> NewSourceHtlcFundedQuery<Bitcoin, TL, BitcoinQuantity, TA, S> for BitcoinQuery
where
TL: Ledger,
TA: Asset,
S: IntoSecretHash,
{
fn new_source_htlc_funded_query(
swap: &OngoingSwap<Bitcoin, TL, BitcoinQuantity, TA, S>,
) -> Self {
impl NewHtlcFundedQuery<Bitcoin, BitcoinQuantity> for BitcoinQuery {
fn new_htlc_funded_query(htlc_params: &HtlcParams<Bitcoin, BitcoinQuantity>) -> Self {
BitcoinQuery::Transaction {
to_address: Some(bitcoin_htlc_address(swap)),
to_address: Some(htlc_params.compute_address()),
from_outpoint: None,
unlock_script: None,
}
}
}

impl<TL, TA, S> NewSourceHtlcRefundedQuery<Bitcoin, TL, BitcoinQuantity, TA, S> for BitcoinQuery
where
TL: Ledger,
TA: Asset,
S: IntoSecretHash,
{
fn new_source_htlc_refunded_query(
swap: &OngoingSwap<Bitcoin, TL, BitcoinQuantity, TA, S>,
source_htlc_location: &OutPoint,
impl NewHtlcRefundedQuery<Bitcoin, BitcoinQuantity> for BitcoinQuery {
fn new_htlc_refunded_query(
_htlc_params: &HtlcParams<Bitcoin, BitcoinQuantity>,
htlc_location: &OutPoint,
) -> Self {
BitcoinQuery::Transaction {
to_address: None,
from_outpoint: Some(source_htlc_location.clone()),
unlock_script: Some(vec![
swap.source_ledger_refund_identity
.public_key()
.inner()
.serialize()
.to_vec(),
vec![0u8],
]),
from_outpoint: Some(*htlc_location),
unlock_script: Some(vec![vec![0u8]]),
}
}
}

impl<TL, TA, S> NewSourceHtlcRedeemedQuery<Bitcoin, TL, BitcoinQuantity, TA, S> for BitcoinQuery
where
TL: Ledger,
TA: Asset,
S: IntoSecretHash,
{
fn new_source_htlc_redeemed_query(
_swap: &OngoingSwap<Bitcoin, TL, BitcoinQuantity, TA, S>,
source_htlc_location: &OutPoint,
impl NewHtlcRedeemedQuery<Bitcoin, BitcoinQuantity> for BitcoinQuery {
fn new_htlc_redeemed_query(
_htlc_params: &HtlcParams<Bitcoin, BitcoinQuantity>,
htlc_location: &OutPoint,
) -> Self {
BitcoinQuery::Transaction {
to_address: None,
from_outpoint: Some(source_htlc_location.clone()),
from_outpoint: Some(*htlc_location),
unlock_script: Some(vec![vec![1u8]]),
}
}
Expand Down
Loading