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

Add message compression and versioning #466

Merged
merged 40 commits into from
Nov 12, 2021
Merged

Add message compression and versioning #466

merged 40 commits into from
Nov 12, 2021

Conversation

abdulmth
Copy link
Contributor

@abdulmth abdulmth commented Oct 30, 2021

Description of change

  • Messages are compressed with brotli before saving them to the tangle, and decompressed before getting parsed.
  • Introducing message version and encoding version.

The format of a message on the tangle is as follows:
$message_version $encoding_version $data

where

  • $message_version is a one byte flag indicating the message version (1 for the current, not-modified message).
  • $encoding_version is a one byte flag indicating the encoding version (1 for json + brotli).
  • $data is the compressed data.

$message_version can be used in the future to indicate the format that follows the first byte.

Links to any relevant issues

implements issue #434

Type of change

Add an x to the boxes that are relevant to your changes.

  • Bug fix (a non-breaking change which fixes an issue)
  • Enhancement (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Fix

How the change has been tested

Unit tests, manual tests, wasm bindings tests.

Change checklist

Add an x to the boxes that are relevant to your changes.

  • I have followed the contribution guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@abdulmth
Copy link
Contributor Author

abdulmth commented Nov 2, 2021

we might need to check if the license of Brotli is compatible with the library
https://github.com/dropbox/rust-brotli/blob/master/LICENSE

@abdulmth abdulmth marked this pull request as ready for review November 2, 2021 15:20
@olivereanderson
Copy link
Contributor

we might need to check if the license of Brotli is compatible with the library https://github.com/dropbox/rust-brotli/blob/master/LICENSE

It looks like it is licensed under the BSD 3-Clause. According to the accepted answer here: https://softwareengineering.stackexchange.com/questions/40561/is-bsd-license-compatible-with-apache
it should be compatible with the Apache 2.0 License, but we might have to double check.

@olivereanderson
Copy link
Contributor

Would it maybe make more sense to move compression_brotli.rs into the identity-core crate?

@cycraig
Copy link
Contributor

cycraig commented Nov 3, 2021

Would it maybe make more sense to move compression_brotli.rs into the identity-core crate?

It's specific to IOTA messages.

Copy link
Contributor

@cycraig cycraig left a comment

Choose a reason for hiding this comment

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

Looks good overall.

Mostly nits and questions from my side. We need to decide whether or not to support uncompressed messages at all, for instance. We might want to move around some of the files since the tangle folder is getting a bit crowded.

identity-iota/Cargo.toml Outdated Show resolved Hide resolved
identity-iota/src/tangle/compression_brotli.rs Outdated Show resolved Hide resolved
identity-iota/src/tangle/encoding.rs Outdated Show resolved Hide resolved
identity-iota/src/tangle/compression_brotli.rs Outdated Show resolved Hide resolved
identity-iota/src/tangle/compression_brotli.rs Outdated Show resolved Hide resolved
identity-iota/src/tangle/message_version.rs Outdated Show resolved Hide resolved
identity-iota/src/tangle/encoding.rs Outdated Show resolved Hide resolved
identity-iota/src/tangle/encoding.rs Outdated Show resolved Hide resolved
identity-iota/src/tangle/encoding.rs Outdated Show resolved Hide resolved
identity-iota/src/tangle/encoding.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@cycraig cycraig left a comment

Choose a reason for hiding this comment

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

Oh, forgot to add these comments: tests should be wrapped in a tests module to prevent compiling them by default.

identity-iota/src/tangle/message_version.rs Outdated Show resolved Hide resolved
identity-iota/src/tangle/encoding.rs Outdated Show resolved Hide resolved
@cycraig
Copy link
Contributor

cycraig commented Nov 3, 2021

we might need to check if the license of Brotli is compatible with the library https://github.com/dropbox/rust-brotli/blob/master/LICENSE

It looks like it is licensed under the BSD 3-Clause. According to the accepted answer here: https://softwareengineering.stackexchange.com/questions/40561/is-bsd-license-compatible-with-apache
it should be compatible with the Apache 2.0 License, but we might have to double check.

crypto.rs allows dependencies with a BSD 3-Clause licence so I think we should be good: https://github.com/iotaledger/crypto.rs/blob/fe43116710074f8e3930823bb130c736e51f32c4/.github/workflows/about.toml

@cycraig cycraig changed the title Feat/compression Add message compression and versioning Nov 3, 2021
@cycraig cycraig added the Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog label Nov 3, 2021
@abdulmth abdulmth requested a review from cycraig November 8, 2021 14:52
identity-iota/src/tangle/did_encoding.rs Outdated Show resolved Hide resolved
identity-iota/src/tangle/did_encoding.rs Outdated Show resolved Hide resolved
identity-iota/src/tangle/did_message_versioning.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@olivereanderson olivereanderson left a comment

Choose a reason for hiding this comment

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

I think it would be neater to change the module's constant to be an associated constant, apart from that this looks good to me.

@abdulmth abdulmth merged commit ad86308 into dev Nov 12, 2021
@cycraig cycraig deleted the feat/compression branch November 27, 2021 15:47
@cycraig cycraig added Rust Related to the core Rust code. Becomes part of the Rust changelog. Wasm Related to Wasm bindings. Becomes part of the Wasm changelog labels Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking change A change to the API that requires a major release. Part of "Changed" section in changelog Rust Related to the core Rust code. Becomes part of the Rust changelog. Wasm Related to Wasm bindings. Becomes part of the Wasm changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants