-
Notifications
You must be signed in to change notification settings - Fork 54
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
Go Mobile support for DID Key #457
Conversation
Codecov Report
@@ Coverage Diff @@
## main #457 +/- ##
==========================================
- Coverage 57.42% 57.23% -0.19%
==========================================
Files 67 67
Lines 7499 7494 -5
==========================================
- Hits 4306 4289 -17
- Misses 2456 2468 +12
Partials 737 737
|
CodeQL will be broken until the authors update it to support go1.21, should not block merge |
crypto/keys.go
Outdated
ECDSAMarshalCompressed = Option{Name: "ecdsa-compressed", Value: true} | ||
ECDSAUnmarshalCompressed = Option{Name: "ecdsa-compressed", Value: true} |
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.
The value of both options is the same. Is that intentional?
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.
yes, one used for marshaling one for unmarshaling
I could condense to a ECDSACompressed
option
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 generally discourage using an Option
type which has a field named Name
. It's bypasses the type system, and makes it easy to shoot yourself in the foot.
I would suggest using an enum es below:
const (
ECDSAMarshalCompressed int = iota
ECDSAUnmarshalCompressed
)
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.
done
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.
The methods in here aren't tested. I think we should have some sample code that exercises it. It can act as documentation as well on how it should be called.
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.
agree will add tests
|
||
// IOS Generates the iOS packages | ||
// Note: this command also installs "gomobile" if not present | ||
func IOS() error { |
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.
Should this target, as well as the Android target, be documented in a README?
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.
yes good call
tests added, comments made, bugs fixed. should be good to go |
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.
Non-blocking comments.
|
||
## Android | ||
|
||
``` |
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 think it's important to cover:
- Documentation on how to include this in an android/ios project, with a simple example.
- CI that exercises actual Android/ios code so we can be confident this works.
- Guidance to readers that tells them how to discover available functionality within the SDK.
Of course, that's a lot of scope for this PR, so tracking in separate issues is cool. But I think this is critically important.
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.
will do a mix of now and new issues
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.
mobile/did_key.go
Outdated
type GenerateDIDKeyResult struct { | ||
DID string `json:"did"` | ||
JWK map[string]any `json:"jwk"` | ||
} |
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.
godocs for all fields here. This should be treated this as a public API
} | ||
|
||
// GenerateDIDKey generates a new DID key and returns a JSON representation of GenerateDIDKeyResult | ||
func GenerateDIDKey(kt string) ([]byte, error) { |
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.
Can you include the possible kt values in the godoc?
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'll add that the values are the result of the GetSupportedDIDKeyTypes
mobile/did_key.go
Outdated
type CreateDIDKeyRequest struct { | ||
KeyType string `json:"keyType"` | ||
PublicKeyJWK map[string]any `json:"publicKeyJwk"` | ||
} |
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.
godocs for all types here.
mobile/did_key.go
Outdated
type CreateDIDKeyResult struct { | ||
DID string `json:"did"` | ||
} |
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.
godocs, hopefully with an example. Something like
// DID that was created. E.g. `did:key:zHjkdjd...`
} | ||
|
||
type Document struct { | ||
DIDDocument map[string]any `json:"didDocument"` |
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.
You know what I want :)
|
||
// CreateDIDKey creates a new DID key from an existing public key, accepting a JSON representation of CreateDIDKeyRequest | ||
// and returns a JSON representation of CreateDIDKeyResult which contains the DID of the newly created key as a string | ||
func CreateDIDKey(requestBytes []byte) ([]byte, error) { |
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.
Let's make it easy for clients to understand what they should put into requestBytes
: can we type this?
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 mention CreateDIDKeyRequest
here - is there a way I can make it clearer?
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.
It's tough to say, because I don't know how a mobile dev would interact with this. I'm of the opinion that a worthy goal is to have mobile devs not have to read any go code at all.
So I ask myself, as a mobile dev, do I know what I should put in here? I think the current answer is probably not. I would make a suggestion if I understood better how calling from Java/Kotlin, Swift would look like.
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 think go docs are the best we can realistically do...if they don't read the API docs they'll always be lost
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.
How about having the type of the parameter this function accepts be CreateDIDKeyRequest
? According to the documentation, structs are allowed.
It feels like it's a bit easier to deal with a typed object in Kotlin / Swift.
I was hoping that documentation gets automatically translated into the artifact that's created (the AAR or the Obj-C lib), but it unfortunately doesn't look like that's the case. That's unfortunate :(
To mitigate, I would say adding an example in the target language that demonstrates how to call this function would be great.
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.
will take this in a follow up - but after seeing what the feedback is from mobile devs
No description provided.