Feat/multidid by zachferland · Pull Request #1 · 3box/rs-did

zachferland · 2023-04-24T16:54:10Z

First go at writing anything in rust, timeboxed so a few todos, and need think thru a few parts, relied a lot on compiler feedback to just get running.

Based on js implementation here - https://github.com/ceramicnetwork/js-did/tree/main/packages/multidid

oed · 2023-04-24T17:53:34Z

Why would this not go into rust-ceramic? Seems unnecessary to have multiple repos for now?

zachferland · 2023-04-24T18:20:25Z

yeah know we mentioned that before, just wasnt a lot of crates in there, so didnt seem to make a lot of sense, and doesnt share a lots of dependencies/build, but will be easy to move if needed

oed · 2023-04-24T18:21:57Z

What's the value of keeping it separate? Seems like it will be harder to manage? We will need to use this in Ceramic regardless

Thoughts @nathanielc ?

dbcfd · 2023-04-24T18:32:41Z

For libraries, you usually omit the lock files.

dbcfd · 2023-04-24T18:33:19Z

If there's not multiple crates in a repo, you don't need to use workspace, and instead just a top level Cargo.toml with files under source.

dbcfd · 2023-04-24T18:35:08Z

+/**
+ * Multicodec Codes https://github.com/multiformats/multicodec/blob/master/table.csv
+ */
+ const MULTIDID_CODEC: u32 = 0xd1d; // did


For a group of codecs, rust approach is usually to put them in an enum. That way they can be strongly typed, and easily shared by making the enum public https://github.com/3box/rust-ceramic/pull/3/files#diff-ecd9c86aee006d0568451cc27f6aba8ae777dee68cc03920d480ac80e04e5ac2R14

For example see how mulitbase does it here https://docs.rs/multibase/latest/multibase/enum.Base.html

Notice the from_code and code methods that allow conversion to an integer code.

A peak behind the scenes show they use a macro to implement the methods on each variant of the enum https://docs.rs/multibase/latest/src/multibase/base.rs.html While its a bit of an advanced technique its a good practice in this case and reads well.

tried doing this as enum now, will probably try to do as macro after, but havent gotten there yet, very verbose this way but nice to be properly typed

the did key codecs are both top level method (did:?) determinants, and method specific (did:key:?) determinants, making cross boundaries a bit, all other methods wont do this, didnt know if there was better way represent as enum here where did:key codecs need to be represented at top level and as subset

If for some reason enums don't work, you can also create a stronger type.

pub type Codec = u32

then

const MULTIDID_CODEC: Codec = 0xd1d;

That way you can scope to only codec types, and not any u32.

Keep trying with enums though. If you still can't get it, let me know.

dbcfd · 2023-04-24T18:35:49Z

+
+impl Multidid {
+
+    pub fn new(code: u32, id: Vec<u8>, url: Vec<u8>) -> Result<Self, &'static str> {


Rather than static str, you can use thiserror or anyhow.

Danny and I are on the same page :)

Also in this case creating a Multidid cannot error so you can simply return Self instead of wrapping it in a Result type.

remove a few instances where result not needed

used anyhow for now, will probably type errors in the future

dbcfd · 2023-04-24T18:37:07Z

+        let method_id_offset = method_code_offset + &self.method_code.required_space();
+
+        let method_id_len;
+        if &self.method_code == &ANY_METHOD_CODE {


Rather than unbound variables, you can use assigment from the block

let method_id_len = if &self.method_code = &ANY_METHOD_CODE { 0 } else { ... }

This also works with early returns from different cases.

dbcfd · 2023-04-24T18:38:01Z

+        if &method_code == &ANY_METHOD_CODE {
+            method_id_len = 0;
+        } else if &method_code == &PKH_METHOD_CODE {
+            return Err("Not implemented");


Make this more verbose by saying "PKH Method is not implemented"

dbcfd · 2023-04-24T18:39:11Z

+            return Err("No matching did method code found")
+        }
+
+        let mut method_id = vec![0;method_id_len as usize];


I think these can be simplifed with

let method_id = &bytes[method_id_offsert..url_len_offset].to_vec()

dbcfd · 2023-04-24T18:39:52Z

+    } 
+
+    // return bs58btc_str only
+    pub fn to_multibase(&self) -> Result<String, &'static str> {


How about a generic to_multibase that takes in a base as an argument?

dbcfd · 2023-04-24T18:40:26Z

+        return Multidid::from_bytes(bytes);
+    }
+
+    pub fn from_string(did: &str) -> Result<Multidid, &'static str> {


Instead of having this as a a function, implement the trait FromStr.

nathanielc · 2023-04-24T18:41:50Z

It could go either way. Placing it in the rust-ceramic repo would be easiest for now as it means we can leverage the same CI jobs etc and not have to manage as many repos. However its also not too much effort to copy/paste the CI jobs from rust-ceramic or rust-dag-jose repos

nathanielc

Overall looks great. Generally improving the error handling with anyhow or thiserror would be good.

Generally speaking I prefer thiserror for libraries as it allows for clear communication about the ways in which the library can error. However as a first pass using anyhow error is good enough if there are not clear use cases for differentiating different kinds of errors.

nathanielc · 2023-04-24T18:42:40Z

Same here, do not commit the Cargo.lock file for libraries.

nathanielc · 2023-04-24T18:46:16Z

+/**
+ * Multicodec Codes https://github.com/multiformats/multicodec/blob/master/table.csv
+ */
+ const MULTIDID_CODEC: u32 = 0xd1d; // did


For example see how mulitbase does it here https://docs.rs/multibase/latest/multibase/enum.Base.html

Notice the from_code and code methods that allow conversion to an integer code.

A peak behind the scenes show they use a macro to implement the methods on each variant of the enum https://docs.rs/multibase/latest/src/multibase/base.rs.html While its a bit of an advanced technique its a good practice in this case and reads well.

nathanielc · 2023-04-24T18:48:44Z

+ const RSA_CODE: u32 = 0x1205; // rsa-pub
+ const KEY_CODES: [u32; 8] = [SECP256K1_CODE, BLS12_381_G2_CODE, X25519_CODE, ED25519_CODE, P256_CODE, P384_CODE, P521_CODE, RSA_CODE];
+
+fn key_method_code_len(code: &u32, key_prefix: &[u8]) ->  Result<u16, &'static str> {


I would look into the anyhow crate that defines a very generic error type. It makes it easy to return errors from functions without having to use static strings as error messages or build your own type.

As an aside if you do need to build your own error type I typically use thiserror to do so.

nathanielc · 2023-04-24T18:50:19Z

+
+fn key_method_code_len(code: &u32, key_prefix: &[u8]) ->  Result<u16, &'static str> {
+    if code == &RSA_CODE {
+        return Ok(*rsa_code_len(key_prefix).unwrap())


If you change the return of this function to use the anyhow::Result error type you can then use the ? syntax to check for an error and return it instead of using .unwrap().

This code would then become:

return Ok(*rsa_code_len(key_prefix)?)

The difference is now an error is returned from the function instead of panicing the current Rust thread.

nathanielc · 2023-04-24T18:50:59Z

+    ]);
+
+    let val = *km_codes_len.get(code).ok_or("Key not supported").unwrap();
+    return Ok(val)


nit, the last expression of a function is the return value so this line can be simply Ok(val), no need for the return keyword.

nathanielc · 2023-04-24T18:52:22Z

+
+impl Multidid {
+
+    pub fn new(code: u32, id: Vec<u8>, url: Vec<u8>) -> Result<Self, &'static str> {


Danny and I are on the same page :)

Also in this case creating a Multidid cannot error so you can simply return Self instead of wrapping it in a Result type.

nathanielc · 2023-04-24T18:54:52Z

+        let mdid = Multidid::from_string(did_str).unwrap();
+        assert_eq!(mdid.to_string().unwrap(), did_str);
+    }
+}


These tests are great!

oed · 2023-04-24T19:10:48Z

Also fwiw, I think it's unlikely that we will implement all the functionality that we have in js-did because spruce has a bunch of rust DID stuff already.

zachferland · 2023-05-01T20:14:35Z

thanks @dbcfd and @nathanielc for reviewing! going to go through these shortly, and covered some during rust hours

zachferland · 2023-05-02T21:47:36Z

Covered almost all comments, all made sense, some parts I will revisit in future when time and/or get more familiar with rust.

Aim to merge/close for now, unless any standout issues

zachferland added 2 commits April 24, 2023 12:48

cargo workspace

9927ca1

feat: init multidid lib

fd145e0

zachferland requested review from AaronGoldman and nathanielc April 24, 2023 16:57

dbcfd suggested changes Apr 24, 2023

View reviewed changes

nathanielc suggested changes Apr 24, 2023

View reviewed changes

zachferland added 5 commits May 2, 2023 11:36

ref: remove lock files for lib

fa66924

ref: error descrip, vec, multibase arg

30197cc

ref: use anyhow

a95b936

ref: try enum, need less verbose way

db14b88

ref: if assign, format

a317022

zachferland requested review from dbcfd and nathanielc May 2, 2023 21:47


		impl Multidid {

		pub fn new(code: u32, id: Vec<u8>, url: Vec<u8>) -> Result<Self, &'static str> {

Conversation

zachferland commented Apr 24, 2023

Uh oh!

oed commented Apr 24, 2023

Uh oh!

zachferland commented Apr 24, 2023

Uh oh!

oed commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanielc commented Apr 24, 2023

Uh oh!

nathanielc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oed commented Apr 24, 2023

Uh oh!

zachferland commented May 1, 2023

Uh oh!

zachferland commented May 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

oed commented Apr 24, 2023 •

edited

Loading