Skip to content
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

Full Lifecycle API Support #184

Merged
merged 39 commits into from Jul 29, 2019

Conversation

@yeastplume
Copy link
Member

commented Jul 9, 2019

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.

yeastplume added 16 commits Jul 9, 2019
Remove config from wallet, implement open_wallet, close_wallet in lif…
…ecycle provider, remove password + open_with_credentials from WalletBackend + impl
@yeastplume

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

  • 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.
yeastplume added 6 commits Jul 11, 2019
yeastplume added 6 commits Jul 15, 2019
yeastplume added 9 commits Jul 15, 2019

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

@yeastplume

This comment has been minimized.

Copy link
Member Author

commented Jul 23, 2019

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
Copy link

left a comment

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

@@ -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("");

This comment has been minimized.

Copy link
@rentenmark

rentenmark Jul 24, 2019

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

This comment has been minimized.

Copy link
@yeastplume

yeastplume Jul 25, 2019

Author Member

Think this is okay, it's just test setup

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

This comment has been minimized.

Copy link
@rentenmark

rentenmark Jul 24, 2019

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

This comment has been minimized.

Copy link
@yeastplume

yeastplume Jul 25, 2019

Author Member

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

// 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 {

This comment has been minimized.

Copy link
@rentenmark

rentenmark Jul 24, 2019

could probably use unwrap_or, it's a style question


// 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());

This comment has been minimized.

Copy link
@rentenmark

rentenmark Jul 24, 2019

should this be info! ?

This comment has been minimized.

Copy link
@yeastplume

yeastplume Jul 25, 2019

Author Member

Leftover debug println, thanks for catching!

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

This comment has been minimized.

Copy link
@rentenmark

rentenmark Jul 24, 2019

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

This comment has been minimized.

Copy link
@yeastplume

yeastplume Jul 25, 2019

Author Member

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.

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 comment has been minimized.

Copy link
@rentenmark

rentenmark Jul 24, 2019

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

This comment has been minimized.

Copy link
@yeastplume

yeastplume Jul 25, 2019

Author Member

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

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

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
Copy link

left a comment

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

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

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

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

9 checks passed

mimblewimble.grin-wallet Build #20190725.1 succeeded
Details
mimblewimble.grin-wallet (linux config/libwallet/api) linux config/libwallet/api succeeded
Details
mimblewimble.grin-wallet (linux controller/all) linux controller/all succeeded
Details
mimblewimble.grin-wallet (linux impls) linux impls succeeded
Details
mimblewimble.grin-wallet (linux release) linux release succeeded
Details
mimblewimble.grin-wallet (macos release) macos release succeeded
Details
mimblewimble.grin-wallet (macos test) macos test succeeded
Details
mimblewimble.grin-wallet (windows release) windows release succeeded
Details
mimblewimble.grin-wallet (windows test) windows test succeeded
Details
yeastplume added a commit that referenced this pull request Jul 29, 2019
Merge milestone/2.1.0 into master (#199)
* 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 yeastplume deleted the yeastplume:full_lifecycle_api branch Aug 13, 2019

@yeastplume

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.