Skip to content

Encoding library and standardizing usage#48

Merged
austinabell merged 15 commits into
masterfrom
austin/encoding
Dec 3, 2019
Merged

Encoding library and standardizing usage#48
austinabell merged 15 commits into
masterfrom
austin/encoding

Conversation

@austinabell

Copy link
Copy Markdown
Contributor
  • Creates encoding library which includes the traits to be able to encode and decode these objects without knowledge of their structure.
    • Will expand upon to try to generalize cbor and json encoding so that there doesn't have to be a specific implementation, but I assume with how things are encoded right now from go which isn't defined in any spec, these custom implementations will have to be relied upon
  • Creates a generic blake hashing function to be able to hash any data, will probably be used in all systems to including now to remove the need to duplicate logic

If this doesn't get reviewed by the time I implement the generic cbor encoding and utility functions, I will include in this PR. Opening to allow this to be used as soon as necessary and not have to rewrite.

@ec2

ec2 commented Nov 27, 2019

Copy link
Copy Markdown
Contributor

I'd say hold off on this PR until we get CBOR in. Based on how serde looks like its used, we the Cbor traits that your implementing. You simply need to derive Serialize and Deserialize and you get Cbor for free.

@austinabell

Copy link
Copy Markdown
Contributor Author

I'd say hold off on this PR until we get CBOR in. Based on how serde looks like its used, we the Cbor traits that your implementing. You simply need to derive Serialize and Deserialize and you get Cbor for free.

The reason I state that these specific implementations will be needed is because the data structures need to have very specific cbor encodings and not just one that is generic. I agree that this should probably be finalized since it isn't clear the cbor encoded formats and how the library will be used

@ansermino

Copy link
Copy Markdown
Contributor

It became clear today that serde will not be the soloution. Since we need to match the IPLD data model, it seems sensible to implement traits to be able to explicitly handle encoding.

@austinabell

Copy link
Copy Markdown
Contributor Author

Cleaned up usage and only included things I assume will be needed. Trait name or function signature may change, but this is easy to change in the future. The other components will be reusable and blake2 hashing will be needed outside of the context of multihash.

PR is ready to review

Comment thread encoding/src/errors.rs Outdated
Comment thread encoding/Cargo.toml Outdated
Comment thread encoding/src/hash.rs
Comment thread vm/address/src/lib.rs
@@ -235,21 +231,5 @@ pub fn validate_checksum(ingest: Vec<u8>, expect: Vec<u8>) -> bool {

/// Returns an address hash for given data
fn address_hash(ingest: Vec<u8>) -> Vec<u8> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is this actually used for?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

to create the 20 byte hash used as the address for secp256k1 and actor protocols

@austinabell austinabell requested a review from ansermino December 2, 2019 17:41
@austinabell

austinabell commented Dec 2, 2019

Copy link
Copy Markdown
Contributor Author

@ec2 @GregTheGreek @ansermino bork

usage example with address if it makes this easier to review: austin/encoding...austin/addresscbor

@ec2 ec2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@austinabell austinabell merged commit f777233 into master Dec 3, 2019
@austinabell austinabell deleted the austin/encoding branch December 3, 2019 17:03
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.

5 participants