-
Notifications
You must be signed in to change notification settings - Fork 17
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 bep44 encoded for did:dht #209
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
crates/dids/src/method/dht/bep44.rs
Outdated
use keys::key::{KeyError, PublicKey}; | ||
|
||
const MIN_MESSAGE_LEN: usize = 72; | ||
const MAX_MESSAGE_LEN: usize = 1072; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to split this into something like "max V length" since the sign and seq are separate and we need to enforce that v is no longer than 1000b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a const MAX_V_LEN
and some documentation to keep it more clear. We need to do the check on MIN_MESSAGE_LEN
at the beginner of decode()
regardless so we don't try to access an index out of bounds.
crates/dids/src/method/dht/bep44.rs
Outdated
let seq: u64 = SystemTime::now() | ||
.duration_since(UNIX_EPOCH)? | ||
.as_micros() | ||
.try_into() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what this does but the spec has the seq in seconds https://did-dht.com/#create
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's a mistake because copied it blindly from web5-dart. Here's a PR to update it in dart decentralized-identity/web5-dart#87
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good to go once that comment on seconds is addressed!
and yeah I guess also maybe we should enforce the 1000 byte limitation in the new()
function
This PR does not yet make use of the bep44 mod. In a follow up PR that implements
DidDht::create
, this encoder will actually be used.See also the dart implementation: https://github.com/TBD54566975/web5-dart/blob/main/packages/web5/lib/src/dids/did_dht/bep44.dart