Full Lifecycle API Support #184

yeastplume · 2019-07-09T13:29:47Z

Exploratory PR being developed in tandem with: mimblewimble/grin-rfcs#11

Thus far, this:

Creates a new wallet Trait, WalletLCProvider, which is meant to provide implementations of the lifecycle methods as found in the RFC
Redefines the 'WalletInst' trait to one that contains all the necessary components of a wallet, and use that trait everywhere where a wallet instance is held
Put explicit lifetimes on the wallet traits, to avoid issues with 'static lifetime requirements
Default implementation of a WalletLCProvider (DefautLCProvider) and WalletInst (DefaultWalletInst) in the impls crate, which should replace the older static instantiation methods and allow for the wallet backend to be re-loaded and swapped in/out etc during a running instance

Also, this is an all-out war with the Rust compiler, but getting there.

…ecycle provider, remove password + open_with_credentials from WalletBackend + impl

yeastplume · 2019-07-10T16:05:51Z

Remove 'WalletConfig' from underlying wallet traits, backend implementations should only need the data directory
change 'open_with_credentials' in WalletBackend to 'set_keychain', opening of the keychain and setting it in the wallet should be the job of a lifecycle provider.
Once the wallet keychain is opened, it remains open until end of execution or 'close_wallet' is called. This is in contrast to the previous state, where it was re-opened during every API call.

… to v2.0.0 wallet

…workflow

yeastplume · 2019-07-23T14:35:45Z

Ready for any review now, paves the way for changes in the associated RFC without actually changing anything external for users or wallet developers.

Note there's an internal change whereby the seed is now kept in memory instead of the password as before when the wallet is listening. This won't be the case at the next release, where we'd be expecting to replace this with an XORed token value (see current proposal of security model in RFC)

rentenmark

didn't take a look at impls/src/lifecycle/default.rs yet, obviously that is where the meat and potatoes is

The new type is really quite unfortunate and verbose, not sure there is any way around that

overall this is an absolutely massive PR that has a few different refactors in it, hopefully no one else is working in these files but I suppose they're not

I'm not sure that changes to the wallet cli are unit tested but that functionality has been changed due to refactor, would be worth testing out. There should probably be a smoke test suite that tests functionality via CLI (maybe there already is?)

rentenmark · 2019-07-24T12:36:05Z

api/src/foreign.rs

@@ -462,11 +471,23 @@ macro_rules! doctest_helper_setup_doc_env_foreign {
 			.unwrap();
 		let mut wallet_config = WalletConfig::default();
 		wallet_config.data_file_dir = dir.to_owned();
-		let pw = "";
+		let pw = ZeroingString::from("");


it would be cool to see a lazy static for this. C# for example has string.Empty etc

Think this is okay, it's just test setup

rentenmark · 2019-07-24T12:39:33Z

api/src/owner_rpc.rs

@@ -1338,7 +1340,8 @@ pub fn run_doctest_owner(
 ) -> Result<Option<serde_json::Value>, String> {


this function has a lot of unwrap. I understand that it is a test helper but even so, it seems un-necessary not sure how difficult the refactor of this return type would be

I think unwraps in tests are okay for now, happy to see a PR later that removes them (a lot of these tests were written before you could return a Result from main or from a test).

rentenmark · 2019-07-24T12:41:26Z

config/src/config.rs

 	// Use config file if current directory if it exists, .grin home otherwise
 	if let Some(p) = check_config_current_dir(WALLET_CONFIG_FILE_NAME) {
 		GlobalWalletConfig::new(p.to_str().unwrap())
 	} else {
 		// Check if grin dir exists
-		let grin_path = get_grin_path(chain_type)?;
+		let grin_path = match data_path {


could probably use unwrap_or, it's a style question

rentenmark · 2019-07-24T12:42:14Z

config/src/config.rs


 		// Get path to default config file
 		let mut config_path = grin_path.clone();
 		config_path.push(WALLET_CONFIG_FILE_NAME);

-		// Spit it out if it doesn't exist
+		println!("CONFIG PATH: {}", config_path.to_str().unwrap());


should this be info! ?

Leftover debug println, thanks for catching!

rentenmark · 2019-07-24T12:44:06Z

controller/src/controller.rs

@@ -131,35 +135,36 @@ where

 	// If so configured, add the foreign API to the same port
 	if owner_api_include_foreign.unwrap_or(false) {
-		info!("Starting HTTP Foreign API on Owner server at {}.", addr);
-		let foreign_api_handler_v2 = ForeignAPIHandlerV2::new(wallet.clone());
+		warn!("Starting HTTP Foreign API on Owner server at {}.", addr);


why info->warn ? if we want to see the messages we can set the log level, these don't look like warnings to me

Changed this for consistency with how it's done in other places. The default in grin.toml is warn, and I think we want to make sure something like that is definitely printed to the console by default.

rentenmark · 2019-07-24T12:44:33Z

controller/src/controller.rs

-pub fn foreign_listener<T: ?Sized, C, K>(
-	wallet: Arc<Mutex<T>>,
+pub fn foreign_listener<L, C, K>(
+	wallet: Arc<Mutex<Box<dyn WalletInst<'static, L, C, K> + 'static>>>,


this type is unfortunate but i understand how it came to be

Indeed... the joys of rust. At least it works how you think it's going to work once you're done fighting with the compiler

yeastplume · 2019-07-25T11:08:21Z

Thanks for review, wallet APIs are definitely exercised extensively by tests, the CLI is as well but a bit less so. I've manually tested that all of the init/recover/restore operations work exactly as they did before these changes.

More tests are definitely needed, however I'm planning to replace most of the CLI code with the CLI code borrowed from wallet713, to make the whole thing more cohesive. Will definitely be adding to the test suite at that stage to ensure all operations are tested at the CLI/listener level as well.

DavidBurkett

So many changes. Overall direction looks good, but I'm hoping now that you've finished laying the groundwork for the full lifecycle, we can start having smaller, more incremental reviews? I suggest for the new APIs we're going to be adding, there should probably be a review per API. It's a bit slower with more overhead, but it's probably the best way to get real input from wallet devs about what the APIs should look like. It would also be a natural way of splitting up the work to let other devs get their feet wet.

yeastplume · 2019-07-29T11:05:40Z

Yes, next bits of functionality to go in related the the current RFC are much more easily split-up. I don't think there will be a need for another refactor of this size (in the foreseeable future, 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

yeastplume · 2019-09-09T14:04:33Z

#212

* 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 added 16 commits July 9, 2019 11:28

refactoring wallet lib traits

89e83cd

rustfmt

60d89ca

rustfmt

c0410d2

add new files

82e6cf1

explicit lifetime specifiers on all wallet traits

1c938ac

rustfmt

d0d5907

modify apis to use new walletinst

5e491aa

rustfmt

ba4a2a7

converting controller crate

8094669

rustfmt

e84d60f

controller crate compiling

dd17ce0

rustfmt

2274435

compilation

6b41858

rustfmt

628eb4f

Remove config from wallet, implement open_wallet, close_wallet in lif…

1deaceb

…ecycle provider, remove password + open_with_credentials from WalletBackend + impl

rustfmt

3092863

yeastplume added 6 commits July 11, 2019 13:37

full compilation, changing recovery + init to new model

b0304ed

rustfmt

1541f18

wallet initialisation working, init command output and flow identical…

22fc769

… to v2.0.0 wallet

rustfmt

bba7196

fix listener and owner api startup

b43ed4f

rustfmt

29b3e79

yeastplume mentioned this pull request Jul 15, 2019

Refactor - Split WalletCommAdapter into multiple traits #180

Merged

yeastplume added 6 commits July 15, 2019 10:09

merge from 2.1.0 upstream

f7a587c

rustfmt

fe2dc09

move encryption test

5ccc4a8

rustfmt

59cb6bc

fix api doctests

1f0dbed

rustfmt

ccfdb68

yeastplume added 9 commits July 15, 2019 15:02

fix for most tests in controller crate

363ee07

rustfmt

d2c72c8

fix for check tests in controller crate

3453480

fix main wallet tests

9866ce6

rustfmt

0870c03

add explicit functions to handle mnemonic recovery, fix CLI recovery …

af83011

…workflow

rustfmt

e157854

update keybase adapter to use new wallet format

c1c9779

rustfmt

e267ce1

yeastplume changed the title ~~[WIP] Full Lifecycle API Support~~ Full Lifecycle API Support Jul 23, 2019

test fix

d712bb2

rentenmark reviewed Jul 24, 2019

View reviewed changes

remove debug output

82854c3

DavidBurkett approved these changes Jul 27, 2019

View reviewed changes

yeastplume merged commit c8e51bc into mimblewimble:milestone/2.1.0 Jul 29, 2019

yeastplume deleted the full_lifecycle_api branch August 13, 2019 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full Lifecycle API Support #184

Full Lifecycle API Support #184

yeastplume commented Jul 9, 2019

yeastplume commented Jul 10, 2019

yeastplume commented Jul 23, 2019

rentenmark left a comment •

edited

Loading

rentenmark Jul 24, 2019

yeastplume Jul 25, 2019

rentenmark Jul 24, 2019

yeastplume Jul 25, 2019

rentenmark Jul 24, 2019

rentenmark Jul 24, 2019

yeastplume Jul 25, 2019

rentenmark Jul 24, 2019

yeastplume Jul 25, 2019

rentenmark Jul 24, 2019

yeastplume Jul 25, 2019

yeastplume commented Jul 25, 2019

DavidBurkett left a comment

yeastplume commented Jul 29, 2019

yeastplume commented Sep 9, 2019

		@@ -1338,7 +1340,8 @@ pub fn run_doctest_owner(
		) -> Result<Option<serde_json::Value>, String> {

Full Lifecycle API Support #184

Full Lifecycle API Support #184

Conversation

yeastplume commented Jul 9, 2019

yeastplume commented Jul 10, 2019

yeastplume commented Jul 23, 2019

rentenmark left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeastplume commented Jul 25, 2019

DavidBurkett left a comment

Choose a reason for hiding this comment

yeastplume commented Jul 29, 2019

yeastplume commented Sep 9, 2019

rentenmark left a comment •

edited

Loading