Skip to content
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

Fix/Panic in SessionCreate #422

Merged
merged 2 commits into from
May 26, 2023
Merged

Fix/Panic in SessionCreate #422

merged 2 commits into from
May 26, 2023

Conversation

notimetoname
Copy link
Contributor

@notimetoname notimetoname commented May 24, 2023

Do not panic if signer was provided as a default one. Also, return ErrMissingSigner if no signer was provided at all.

@notimetoname notimetoname added the bug Something isn't working label May 24, 2023
@notimetoname notimetoname self-assigned this May 24, 2023
client/errors.go Show resolved Hide resolved
client/session.go Outdated Show resolved Hide resolved
client/session.go Outdated Show resolved Hide resolved
@roman-khimov roman-khimov added this to the v1.0.0-rc9 milestone May 24, 2023
Copy link
Contributor

@cthulhu-rider cthulhu-rider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw do u expect user.IDFromKey to panic on nil signer? It can return an error

client/session.go Outdated Show resolved Hide resolved
@notimetoname
Copy link
Contributor Author

btw do u expect user.IDFromKey to panic on nil signer? It can return an error

@cthulhu-rider, to be honest, I do not like getting a user from a signer at all cause signer must sign and it even can be not from our sdk at all. It is not smth unnatural to me when a Signer interface drops a Public() PublicKey cause it does not look like it can break Sign process but it will break user.IDFromSigner in our case. That is where my ideas break.
But that is what we get in RC 8, and the PR should just fix the panic sdk throws, so we can continue the discussion in some other issue.

cc @roman-khimov, @smallhive

@roman-khimov
Copy link
Member

signer must sign and it even can be not from our sdk at all

The problem is key->ID mapping. It's tied to Neo's verification scripts and that can only have 256r1 keys.

@notimetoname
Copy link
Contributor Author

@roman-khimov, if key->ID relation is that strong, i would not get a user from a Signer and return key->ID conversion (i did sdk update and it adds more code, not removes). Anyway, i do not have an answer to @cthulhu-rider's question: "do u expect user.IDFromKey to panic on nil signer?" and need help.

@roman-khimov
Copy link
Member

if key->ID relation is that strong

Then we have an option of adding ID() user.ID to Signer itself. It can internally do IDFromKey or whatever else (GetScriptHash()), but it's then left up to the Signer.

do u expect user.IDFromKey to panic on nil signer?

I do.

Do not panic if signer was provided as a default one. Also, return
`ErrMissingSigner` if no signer was provided at all.

Signed-off-by: Pavel Karpy <carpawell@morphbits.io>
@notimetoname
Copy link
Contributor Author

Then we have an option of adding ID() user.ID to Signer itself.

I like it more.

client/accounting.go Outdated Show resolved Hide resolved
client/accounting.go Show resolved Hide resolved
client/container_test.go Outdated Show resolved Hide resolved
Also, drop unnecessary punctuation in docs.

Signed-off-by: Pavel Karpy <carpawell@morphbits.io>
@roman-khimov roman-khimov merged commit c56e78b into master May 26, 2023
@roman-khimov roman-khimov deleted the fix/session-creation-panic branch May 26, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants