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

V2 slate versioning enablement #85

Merged
merged 10 commits into from Apr 23, 2019

Conversation

Projects
None yet
2 participants
@yeastplume
Copy link
Member

commented Apr 18, 2019

Implementation enabling the V2 api to accept differing versions of a Slate as suggested by @bddap in #84. (Would appreciate a bit of a review to see if there's anything here than can be made simpler)

  • Adds a VersionedSlate type with methods as suggested in the issue
  • Modifies the foreign api's receive_tx function to use a versioned slate
  • Adds another conversion of Slate Implementation Itself -> Most recent slate definition. (e.g. Slate->SlateV2 and back), to facilitate the conversion pipeline. Maintenance to include new slate definitions in the conversion pipeline should still remain constant (Slate <-> Most recent (a non-conversion, really) and Most Recent <->2nd most recent will need to be implemented each time there's a new slate version)
  • Test to call the foreign RPC api with an older V1 slate and parse the result (which works)

Glad to have this in before 1.1.0 release as knowing this is in place will make slate changes much easier going forward.

Also note this won't compile until a tiny PR is merged in Grin, being created right now.

yeastplume added some commits Apr 15, 2019

@bddap
Copy link
Contributor

left a comment

Higher level question: If slates can always be converted from V0 format, why don't we always use V0 for network communications?

match *self {
VersionedSlate::V2(_) => SlateVersion::V2,
VersionedSlate::V1(_) => SlateVersion::V1,
_ => SlateVersion::V0,

This comment has been minimized.

Copy link
@bddap

bddap Apr 18, 2019

Contributor

Why match on a wildcard here? Matching on VersionedSlate::V0(_) instead would serve as a reminder to update the function when V3 comes around.

This comment has been minimized.

Copy link
@yeastplume

yeastplume Apr 19, 2019

Author Member

Good point, changed

V2(SlateV2),
/// V1 Grin 1.0.1 - 1.0.3)
V1(SlateV1),
/// V2 Grin 1.0.0

This comment has been minimized.

Copy link
@bddap

bddap Apr 18, 2019

Contributor

V2?

This comment has been minimized.

Copy link
@yeastplume

yeastplume Apr 19, 2019

Author Member

👍


//use grin_wallet_libwallet::slate_versions::v1::SlateV1;
//use grin_wallet_libwallet::slate_versions::v2::SlateV2;

This comment has been minimized.

Copy link
@bddap

bddap Apr 18, 2019

Contributor

I suggest running similar tests for every slate version.

Most importantly, test that always request_slate.version() == response_slate.version().

Something like this would work:

// easy-jsonrpc generates typesafe client stubs.
// try adding `pub use crate::foreign_rpc::foreign_rpc as foreign_rpc_client;`
// to `api/src/lib.rs` then running `cargo doc --open`
use grin_wallet_api::foreign_rpc_client;

/// call ForeignRpc::receive_tx on vs and return the result
fn receive_tx(vs: VersionedSlate) -> VersionedSlate {
	let dir = tempdir().map_err(|e| format!("{:#?}", e)).unwrap();
	let dir = dir
		.path()
		.to_str()
		.ok_or("Failed to convert tmpdir path to string.".to_owned())
		.unwrap();
	let bound_method = foreign_rpc_client::receive_tx(
		vs,
		None,
		Some("Thanks for saving my dog from that tree, bddap.".into()),
	)
	.unwrap();
	let (call, tracker) = bound_method.call();
	let json_response = run_doctest_foreign(call.as_request(), dir, 5, false)
		.unwrap()
		.unwrap();
	let mut response = easy_jsonrpc::Response::from_json_response(json_response).unwrap();
	tracker.get_return(&mut response).unwrap().unwrap()
}

#[test]
fn version_unchanged() {
	let req: Value = serde_json::from_str(include_str!("slates/v1_req.slate")).unwrap();
	let slate: VersionedSlate = serde_json::from_value(req["params"][0].clone()).unwrap();
	let slate_req: Slate = slate.into();

	assert_eq!(
		receive_tx(VersionedSlate::into_version(
			slate_req.clone(),
			SlateVersion::V0
		))
		.version(),
		SlateVersion::V0
	);

	assert_eq!(
		receive_tx(VersionedSlate::into_version(
			slate_req.clone(),
			SlateVersion::V1
		))
		.version(),
		SlateVersion::V1
	);

	assert_eq!(
		receive_tx(VersionedSlate::into_version(
			slate_req.clone(),
			SlateVersion::V2
		))
		.version(),
		SlateVersion::V2
	);

	// compile time test will remind us to update these tests when updating slate format
	fn _all_versions_tested(vs: VersionedSlate) {
		match vs {
			VersionedSlate::V0(_) => (),
			VersionedSlate::V1(_) => (),
			VersionedSlate::V2(_) => (),
		}
	}
}

This comment has been minimized.

Copy link
@yeastplume

yeastplume Apr 19, 2019

Author Member

Thanks for putting this together, I've included this in the last push

@yeastplume

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2019

We're eventually going to drop support for earlier slate versions, and want to be able to make breaking but managed changes to the slate format, so while converting everything to v0 is an option now, it won't be in future. There will also be more consumers of the slate than just us, and V0 is very annoying to work with due to the way fields were serialized as byte arrays.

@yeastplume yeastplume merged commit 47ee03c into mimblewimble:master Apr 23, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

garyyu added a commit to gottstech/grin-wallet that referenced this pull request Apr 26, 2019

merge from upstream (#5)
* V2 slate versioning enablement (mimblewimble#85)

* fix for command line listener port override

* reduce parameter query size

* Add slate versioning

* rustfmt

* bump version number

* Add tests for slate version conversion

* rustfmt

* Updates and test addition based on bdap's review

* rustfmt

* fix mimblewimble#88 (mimblewimble#89)

* Add `check_version` function to Foreign API (mimblewimble#87)

* move api deser types into separate types mod

* rustfmt

* missing types file

* make all exports from libwallet more explicit

* rustfmt

* add version check function to foreign api

* rustfmt

* change check_version return value to result, for consistency

* fix the merge missed parts

* rustfmt

@yeastplume yeastplume deleted the yeastplume:v2_slate_versioning branch May 20, 2019

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