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

Refactor: Move ledger code to its own subfolder #6780

Closed
4 tasks
ValarDragon opened this issue Jul 19, 2020 · 5 comments · Fixed by #6817
Closed
4 tasks

Refactor: Move ledger code to its own subfolder #6780

ValarDragon opened this issue Jul 19, 2020 · 5 comments · Fixed by #6817
Assignees
@alessio
Labels
C:Crypto C:Ledger Issues and features related Ledger integration and functionality Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.

Comments

@ValarDragon
Copy link
Contributor

Summary

Currently there are 5 files related to the ledger in /crypto. A minor change that I think improves overall codestructure would be to move all of these to their own subfolder /crypto/ledger.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@fedekunze
Copy link
Collaborator

cc: @alessio

@fedekunze fedekunze added Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. C:Ledger Issues and features related Ledger integration and functionality C:Crypto labels Jul 19, 2020
@alessio
Copy link
Contributor

alessio commented Jul 20, 2020

IMHO Ledger-related amount of code is too small for it to justify the creation of another directory (ref: https://about.sourcegraph.com/go/idiomatic-go#tiny-package-syndrome)

@ValarDragon
Copy link
Contributor Author

ValarDragon commented Jul 22, 2020 •
edited
Loading

The point of the tiny-package syndrome was to argue that 1-2 file large packages aren't a great idea in some cases.

Here the ledger code is the majority of the directory, which is unexpected functionality imo of this top level dir. Its responsiblity at the moment is Handling encoding of things to disk (armor*.go), handling encoding of go objects for the sub-directories (amino.go, test_encoding.go), explaining what its sub-directories do (bcrypt_readme), and oddly handling all ledger logic.

Feels like packaging the ledger logic is good for modularity, but its up to the repo maintainers.

@alessio
Copy link
Contributor

alessio commented Jul 22, 2020

OK, I think you have a point @ValarDragon. I'll take care of the migration shortly

@alessio alessio self-assigned this Jul 22, 2020
alessio pushed a commit that referenced this issue Jul 22, 2020
crypto: move ledger code to its own subfolder
befd78d 
crypto -> crypto/ledger:
- crypto.LedgerShowAddress -> ledger.ShowAddress
- crypto.NewPrivKeyLedgerSecp256k1 - > ledger.NewPrivKeySecp256k1
- crypto.NewPrivKeyLedgerSecp256k1Unsafe -> ledger.NewPrivKeySecp256k1Unsafe

Closes: #6780
@alessio alessio mentioned this issue Jul 22, 2020
Merged
9 tasks
alessio pushed a commit that referenced this issue Jul 22, 2020
crypto: move ledger code to its own subfolder (#6817)
e0a81d2 
crypto -> crypto/ledger:
- crypto.LedgerShowAddress -> ledger.ShowAddress
- crypto.NewPrivKeyLedgerSecp256k1 - > ledger.NewPrivKeySecp256k1
- crypto.NewPrivKeyLedgerSecp256k1Unsafe -> ledger.NewPrivKeySecp256k1Unsafe

Closes: #6780
@ValarDragon
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alessio alessio

Labels
C:Crypto C:Ledger Issues and features related Ledger integration and functionality Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

crypto: move ledger code to its own subfolder cosmos/cosmos-sdk
3 participants
@alessio @ValarDragon @fedekunze