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.

Sign up for GitHub

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

Jump to bottom

Crypto: delete internal crypto #118

Merged
merged 30 commits into from
Feb 17, 2021
Merged

Crypto: delete internal crypto #118

merged 30 commits into from
Feb 17, 2021

Conversation

tarakby
Copy link
Contributor

@tarakby tarakby commented Nov 24, 2020 •
edited
Loading

Closes: #113

Description

  • deletes the crypto internal folder
  • uses flow-go/crypto without breaking changes to the SDK api.

I tried to not break any external api in this PR, but a better refactoring and cleaning could be done if I'm allowed to introduce breaking changes and remove redundant types from the sdk (for instance remove flow-go-sdk/crypto.PrivateKey and use flow-go/crypto.PrivateKey instead, and a few other examples). Let me know if such changes are possible. cc @psiemens @turbolent

@tarakby tarakby requested a review from psiemens as a code owner November 24, 2020 02:55
@tarakby tarakby requested a review from turbolent November 24, 2020 02:55
@tarakby tarakby closed this Nov 24, 2020
@tarakby tarakby reopened this Nov 24, 2020
@tarakby tarakby closed this Nov 24, 2020
@tarakby tarakby reopened this Nov 24, 2020
@tarakby tarakby self-assigned this Nov 24, 2020
tarakby
tarakby commented Nov 24, 2020
View reviewed changes
crypto/crypto.go Outdated Show resolved Hide resolved
tarakby
tarakby commented Nov 24, 2020
View reviewed changes
crypto/crypto.go Outdated Show resolved Hide resolved
tarakby
tarakby commented Nov 24, 2020
View reviewed changes
crypto/crypto.go Outdated Show resolved Hide resolved
turbolent
turbolent approved these changes Nov 25, 2020
View reviewed changes
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

crypto/crypto.go Outdated Show resolved Hide resolved
crypto/crypto.go Outdated Show resolved Hide resolved
@Kay-Zee Kay-Zee force-pushed the tarak/use-flow-go-crypto branch 2 times, most recently from 6a12299 to 243061e Compare December 30, 2020 01:48
@tarakby
Copy link
Contributor Author

tarakby commented Jan 8, 2021

Hey @psiemens, do you have a preference about making breaking changes to the SDK crypto APIs ? If you think breaking changes are fine, I'll make some cleaner changes to this PR. If breaking changes are a problem, we can merge the PR in its current state. Thanks!

psiemens
psiemens reviewed Jan 15, 2021
View reviewed changes
Copy link
Contributor

@psiemens psiemens left a comment

Choose a reason for hiding this comment

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

Looks good! Just added one point in a comment.

I'd prefer to avoid breaking changes for now, so the way you have it is great 👌

crypto/crypto.go Show resolved Hide resolved
flow.go Outdated
@@ -87,10 +87,10 @@ func (id ChainID) String() string {
// entityHasher is a thread-safe hasher used to hash Flow entities.
type entityHasher struct {
mut sync.Mutex
hasher crypto.Hasher
hasher hash.Hasher
Copy link
Contributor

Choose a reason for hiding this comment

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

Could these all stay as crypto.Hash and crypto.Hasher?

I'd prefer to have the rest of the SDK files depend only on flow-go-sdk/crypto

crypto/crypto_test.go Show resolved Hide resolved
@tarakby
Copy link
Contributor Author

tarakby commented Jan 22, 2021 •
edited
Loading

@turbolent Is there a way other than go mod tidy to get a consistent go.sum ?
The CI make check-tidy is showing a go.sum difference but go mod tidy doesn't seem to fix it 🤷‍♂️

@turbolent
Copy link
Member

@tarakby you probably just want to commit the changes in to go.sum file. If that's the output of go mod tidy, then that's what should be in the repo

psiemens
psiemens approved these changes Jan 22, 2021
View reviewed changes
Copy link
Contributor

@psiemens psiemens left a comment

Choose a reason for hiding this comment

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

This looks good! I think you just need to do a rebase and re-run go mod tidy to clear up the conflict. I can do that if you'd like 😄

@tarakby
Copy link
Contributor Author

tarakby commented Jan 28, 2021

Yes please @psiemens, feel free to merge it. You can try with the current PR or this one, it has the same changes but a cleaner history, my merging and rebasing doesn't seem to work 😅

@psiemens psiemens mentioned this pull request Feb 17, 2021
Closed
@psiemens psiemens merged commit 9a8f185 into master Feb 17, 2021
@psiemens psiemens deleted the tarak/use-flow-go-crypto branch February 17, 2021 18:03
@phillipleblanc phillipleblanc mentioned this pull request Jun 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@psiemens psiemens psiemens approved these changes

@turbolent turbolent turbolent approved these changes

@janezpodhostnik janezpodhostnik Awaiting requested review from janezpodhostnik janezpodhostnik is a code owner

Assignees

@tarakby tarakby

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

crypto: use flow-go/crypto instead of internal
4 participants
@tarakby @turbolent @psiemens @Kay-Zee