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

refactor: replace common package with pkg #1936

Merged
merged 49 commits into from
Mar 27, 2024

Conversation

skosito
Copy link
Contributor

@skosito skosito commented Mar 22, 2024

Description

Its a big PR, but the only place to review is common package renaming to pkg and split into subpackages (couple of files like commands, version and constant are still in pkg package).
Because of this split, i also did the same for common proto files, now called pkg too.

Best way to review this is to check it out locally and check proto and pkg files, eg. if organization in subpackages in ok, if naming of subpackages is ok etc - the rest of the changes are just fixing imports (apart from proof test which i will flag in the comment).

Closes: #1905

Type of change

Refactor

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Checklist:

  • I have added unit tests that prove my fix feature works

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 63.63636% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 51.92%. Comparing base (2b77e0b) to head (fcc1383).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1936      +/-   ##
===========================================
- Coverage    52.12%   51.92%   -0.21%     
===========================================
  Files          235      235              
  Lines        13347    13347              
===========================================
- Hits          6957     6930      -27     
- Misses        5953     5982      +29     
+ Partials       437      435       -2     
Files Coverage Δ
pkg/authz/authz_tx_types.go 100.00% <ø> (ø)
pkg/chains/address.go 100.00% <ø> (ø)
pkg/chains/bitcoin.go 100.00% <ø> (ø)
pkg/chains/chain.go 96.69% <ø> (ø)
pkg/chains/chain_id.go 100.00% <ø> (ø)
pkg/chains/chains.go 100.00% <ø> (ø)
pkg/chains/conversion.go 100.00% <ø> (ø)
pkg/coin/coin.go 100.00% <ø> (ø)
pkg/crypto/tss.go 82.60% <100.00%> (ø)
pkg/gas/gas_limits.go 100.00% <ø> (ø)
... and 6 more

... and 30 files with indirect coverage changes

@skosito skosito changed the title refactor: common package rename refactor: replace common package with pkg Mar 26, 2024
@skosito skosito marked this pull request as ready for review March 26, 2024 12:50
Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

It seems most of the content in cosmos.go is unused and can be removed (you can see those with Goland for ex)
I might even think this package is not needed anymore but removing it might be higher than the scope of this PR

pkg/version.go Outdated Show resolved Hide resolved
pkg/chains/utils.go Outdated Show resolved Hide resolved
pkg/commands.go Outdated Show resolved Hide resolved
@skosito
Copy link
Contributor Author

skosito commented Mar 26, 2024

@lumtis regarding cosmos package, i removed unused and replaced some usages to use sdk types directly dcb3b5a

only thing left in cosmos package are some legacybech32 methods, i thought to maybe leave them there instead of having legacy package imported on couple of places, and we can maybe open issue to replace those deprecated methods with proper ones?

@skosito skosito requested a review from lumtis March 26, 2024 16:28
@lumtis
Copy link
Member

lumtis commented Mar 26, 2024

only thing left in cosmos package are some legacybech32 methods, i thought to maybe leave them there instead of having legacy package imported on couple of places, and we can maybe open issue to replace those deprecated methods with proper ones?

This sounds good to me

@skosito skosito merged commit f693378 into develop Mar 27, 2024
21 checks passed
@skosito skosito deleted the refactor-common-package-rename branch March 27, 2024 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace common package with pkg
3 participants