Refactor - Split WalletCommAdapter into multiple traits #180

bddap · 2019-07-06T00:00:45Z

This is WIP, consider it experimental for now.

Edit: Ready for merge

bddap · 2019-07-08T21:56:29Z

Did a bit of investigation on this and it looks like this feature warrants a new WalletCommAdapter.

WalletCommAdapter currently exposes a [https://deterministic.space/elegant-apis-in-rust.html#dont-stringly-type-your-api](stringly typed api) where the 'addr' argument is a string that represents a different type depending the Implementer.

I'm going to start by refactoring WalletCommAdapter, and refactoring implementors of WalletCommAdaptor to hold their address as struct fields. Not only will this make server authentication implementable, it will also be more strongly typed and will enable future efforts such as an i2p comm adaptor.

This reverts commit f52a8d2.

bddap · 2019-07-09T23:50:20Z

WalletCommAdapter declares a listen method, but listen() is not implementable for all implementers of WalletCommAdapter. For example, it doesn't make sense for the FileWalletCommAdapter to listen(). Splitting the WalletCommAdapter trait in twain, or a similar solution, is next on my list after the task mentioned in the previous comment.

yeastplume · 2019-07-10T09:00:19Z

Yes, that trait definitely needs a re-think.

Slightly related, I'm going to propose that we make the grinbox spec a published open standard, and provide support for it in the default wallet. If you're able to look into that and have a think about it as part of all this work, I think it will be very valuable to help reduce the friction for the vast majority of transaction cases.

bddap · 2019-07-10T18:52:31Z

Definitely agree. From what I can tell, grinbox comms can be implemented as another WalletCommAdapter. I'll keep grinbox support as an explicit end goal of these refactors.

…ntors, visiting havok on automated tests, at least one of which is now out of date and failing

…d behaviors.

1. args, a stringly typed argument to put_tx 2. NullAdapter, which is no longer used

bddap

Reviewing this myself. Read over the review for explanations of the changes.

api/src/owner.rs

bddap · 2019-07-12T16:32:39Z

controller/src/command.rs

@@ -116,12 +113,6 @@ pub struct ListenArgs {
 }

 pub fn listen(config: &WalletConfig, args: &ListenArgs, g_args: &GlobalArgs) -> Result<(), Error> {
-	let mut params = HashMap::new();


This object was only serving to hold variables local to the function. The http adapter has no listen implementation, and the keybase adapter never uses it's "params" argument.

bddap · 2019-07-12T16:47:05Z

controller/src/command.rs

-				"file" => FileWalletCommAdapter::new(),
-				"keybase" => KeybaseWalletCommAdapter::new(),
-				"self" => NullWalletCommAdapter::new(),
-				_ => NullWalletCommAdapter::new(),


After the changes in the pr, this general case (when method is not http, file, keybase, or self) now results in an error. Apart from that, previous behavior is preserved.

bddap · 2019-07-12T16:55:02Z

controller/src/command.rs

-				api.tx_lock_outputs(&slate, 0)?;
-				if args.method == "self" {
+
+			match args.method.as_str() {


With WalletCommAdapter split into several different traits, it makes sense to handle a different "methods" explicitly. Hopefully the new match statement is easier to read as well.

Reviewer, please read over this new match statement to double check that it is desired behavior.

bddap · 2019-07-12T16:57:26Z

controller/src/command.rs

@@ -346,8 +332,7 @@ pub fn receive(
 	g_args: &GlobalArgs,
 	args: ReceiveArgs,
 ) -> Result<(), Error> {
-	let adapter = FileWalletCommAdapter::new();
-	let mut slate = adapter.receive_tx_async(&args.input)?;
+	let mut slate = PathToSlate((&args.input).into()).get_tx()?;


(&args.input).into() creates a PathBuf from a String without taking ownership of said String.

bddap · 2019-07-12T18:04:32Z

impls/src/adapters/keybase.rs

-	// Send a slate to a keybase username then wait for a response for TTL seconds.
-	fn send_tx_sync(&self, addr: &str, slate: &Slate) -> Result<Slate, Error> {
-		// Limit only one recipient
-		if addr.matches(",").count() > 0 {


Check moved to constructor.

bddap · 2019-07-12T18:06:51Z

impls/src/adapters/mod.rs

 	/// Start a listener, passing received messages to the wallet api directly
 	/// Takes a wallet config for now to avoid needing all sorts of awkward
 	/// type parameters on this trait
 	fn listen(
 		&self,
-		params: HashMap<String, String>,


Params was a) loosely typed, and b) unused.

bddap · 2019-07-12T18:17:29Z

impls/src/adapters/mod.rs

+}
+
+/// select a SlateSender based on method and dest fields from, e.g., SendArgs
+pub fn create_sender(method: &str, dest: &str) -> Result<Box<dyn SlateSender>, Error> {


If this method does not work in the context of I2P or public key based server authentication, it won't be much trouble to simply delete it. It's only used in two places.

bddap · 2019-07-12T18:20:09Z

impls/src/adapters/null.rs

@@ -1,59 +0,0 @@
-// Copyright 2018 The Grin Developers


A null adapter is no longer needed and is never used.

bddap · 2019-07-12T18:21:18Z

impls/src/test_framework/testclient.rs

@@ -344,55 +343,6 @@ impl LocalWalletClient {
 	}
 }

-impl WalletCommAdapter for LocalWalletClient {


This implementation was never used.

bddap · 2019-07-12T18:43:09Z

So far none of these changes have nothing to do with Public Key based server authentication. Instead the changes so far are general improvements paving the way for new comm adapters.

I'm going to rename this pr, limiting scope to the refactor only.

bddap · 2019-07-12T19:02:00Z

Requesting merge.

Key changes:

Add typed fields to comm adapters to avoid passing of stringly typed arguments to WalletCommAdapter methods.
Split WalletCommAdapter into four separate traits, one for each behavior it previously tried to encompass.

New traits:

SlateSender - Sends transactions to a corresponding SlateReceiver
SlateReceiver - Listens for slates from SlateSender
SlatePutter - Asynchronous version of SlateSender; Posts slates to be read later by a corresponding SlateGetter
SlateGetter - Asynchronous version of SlateReceiver; Checks for a transactions from a corresponding SlatePutter, returns the transaction if it exists

Implementation matrix:

	PathToSlate	HttpSlateSender	KeybaseAllChannels	KeybaseChannel
SlateSender	✗	✓	✗	✓
SlateReceiver	✗	✗	✓	✗
SlatePutter	✓	✗	✗	✗
SlateGetter	✓	✗	✗	✗

yeastplume · 2019-07-15T08:16:49Z

All looks really good, thanks for doing this refactor and yes, the previous trait didn't make a lot of sense, this is much more coherent.

I'm going to merge this into the milestone\2.1.0 branch (we're keeping master for any required fixes or changes to 2.0.0). This should give us plenty of time to test before the target release anyhow. Also need to merge these changes onto my own current refactor at #184, so we'll need a decent round of testing before 2.1.0 anyhow.

* version bump for next potential release * Merge master into milestone/2.1.0 (#182) * Derive --version output dynamically from cargo package version (#174) * add --txid to the `wallet txs` command (#176) * add --txid to the `wallet txs` command * add test for `wallet txs` command with `--txid` parameter * Refactor - Split WalletCommAdapter into multiple traits (#180) * Derive --version output dynamically from cargo package version (#174) * add server auth argument to http client * Revert "add server auth argument to http client" This reverts commit f52a8d2. * modify WalletCommAdapter, moving dest argument into fields on implementors, visiting havok on automated tests, at least one of which is now out of date and failing * Split WalletCommAdapter into four traits, one for each of its intended behaviors. * Remove two vestigals 1. args, a stringly typed argument to put_tx 2. NullAdapter, which is no longer used * Remove unused "params" argument from listen method. * Re-add previously existing TODO comment * Fix non-test build * completely Fix non-test build * Full Lifecycle API Support (#184) * refactoring wallet lib traits * rustfmt * rustfmt * add new files * explicit lifetime specifiers on all wallet traits * rustfmt * modify apis to use new walletinst * rustfmt * converting controller crate * rustfmt * controller crate compiling * rustfmt * compilation * rustfmt * Remove config from wallet, implement open_wallet, close_wallet in lifecycle provider, remove password + open_with_credentials from WalletBackend + impl * rustfmt * full compilation, changing recovery + init to new model * rustfmt * wallet initialisation working, init command output and flow identical to v2.0.0 wallet * rustfmt * fix listener and owner api startup * rustfmt * rustfmt * move encryption test * rustfmt * fix api doctests * rustfmt * fix for most tests in controller crate * rustfmt * fix for check tests in controller crate * fix main wallet tests * rustfmt * add explicit functions to handle mnemonic recovery, fix CLI recovery workflow * rustfmt * update keybase adapter to use new wallet format * rustfmt * test fix * remove debug output

* version bump for next potential release * Merge master into milestone/2.1.0 (mimblewimble#182) * Derive --version output dynamically from cargo package version (mimblewimble#174) * add --txid to the `wallet txs` command (mimblewimble#176) * add --txid to the `wallet txs` command * add test for `wallet txs` command with `--txid` parameter * Refactor - Split WalletCommAdapter into multiple traits (mimblewimble#180) * Derive --version output dynamically from cargo package version (mimblewimble#174) * add server auth argument to http client * Revert "add server auth argument to http client" This reverts commit f52a8d2. * modify WalletCommAdapter, moving dest argument into fields on implementors, visiting havok on automated tests, at least one of which is now out of date and failing * Split WalletCommAdapter into four traits, one for each of its intended behaviors. * Remove two vestigals 1. args, a stringly typed argument to put_tx 2. NullAdapter, which is no longer used * Remove unused "params" argument from listen method. * Re-add previously existing TODO comment * Fix non-test build * completely Fix non-test build * Full Lifecycle API Support (mimblewimble#184) * refactoring wallet lib traits * rustfmt * rustfmt * add new files * explicit lifetime specifiers on all wallet traits * rustfmt * modify apis to use new walletinst * rustfmt * converting controller crate * rustfmt * controller crate compiling * rustfmt * compilation * rustfmt * Remove config from wallet, implement open_wallet, close_wallet in lifecycle provider, remove password + open_with_credentials from WalletBackend + impl * rustfmt * full compilation, changing recovery + init to new model * rustfmt * wallet initialisation working, init command output and flow identical to v2.0.0 wallet * rustfmt * fix listener and owner api startup * rustfmt * rustfmt * move encryption test * rustfmt * fix api doctests * rustfmt * fix for most tests in controller crate * rustfmt * fix for check tests in controller crate * fix main wallet tests * rustfmt * add explicit functions to handle mnemonic recovery, fix CLI recovery workflow * rustfmt * update keybase adapter to use new wallet format * rustfmt * test fix * remove debug output

yeastplume and others added 2 commits July 2, 2019 10:24

Derive --version output dynamically from cargo package version (mimbl…

21dd3d5

…ewimble#174)

add server auth argument to http client

f52a8d2

bddap force-pushed the wallet-authentication branch from 03a9a57 to f52a8d2 Compare July 8, 2019 19:33

Revert "add server auth argument to http client"

ed4bc9b

This reverts commit f52a8d2.

bddap added 5 commits July 10, 2019 12:41

modify WalletCommAdapter, moving dest argument into fields on impleme…

1a31931

…ntors, visiting havok on automated tests, at least one of which is now out of date and failing

Split WalletCommAdapter into four traits, one for each of its intende…

0e37b0f

…d behaviors.

Merge branch 'master' into wallet-authentication

b2585eb

Remove two vestigals

840eb49

1. args, a stringly typed argument to put_tx 2. NullAdapter, which is no longer used

Remove unused "params" argument from listen method.

6beef85

bddap commented Jul 12, 2019

View reviewed changes

bddap changed the title ~~[WIP] Public key Server authentication for https wallets without PKI~~ Refactor - Split WalletCommAdapter into multiple traits Jul 12, 2019

bddap added 2 commits July 12, 2019 12:05

Re-add previously existing TODO comment

acaf66e

Fix non-test build

42f5061

bddap mentioned this pull request Jul 12, 2019

[WIP] Server authentication in transactions #186

Closed

completely Fix non-test build

4700090

yeastplume changed the base branch from master to milestone/2.1.0 July 15, 2019 08:17

Merge branch 'milestone/2.1.0' into wallet-authentication

669f29e

yeastplume merged commit 1e8359b into mimblewimble:milestone/2.1.0 Jul 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor - Split WalletCommAdapter into multiple traits #180

Refactor - Split WalletCommAdapter into multiple traits #180

bddap commented Jul 6, 2019 •

edited

Loading

bddap commented Jul 8, 2019

bddap commented Jul 9, 2019

yeastplume commented Jul 10, 2019

bddap commented Jul 10, 2019

bddap left a comment

bddap Jul 12, 2019

bddap Jul 12, 2019

bddap Jul 12, 2019

bddap Jul 12, 2019

bddap Jul 12, 2019

bddap Jul 12, 2019

bddap Jul 12, 2019

bddap Jul 12, 2019

bddap Jul 12, 2019

bddap commented Jul 12, 2019

bddap commented Jul 12, 2019

yeastplume commented Jul 15, 2019

Refactor - Split WalletCommAdapter into multiple traits #180

Refactor - Split WalletCommAdapter into multiple traits #180

Conversation

bddap commented Jul 6, 2019 • edited Loading

bddap commented Jul 8, 2019

bddap commented Jul 9, 2019

yeastplume commented Jul 10, 2019

bddap commented Jul 10, 2019

bddap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bddap commented Jul 12, 2019

bddap commented Jul 12, 2019

yeastplume commented Jul 15, 2019

bddap commented Jul 6, 2019 •

edited

Loading