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

Slatepack Implementation, Pt 1 #410

Closed
wants to merge 1 commit into from

Conversation

yeastplume
Copy link
Member

Adds a first pass at the implementation of Slatepack via a new Slatepack file adapter. Currently only implements the adapter, a slatepack model, its binary serialization and tests that operate on and output slatepack.

All changes so far are isolated and not being called from anywhere in the code other than tests. Ongoing results can be viewed by commenting out the clean_output_dir statements at the bottom of controller/tests/slatepack.rs and running cargo test slatepack from the controller directory.

Note this isn't going to be a perfect implementation of slatepack just yet, the idea is to do this in a few stages while keeping the results in tests only, then integrate the workflow into the command-line once we're all happy with the implementation.

Recreated from #400

@yeastplume
Copy link
Member Author

@j01tz Also note this integrates code from your slatepack armoring repository, so if you'd like to make any changes or tweaks to it it's probably better to do it here once this PR is merged. The slatepack tests mentioned above should make it easy to tweak and check results

@lehnberg
Copy link
Collaborator

Any point in breaking out slatepack into its own library, managed in a standalone repo under /mimblewimble?

@yeastplume
Copy link
Member Author

Any point in breaking out slatepack into its own library, managed in a standalone repo under /mimblewimble?

That would be a ton of extra work and I'm not sure what benefit that would bring. Right now it's assumed slatepack will be exposed via the wallet API which I think should be fine.

Copy link
Member

@j01tz j01tz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for not getting to this sooner, very excited to see this materialize!

At first look I'm not sure we will need to bother tracking use_bin and use_armored for slatepacks beyond initial testing because every SlatepackMessage input to or output from a wallet should be armored with a binary payload according to the spec.

Once a SlatepackMessage is decoded according to the SlatepackWorkflow process I think it would become a more generic struct for further handling (which may or may not need something to track its handling during the completion of the workflow- but at this point we no longer have a SlatepackMessage but a more generic slate data that is in the process of either becoming a new SlatepackMessage or a traditional JSON slate struct).

The way I've been thinking about it, any time a 'slatepack' transaction occurs, the user inputs taken by the wallet are only either:

  • a bin/armored SlatepackMessage
  • an optional SlatepackAddress and required amount (with no choice between binary/json or armor/not)

Any output is in one of two possible forms:

  • a bin/armored SlatepackMessage
  • a traditional JSON slate object directly sent via jsonrpc to the wallet of the counterparty via Tor method
    • falls back to a bin/armored SlatepackMessage output

I'm not sure if this distinction is important or even makes sense to point out- I may just need to take a closer look at the surrounding code for better context.

I also never added support for multiple messages in the armor.rs code. Just noting that now so one of us hopefully remembers to go back to add it. Unfortunately it is probably a necessary evil if we want slatepack to be universally adopted while still covering all edge cases (maybe not all merchants/exchanges would be expected to support this functionality in their UX, but all wallets would be expected to).

I'll come back to this code and do some closer review with pt 2, thanks for putting this together so fast 👍

@yeastplume
Copy link
Member Author

Apologies for not getting to this sooner, very excited to see this materialize!

At first look I'm not sure we will need to bother tracking use_bin and use_armored for slatepacks beyond initial testing because every SlatepackMessage input to or output from a wallet should be armored with a binary payload according to the spec.

Right, those flags and the JSON serialization are all in place so that every stage of the transaction building can be output/viewed/tested (as is happening in the current integration test).

Once a SlatepackMessage is decoded according to the SlatepackWorkflow process I think it would become a more generic struct for further handling (which may or may not need something to track its handling during the completion of the workflow- but at this point we no longer have a SlatepackMessage but a more generic slate data that is in the process of either becoming a new SlatepackMessage or a traditional JSON slate struct).

Yes, internally they're deserialized into Rust structs and acted upon like any other data, same as the slate which loses all concept of version and formatting once it's read.

The way I've been thinking about it, any time a 'slatepack' transaction occurs, the user inputs taken by the wallet are only either:

  • a bin/armored SlatepackMessage
  • an optional SlatepackAddress and required amount (with no choice between binary/json or armor/not)

Any output is in one of two possible forms:

  • a bin/armored SlatepackMessage

  • a traditional JSON slate object directly sent via jsonrpc to the wallet of the counterparty via Tor method

    • falls back to a bin/armored SlatepackMessage output

I'm not sure if this distinction is important or even makes sense to point out- I may just need to take a closer look at the surrounding code for better context.

All understood and working towards the same notions, the extra code for other serialization formats are purely to assist development.

I also never added support for multiple messages in the armor.rs code. Just noting that now so one of us hopefully remembers to go back to add it. Unfortunately it is probably a necessary evil if we want slatepack to be universally adopted while still covering all edge cases (maybe not all merchants/exchanges would be expected to support this functionality in their UX, but all wallets would be expected to).

I missed this requirement, will look at it further.

I'll come back to this code and do some closer review with pt 2, thanks for putting this together so fast

@yeastplume
Copy link
Member Author

Superseded by #411

@yeastplume yeastplume closed this May 22, 2020
@yeastplume yeastplume deleted the slatepack branch July 13, 2020 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants