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/keystore #40

Merged
merged 3 commits into from
May 18, 2020
Merged

Fix/keystore #40

merged 3 commits into from
May 18, 2020

Conversation

edison0xyz
Copy link
Contributor

The following error was thrown in the Keystore

# github.com/Zilliqa/gozilliqa-sdk/crypto
../../Zilliqa/gozilliqa-sdk/crypto/keystore.go:185:22: multiple-value uuid.NewV4() in single-value context

Issue has similarly being raised here. satori/go.uuid#106. Looks like the function now returns multiple values, which broke the implementation. I've just made a hack and it works.

To decide

Should we stop using this dependency (https://github.com/satori/go.uuid) since it was last maintained in 2018? I saw that you are using "github.com/google/uuid" under the workpool package. I looked through briefly and I think it can replace this repository since it also follows the RFC 4122 standard.

The original repo is dead, and someone tried to create a fork https://github.com/gofrs/uuid which is also dead.

Thoughts, @renlulu?

System

go version 1.14.2

@edison0xyz edison0xyz added the bug Something isn't working label May 14, 2020
@edison0xyz edison0xyz requested a review from renlulu May 14, 2020 16:17
Copy link
Contributor

@renlulu renlulu left a comment

Choose a reason for hiding this comment

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

Many thanks for the fix, yes, it's fine to me to use github.com/google/uuid to replace github.com/satori/go.uuid, would you like to do it in this PR xD

@edison0xyz
Copy link
Contributor Author

Many thanks for the fix, yes, it's fine to me to use github.com/google/uuid to replace github.com/satori/go.uuid, would you like to do it in this PR xD

Sure will do. Will get this out in a bit. Don't merge the PR first.

@renlulu
Copy link
Contributor

renlulu commented May 15, 2020

Many thanks for the fix, yes, it's fine to me to use github.com/google/uuid to replace github.com/satori/go.uuid, would you like to do it in this PR xD

Sure will do. Will get this out in a bit. Don't merge the PR first.

OK, thanks!

@renlulu renlulu self-requested a review May 15, 2020 08:12
@renlulu renlulu merged commit baf793c into dev May 18, 2020
@renlulu renlulu deleted the fix/keystore branch July 14, 2020 08:39
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.

2 participants