-
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
ION SDK #307
ION SDK #307
Conversation
Codecov Report
@@ Coverage Diff @@
## main #307 +/- ##
==========================================
+ Coverage 58.68% 58.91% +0.23%
==========================================
Files 42 45 +3
Lines 4717 5125 +408
==========================================
+ Hits 2768 3019 +251
- Misses 1462 1568 +106
- Partials 487 538 +51
|
depends on #308 |
57bd763
to
7a3de7d
Compare
7a3de7d
to
6daff52
Compare
* main: Bump github.com/goccy/go-json from 0.10.0 to 0.10.2 (#310) Bump golang.org/x/term from 0.5.0 to 0.6.0 (#299) Update JWX lib to use v2 (#308) # Conflicts: # credential/signing/jws.go # credential/signing/jwt.go # crypto/jwk_test.go # crypto/jwt.go # go.mod # go.sum # wasm/static/main.wasm
privateKey *btcec.PrivateKey | ||
} | ||
|
||
// NewBTCSignerVerifier creates a new signer/verifier for signatures suited for the BTC blockchain |
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.
Is a signer/verifier meant to implement a specific interface? There exists cryptosuite.Signer
, but it's unclear to me what they should represent.
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.
not a specific interface, just a utility to wrap the key
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.
There are a lot of methods above that aren't tested. What's the rationale for not doing so?
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.
they are all tested through the test vectors, though not directly
did/ion/request.go
Outdated
publicKeys := make(map[string]PublicKey) | ||
for _, publicKey := range s.PublicKeysToAdd { | ||
if _, ok := publicKeys[publicKey.ID]; ok { | ||
return fmt.Errorf("public key<%s> is duplicated", publicKey.ID) |
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.
Here and elsewhere. If you want stack traces, use errors.Errorf
. See https://stackoverflow.com/questions/61933650/whats-the-difference-between-errors-wrapf-errors-errorf-and-fmt-errorf for more details. (make sure it's not the native "errors"
library).
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 know this, will update
did/ion/request.go
Outdated
toBeSigned := struct { | ||
UpdateKey crypto.PublicKeyJWK `json:"updateKey"` | ||
DeltaHash string `json:"deltaHash"` | ||
}{ |
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.
Curious why an anonymous struct here vs defining it in model.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.
ah was all anonymous first, forgot to update this
did/ion/request.go
Outdated
// NewUpdateRequest creates a new update request https://identity.foundation/sidetree/spec/#update | ||
func NewUpdateRequest(didSuffix string, updateKey, nextUpdateKey crypto.PublicKeyJWK, signer BTCSignerVerifier, stateChange StateChange) (*UpdateRequest, error) { | ||
if err := stateChange.IsValid(); err != nil { | ||
return nil, err |
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.
Here and elsewhere in this func. I recommend chaining errors with either Wrap
or using :
and Errorf
. See https://github.com/uber-go/guide/blob/master/style.md#error-wrapping for more details.
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.
updated
Co-authored-by: Andres Uribe <auribe@tbd.email>
Co-authored-by: Andres Uribe <auribe@tbd.email>
Co-authored-by: Andres Uribe <auribe@tbd.email>
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.
Not blocking, but part of an important convo.
did/ion/model.go
Outdated
} | ||
|
||
type Delta struct { | ||
Patches []any `json:"patches,omitempty"` |
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 the classic oneof problem with golang, which has no great support :(
We could use the protobuf approach. It's a bit of additional work, but I think it's worth it. What are your thoughts?
@andresuribe87 I agree protobufs would work. I think JSON schemas would be a simpler solution. since the source of truth is going to be an ion node (which will reject an invalid request) I think we are OK for now, but should think about it as an improvement |
* origin/main: ION SDK (#307) # Conflicts: # wasm/static/main.wasm
Add feature parity to the following repos: