-
Notifications
You must be signed in to change notification settings - Fork 5
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
Feature/VerifyToken #76
base: main
Are you sure you want to change the base?
Conversation
auth.go
Outdated
) (*protocol.AuthorizationResponseMessage, error) { | ||
|
||
// VerifyToken performs verification of jws/jwz token using the registered packers in package manager | ||
func (v *Verifier) VerifyToken(token string) (*protocol.AuthorizationResponseMessage, error) { | ||
msg, _, err := v.packageManager.Unpack([]byte(token)) |
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.
we need to pass options as optional params also, so we can verify 'accept delay for proof transition'
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.
Sorry. I'm not understanding. Can you clarify? Are you proposing changing signature to this?
func (v *Verifier) VerifyToken(token string, opts ...pubsignals.VerifyOpt) (*protocol.AuthorizationResponseMessage, error)
Then.. What should we do with this opts params? The v.packageManager.Unpack([]byte(token)) doesn't accept optional parameters. Should I stop using Unpack and verify token in a more low level way?
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 v.packageManager.Unpack([]byte(token)) doesn't accept optional parameters.
This is a valid point.
We need to think about providing options to unpack (or setuping packers with this option as a workaroud).
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.
Params added.
@vmidyllic @ilya-korotya This is still a draft to discuss. I left some comments in my own code as questions. First of all is that this other PR iden3/iden3comm#58 must be accepted before merging this. Another issue is that I failed trying to test it. |
New version in #78
To remove.