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

LLFourn · 2018-11-16T04:08:13Z

Role/IntoSecret is no longer used in events/queries/validation code 🎉
State machine takes a single type parameter which is role which has
defines the types for the state
The response event is produced by another field of type ResponseSource
on context to allow Events to be specifically for ledger events.

thomaseizinger · 2018-11-16T04:11:48Z

application/comit_node/src/swap_protocols/rfc003/actions/bob/btc_eth.rs

-    use swap_protocols::rfc003::Secret;
-
-    #[test]
-    fn given_state_instance_when_calling_actions_should_not_need_to_specify_type_arguments() {


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

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).

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.)

wups. Fixed. Sorry I was lazy.

thomaseizinger · 2018-11-16T04:12:15Z

application/comit_node/src/swap_protocols/rfc003/bitcoin/mod.rs


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

-pub fn bitcoin_htlc<TL: Ledger, TA: Asset, S: IntoSecretHash>(


thomaseizinger · 2018-11-16T04:14:23Z

application/comit_node/src/swap_protocols/rfc003/ethereum/validation.rs

+        _htlc_params: &HtlcParams<Ethereum, EtherQuantity>,
+        _transaction: ethereum_support::Transaction,
+    ) -> Result<ethereum_support::Address, Error<Self>> {
+        unimplemented!()


❌ This is already implemented on master:

https://github.com/coblox/comit-rs/blob/d82d170d6836b98e4fb183158e23ee33d435216a/application/comit_node/src/swap_protocols/rfc003/validation.rs#L92-L121

Please include the implementation here.

thomaseizinger · 2018-11-16T04:18:18Z

application/comit_node/src/swap_protocols/rfc003/roles.rs

+    }
+}
+
+impl<SL: Ledger, TL: Ledger, SA, TA> Clone for Alice<SL, TL, SA, TA> {


❌ Given Alice is only a PhantomData, she should be cloneable?

thomaseizinger · 2018-11-16T04:19:40Z

application/comit_node/src/swap_protocols/rfc003/state_machine.rs

+    > + Send;
+
+#[allow(type_alias_bounds)]
+pub type ResponseFuture<R: Role> = StateMachineResponseFuture<


❓ Should these type definitions move into events/mod.rs?

The idea is to rename Events to LedgerEvents since this response is no longer part of them, so this shouldn't be moved there.

Then I would suggest to do this renaming right away :)
The ResponseFuture is used by the roles right? Maybe it should be moved there then?

Moved it back there

thomaseizinger · 2018-11-16T04:23:04Z

application/comit_node/src/swap_protocols/rfc003/validation.rs

@@ -16,437 +10,13 @@ pub enum Error<A: Asset> {
    WrongTransaction,
 }

-pub trait IsContainedInSourceLedgerTransaction<SL, TL, TA, S>: Send + Sync
+pub trait IsContainedInTransaction<L>: Send + Sync


🔧 Now that this file only contains this trait, maybe rename it to according the trait name?

thomaseizinger · 2018-11-18T22:46:06Z

application/comit_node/src/swap_protocols/rfc003/bitcoin/mod.rs

-    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)


🔧 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.

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).

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].

D4nte

Don't forget to rebase.

application/comit_node/src/swap_protocols/rfc003/bitcoin/queries.rs

application/comit_node/src/swap_protocols/rfc003/roles.rs

thomaseizinger · 2018-11-18T22:49:01Z

application/comit_node/src/swap_protocols/rfc003/bitcoin/queries.rs

        BitcoinQuery::Transaction {
-            to_address: Some(bitcoin_htlc_address(swap)),
+            to_address: Some(htlc.compute_address(htlc_params.ledger.network)),


🔧 You can call the compute_address fn here.

thomaseizinger · 2018-11-19T02:43:50Z

application/comit_node/src/swap_protocols/rfc003/bitcoin/validation.rs

+        let htlc_params = gen_htlc_params(bitcoin_amount);
+        let script = htlc_params.compute_address().script_pubkey();
+
+        let script_pub_key = ScriptPubKey {


🤡 I know that you copied this from the previous test but it would be nice if this would be more realistic test data :)

It would be nice I guess but I don't think there's much point in putting more realistic data unless it needs it to pass.

thomaseizinger · 2018-11-19T02:45:04Z

application/comit_node/src/swap_protocols/rfc003/bitcoin/validation.rs

+            script_pub_key,
+        };
+
+        let transaction = VerboseRawTransaction {


❌ There is actually no more need to instantiate a VerboseRawTransaction here. Should just be a bitcoin::Transaction.

I am fixing this up but why did we ever do this?

Because LQS returned VerboseRawTransaction at some point so Bitcoin::Transaction was VerboseRawTransaction.

thomaseizinger · 2018-11-19T02:46:58Z

application/comit_node/src/swap_protocols/rfc003/events/mod.rs

+{
+}
+
+pub trait ResponseEvent<R: Role> {


❌ We had the naming convention before that the method name is also the trait name. Please be consistent, which ever way (Event postfix or same name) you go :)

I don't think that's a good convention can we discuss this?

Resolved by renaming to CommunicationEvents

thomaseizinger · 2018-11-19T02:47:29Z

application/comit_node/src/swap_protocols/rfc003/events/mod.rs

-        target_htlc_location: &TL::HtlcLocation,
+    fn new_htlc_refunded_query(
+        htlc_params: &HtlcParams<L, A>,
+        source_htlc_location: &L::HtlcLocation,


❌ This variable name should just be htlc_location.

thomaseizinger · 2018-11-19T02:47:46Z

application/comit_node/src/swap_protocols/rfc003/events/mod.rs

-        source_htlc_location: &SL::HtlcLocation,
+    fn new_htlc_redeemed_query(
+        htlc_params: &HtlcParams<L, A>,
+        source_htlc_location: &L::HtlcLocation,


❌ Variable name should just be htlc_location.

thomaseizinger · 2018-11-19T02:52:10Z

application/comit_node/src/swap_protocols/rfc003/ledger.rs


-pub trait Ledger: swap_protocols::ledger::Ledger {
+pub trait Ledger: LedgerExtractSecret {


❌ I find this to be very strange.

Why do we need this 2nd trait?

We need a trait on the ledger to ensure we don't have to write where TL::Ledger: ExtractSecret bounds everywhere.

We could delete the one on Transaction in place of this.

Trait is now on Ledger and only on Ledger

- Role/IntoSecret is no longer used in events/queries/validation code 🎉 - State machine takes a single type parameter which is role which has defines the types for the state - The response event is produced by another field of type ResponseSource on context to allow Events to be specifically for ledger events.

running end-to-end updated it for me🤷‍♂️

🤷‍♂️

..And delete LedgerExtractSecret

bonomat · 2018-11-19T04:53:31Z

application/comit_node/src/swap_protocols/rfc003/bitcoin/validation.rs

+    },
+};
+
+impl IsContainedInTransaction<Bitcoin> for BitcoinQuantity {


🔧 It feels like this should be implemented for (Bitcoin)Transaction, e.g.

impl ContainsHtlcParams<Bitcoin> for Transaction { fn contains_htlc_params( htlc_params: &HtlcParams<Bitcoin, BitcoinQuantity>, ) -> Result<OutPoint, Error<BitcoinQuantity>> { ... }

This feels more natural to me. What do you think?

You implemented it for BitcoinQuantity though? I guess you meant Transaction. But yes I agree with you in general. This doesn't have much to do with the asset. We are not checking the "asset" is in the transaction we are checking that the correct contract is deployed!

my preferred solution is to ask the HTLC itself whether it is "in" the transaction i.e. "was I deployed in this transaction?". But we can't generate the HTLC generically yet (all we have is htlc_params)

I think we should leave it for this PR.

yes correct, fixed my code example 😬

Implementing it for HTLC or HTLCParams would be also an option.
Any other thoughts? @thomaseizinger

The reason the trait is designed like this is because I needed trait-bounds like TA: IsContainedInTransaction.

application/comit_node/src/swap_protocols/rfc003/ethereum/validation.rs

bonomat · 2018-11-19T05:04:12Z

application/comit_node/src/swap_protocols/rfc003/ledger.rs

@@ -19,3 +22,22 @@ pub trait Ledger: swap_protocols::ledger::Ledger {
        + Debug
        + Into<<Self as swap_protocols::ledger::Ledger>::Identity>;
 }
+
+pub trait LedgerExtractSecret: swap_protocols::ledger::Ledger {


❓ Where is used and what for?

It's gone! Now we just have ExtractSecret for ledger. As to why we have to do this on the Ledger rather than on the Transaction: It's annoying to have where <TA as swap_protocols::ledger::Ledger>::Transaction: ExtractSecret everywhere. Constraining it on the ledger forces it to be implemented when the Ledger trait is implemented.

If this issue was fixed we could go back to having it on the Transaction: rust-lang/rust#28055

And rename ResponseEvent to CommunicationEvents

bonomat

Good feels this PR :)
I've added a comment regarding the trait IsContainedInTransaction as I think this is better implemented for Transaction than for *Quantity

1550: Bump uuid from 0.7.4 to 0.8.1 r=mergify[bot] a=dependabot-preview[bot] Bumps [uuid](https://github.com/uuid-rs/uuid) from 0.7.4 to 0.8.1. <details> <summary>Release notes</summary> *Sourced from [uuid's releases](https://github.com/uuid-rs/uuid/releases).* > ## uuid 0.8.1: Docs > This release doesn't contain any source changes, it fixes up errors in the docs, adds clarification and more examples. > > # Contributions > > * Tidy up some docs. ([#434](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/434)) > > ## uuid 0.8.0: API polish and consolidation > This release is pretty substantial; it's the accumulation of some 8 months of work. It includes changes to error types, 128bit integer support, explicit endianness and V1 timestamps. > > # Contributions > > - [#427](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/427) > - [#419](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/419) > - [#424](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/424) > - [#418](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/418) > - [#413](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/413) > - [#407](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/407) > - [#404](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/404) > - [#400](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/400) > - [#399](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/399) > - [#398](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/398) > - [#397](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/397) > - [#396](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/396) > - [#394](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/394) > - [#393](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/393) > - [#390](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/390) > - [#389](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/389) > - [#388](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/388) </details> <details> <summary>Commits</summary> - [`fb62500`](uuid-rs/uuid@fb62500) Merge [#435](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/435) - [`5b12da4`](uuid-rs/uuid@5b12da4) prepare for 0.8.1 release - [`db2fe00`](uuid-rs/uuid@db2fe00) Merge [#434](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/434) - [`1c37051`](uuid-rs/uuid@1c37051) tidy up some docs - [`aa6d78f`](uuid-rs/uuid@aa6d78f) Merge [#433](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/433) - [`b03c098`](uuid-rs/uuid@b03c098) use simpler version bound in readme - [`2799026`](uuid-rs/uuid@2799026) prepare for 0.8.0 release - [`1b8308b`](uuid-rs/uuid@1b8308b) Merge [#427](https://github-redirect.dependabot.com/uuid-rs/uuid/issues/427) - [`2497ac7`](uuid-rs/uuid@2497ac7) tweak feature testing - [`c6d39af`](uuid-rs/uuid@c6d39af) run fmt - Additional commits viewable in [compare view](uuid-rs/uuid@0.7.4...0.8.1) </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=uuid&package-manager=cargo&previous-version=0.7.4&new-version=0.8.1)](https://dependabot.com/compatibility-score.html?dependency-name=uuid&package-manager=cargo&previous-version=0.7.4&new-version=0.8.1) You can trigger a rebase of this PR by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in the `.dependabot/config.yml` file in this repo: - Update frequency - Automerge options (never/patch/minor, and dev/runtime dependencies) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) </details> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

thomaseizinger reviewed Nov 16, 2018

View reviewed changes

LLFourn mentioned this pull request Nov 18, 2018

Alpha and Beta #430

Merged

D4nte added this to the Sprint 0 milestone Nov 18, 2018

thomaseizinger reviewed Nov 18, 2018

View reviewed changes

D4nte reviewed Nov 18, 2018

View reviewed changes

application/comit_node/src/swap_protocols/rfc003/bitcoin/queries.rs Show resolved Hide resolved

application/comit_node/src/swap_protocols/rfc003/roles.rs Outdated Show resolved Hide resolved

application/comit_node/src/swap_protocols/rfc003/roles.rs Outdated Show resolved Hide resolved

thomaseizinger reviewed Nov 18, 2018

View reviewed changes

LLFourn force-pushed the instantiate-state-406 branch from dbfe65c to 870cab0 Compare November 19, 2018 01:17

thomaseizinger reviewed Nov 19, 2018

View reviewed changes

LLFourn added 5 commits November 19, 2018 14:34

Update package-lock.json

14fe1b9

running end-to-end updated it for me🤷‍♂️

Add test for Bob to get to SourceRefunded in state machine

13970b1

Re-implement ether IsContainedInTransaction

d99200e

Move/Rename various things in rfc003

52b013d

LLFourn force-pushed the instantiate-state-406 branch from 4c26b7f to e639143 Compare November 19, 2018 03:34

LLFourn added 3 commits November 19, 2018 15:41

Use bitcoin_support::Transaction instead of VerboseRawTransaction

da6c204

🤷‍♂️

s/source_htlc_location/htlc_location/

cc1e5fd

Move ExtractSecret trait to Ledger

33ef1c7

..And delete LedgerExtractSecret

bonomat reviewed Nov 19, 2018

View reviewed changes

application/comit_node/src/swap_protocols/rfc003/ethereum/validation.rs Show resolved Hide resolved

bonomat reviewed Nov 19, 2018

View reviewed changes

Put LedgerEvents functions on the trait

f0f80cb

And rename ResponseEvent to CommunicationEvents

LLFourn force-pushed the instantiate-state-406 branch from e639143 to f0f80cb Compare November 19, 2018 05:18

bonomat suggested changes Nov 19, 2018

View reviewed changes

Restore erroneously deleted test

bd22a72

bonomat approved these changes Nov 19, 2018

View reviewed changes

LLFourn merged commit f82d6c9 into master Nov 19, 2018

LLFourn deleted the instantiate-state-406 branch November 19, 2018 05:54

LLFourn mentioned this pull request Nov 19, 2018

Instantiate state machine for Alice and Bob #406

Closed

3 tasks


		pub trait Ledger: swap_protocols::ledger::Ledger {
		pub trait Ledger: LedgerExtractSecret {

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

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

Uh oh!

Conversation

LLFourn commented Nov 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

D4nte left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomaseizinger Nov 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bonomat Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LLFourn Nov 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomaseizinger Nov 18, 2018 •

edited

Loading

bonomat Nov 19, 2018 •

edited

Loading

LLFourn Nov 19, 2018 •

edited

Loading

bonomat Nov 19, 2018 •

edited

Loading