Slate binary/base64 encoding #112

mcdallas · 2019-05-19T20:36:21Z

this PR:

Implements the Readable/Writeable traits from grin_core for the Slate struct
Adds a to_bytes() helper method in Slate that serializes to bytes all the fields and prepends variable sized fields with their length
Adds a from_bytes() method that does the opposite
Adds a new StdioWalletCommAdapter that reads/writes base64 slates to stdi/o
Adds a new string method to the invoice, pay, send, receive and finalize commands
and makes it the default for invoice and pay

You can create an invoice with grin-wallet invoice <amount>
you can process an invoice with grin-wallet pay -i <b64string>
you can finalize with grin-wallet finalize -m string -i <b64string>

any of this commands with -m string and without -i will read from stdin

libwallet/src/slate_versions/mod.rs

yeastplume · 2019-05-20T10:34:21Z

libwallet/src/slate_versions/mod.rs

+		};
+
+		Ok(VersionedSlate::V2(slate))
+	}
 }



Sorry, I know you asked but I think this should go into the Slate, not the versioned file. Would the assumption be that you would only really want to serialize the latest slate to binary?

Why would we only want to serialize the latest? If we choose that route, we have to always be backward-compatible with future changes.

Right, in the current setup, it's expected that each new version of the slate will require implementing conversions between previous version->current version and back. So this would require a second implementation to support the binary conversion as well. Perhaps we just want to consider swapping to using one or the other, but not getting into a position where we need to support both.

DavidBurkett · 2019-05-20T10:34:44Z

controller/src/command.rs

@@ -346,7 +348,11 @@ pub fn receive(
 	g_args: &GlobalArgs,
 	args: ReceiveArgs,
 ) -> Result<(), Error> {
-	let adapter = FileWalletCommAdapter::new();
+	let adapter = match args.method.as_str() {
+		"file" => FileWalletCommAdapter::new(),


Our plan is to deprecate "file" then, right? I can't think of any reason we would want file support when we have something as simple as a string.

Not sure I understand, why would we want to deprecate the ability to save a slate as a file?

This fully replaces every use case for file send/receive. One of our many usability issues right now is the fact that there are so many different ways to send/receive grin. Certain exchanges only support certain methods, and the same goes for wallets. It may sound counter-intuitive, but the way we make grin more usable is to support fewer transfer methods, not more.

Still not with you here. Are you suggesting that anyone using the command line wallet should have to cut and paste strings into the command line instead of being allowed to specify a file name, and everyone creating a new transaction for email send (for instance) would either have to cut/paste or pipe output into a file?

Sure, seems easy enough. Regardless, I'm not as worried about the 'file' method as having a separate serialization format for files. If we're going to serialize slates to base64 for files too, then I'm ok with keeping it.

we can keep the file as is for now and have it output a hex or b64 file after the hard fork

DavidBurkett · 2019-05-20T10:37:14Z

So now we have to worry about binary serialization and json serialization with each new slate version. Can we stop supporting json with future versions, and just send base64 via HTTP as well?

yeastplume · 2019-05-20T10:39:08Z

Couple of comments inline, and a few more observations:

I like the addition of a stdio adapter, cleaner than the current output of printlns in random parts of the code.
Why would we want to limit base64 output to just invoicing? If it's included here users will expect it to be possible from the regular send command as well.

yeastplume · 2019-05-20T10:46:08Z

Also, not quite convinced that outputting the base64 to the terminal should be the default, it seems to be more useful in the case of interacting with a webpage as opposed to the existing CLI wallet to CLI wallet transfer

mcdallas · 2019-05-20T10:58:00Z

@DavidBurkett I know you don't like this because it adds maintenance burden, but how do you do QR codes without it ?

DavidBurkett · 2019-05-20T11:02:00Z

@DavidBurkett I know you don't like this because it adds maintenance burden, but how do you do QR codes without it ?

@mcdallas What is it you think I don't like? I love the idea of base64 encoding binary slates. It's something I had thought about before, and I was your biggest proponent when you brought it up after the last dev meeting. What I don't like is continuing to support json slates when this is clearly better.

yeastplume · 2019-05-20T11:02:10Z

Okay, after the discussion above and a bit more thinking, I'm very not sure this is something we just want to smash into 1.1.0 a couple of days before we release it. The major concerns here are that we're creating 2 separate slate formats here that have to be maintained through all future versions, and applying them a bit inconsistently.

I'm not against changing the slate format to be a binary-only one more conducive to compaction, as there's nothing special about the choice of json originally other than it was easiest during early development. But I think the changeover should be planned out beforehand, the binary slate format generally agreed upon, and then on release of a particular version, all slates will be in the binary format (or base64 representation thereof). This way, we'll only need to implement the json->binary conversion once in the upgrade/downgrade chain, and eventually be able to drop support for the json altogether at the next hard fork.

mcdallas · 2019-05-20T11:03:39Z

Why would we want to limit base64 output to just invoicing? If it's included here users will expect it to be possible from the regular send command as well.

This PR adds --method string to all the wallet commands

mcdallas · 2019-05-20T11:05:51Z

Okay, after the discussion above and a bit more thinking, I'm very not sure this is something we just want to smash into 1.1.0 a couple of days before we release it. The major concerns here are that we're creating 2 separate slate formats here that have to be maintained through all future versions, and applying them a bit inconsistently.

@yeastplume the reason that I want this to 1.1.0 is if we release invoicing without strings, all the exchanges/pools are going to implement it over http and when 1.1.1 is released they are never going to change it. Imo it is worth it postponing 1.1.0

yeastplume · 2019-05-20T11:18:33Z

Right, problem here is that 1.1.0 has already been dragging on quite a bit, we can't release grin server 1.1.0 without releasing grin-wallet as well, and we very much need to get it out as we only really have a couple of weeks now to get changes in for the 2.0.0 fork version. For the record, I'm perfectly fine with targeting this functionality for 2.0.0 by treating the binary-serialized slate as Slate V3 implementing the conversion between V2 and V3 in the conversion pipeline, and ensuring all slate input/output is consistent.

Another complication that's just come to mind here here is that the V2 wallet API uses json slates as arguments (and are very easy for API consumers to read,) so I don't think that dropping all json support for slates is viable... again it all needs to be thought through.

mcdallas · 2019-05-24T17:11:58Z

moved ser/deset to a trait and rebased on latest

yeastplume · 2019-05-27T09:41:23Z

@mcdallas Thanks again for continuing to work on this, it looks in a good state now, and thanks for changing the serialization to be consistent with the rest of grin. Only think I'd say now is the binary serialization could probably use a couple of small unit tests, particularly to inform future developers that they need to be changed when the slate format changes.

After thinking a bit more and considering this looks more or less complete I could probably be convinced to merge this in before 1.1.0, so long it comes with the caveat that we're not going to do another beta for it (any issues will have to be fixed in the point release). I'll just summarize current apprehensions:

I'm not entirely sure about making the "string" reader/writer the default, but I can trust your judgement if you believe this is going to be the most convenient mode of operation for the majority of users.
Still don't think it'll be viable long term (or at least, it will be very unpleasant) to have to maintain both the JSON and binary encodings and particularly upgrade/downgrade paths between them, but don't think we can ditch JSON either given the existence of the wallet APIs (and not to mention json makes slates far easier to work with for development). But given this is currently implemented as a feature add that just enhances invoicing workflork as opposed to fundamentally changes how slates are transferred, we can probably think about this later.

Does this thinking make sense? Would anyone else have any major concerns here before we merge this?

mcdallas · 2019-05-27T23:07:00Z

I think we can avoid adding versioning to the binary encoding and be backwards compatible by appending any new field at the end of the slate.

Also for anyone reviewing this, it would be nice to review the serialization format itself and if anything needs to change, because changing anything after it goes live (except adding new fields) will be backwards incompatible

DavidBurkett · 2019-05-27T23:18:15Z

I strongly prefer a version byte.

yeastplume · 2019-07-02T08:56:04Z

Apologies this got closed accidentally due to branch changes (I missed this one,) conversation is still open, would appreciate if you could reopen. But I think this will need an RFC under the upcoming structure changes

yeastplume reviewed May 20, 2019

View reviewed changes

libwallet/src/slate_versions/mod.rs Outdated Show resolved Hide resolved

yeastplume reviewed May 20, 2019

View reviewed changes

DavidBurkett reviewed May 20, 2019

View reviewed changes

mcdallas force-pushed the slate-encoding branch from 50f7e72 to a4a4f16 Compare May 24, 2019 17:07

mcdallas mentioned this pull request May 25, 2019

impl Readable/Writable for PublicKey mimblewimble/grin#2841

Merged

yeastplume added this to the v2.x.x milestone Jun 6, 2019

mcdallas added 11 commits June 12, 2019 16:09

Add encode() method to VersionedSlate

1d360e8

rustfmt

5d5befb

add StdioWalletCommAdapter

2a8882d

rustfmt

3bedd12

add Slate.from_bytes()

ea77598

rustfmt

3264ccc

add "string" method to grin-wallet finalize

92fafa3

add 'string' method to 'send' and 'receive'

2786fb8

rustfmt

a73549b

Move encoding to a trait of Slate

b6560ed

rustfmt

2a78450

mcdallas added 5 commits June 12, 2019 16:19

fix tests

4e5d6f9

rebase on milestone/2.0.0 and fix conflicts

46ec60f

use PublicKey::read/write

f47e6ee

use compact encoding for signatures instead of raw bytes

c81baf1

rustfmt

92c361f

mcdallas force-pushed the slate-encoding branch from 8f02e9c to 92c361f Compare June 13, 2019 17:18

mcdallas changed the base branch from master to milestone/2.0.0 June 13, 2019 17:18

yeastplume closed this Jul 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Slate binary/base64 encoding #112

Slate binary/base64 encoding #112

mcdallas commented May 19, 2019 •

edited

Loading

yeastplume May 20, 2019

DavidBurkett May 20, 2019

yeastplume May 20, 2019

DavidBurkett May 20, 2019

yeastplume May 20, 2019

DavidBurkett May 20, 2019

yeastplume May 20, 2019

DavidBurkett May 20, 2019

mcdallas May 20, 2019

DavidBurkett commented May 20, 2019

yeastplume commented May 20, 2019

yeastplume commented May 20, 2019

mcdallas commented May 20, 2019

DavidBurkett commented May 20, 2019

yeastplume commented May 20, 2019

mcdallas commented May 20, 2019

mcdallas commented May 20, 2019

yeastplume commented May 20, 2019

mcdallas commented May 24, 2019

yeastplume commented May 27, 2019

mcdallas commented May 27, 2019 •

edited

Loading

DavidBurkett commented May 27, 2019

yeastplume commented Jul 2, 2019 •

edited

Loading

Slate binary/base64 encoding #112

Slate binary/base64 encoding #112

Conversation

mcdallas commented May 19, 2019 • 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

DavidBurkett commented May 20, 2019

yeastplume commented May 20, 2019

yeastplume commented May 20, 2019

mcdallas commented May 20, 2019

DavidBurkett commented May 20, 2019

yeastplume commented May 20, 2019

mcdallas commented May 20, 2019

mcdallas commented May 20, 2019

yeastplume commented May 20, 2019

mcdallas commented May 24, 2019

yeastplume commented May 27, 2019

mcdallas commented May 27, 2019 • edited Loading

DavidBurkett commented May 27, 2019

yeastplume commented Jul 2, 2019 • edited Loading

mcdallas commented May 19, 2019 •

edited

Loading

mcdallas commented May 27, 2019 •

edited

Loading

yeastplume commented Jul 2, 2019 •

edited

Loading