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: remove bytes/HexBytes #15211

Merged
merged 11 commits into from
Mar 1, 2023

Conversation

chixiaowen
Copy link
Contributor

@chixiaowen chixiaowen commented Feb 28, 2023

Description

Closes: #15049

this pr has removed bytes/HexBytes in cosmos-sdk


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@chixiaowen chixiaowen changed the title Remove cmtbytes #15049 Refactor: Remove cmtbytes #15049 Feb 28, 2023
@chixiaowen chixiaowen changed the title Refactor: Remove cmtbytes #15049 refactor: remove bytes/HexBytes #15049 Feb 28, 2023
@chixiaowen chixiaowen changed the title refactor: remove bytes/HexBytes #15049 refactor: remove bytes/HexBytes Feb 28, 2023
@chixiaowen
Copy link
Contributor Author

chixiaowen commented Feb 28, 2023

I can not remove only one place,follow :
path: testutil/cli/cmt_mocks.go
func: func (m MockCometRPC) ABCIQueryWithOptions

because I need to modify package of cometbft

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

super nice docs, it looks good to me. It doesnt seem like we use the json encoding anywhere i can see.

UPGRADING.md Outdated Show resolved Hide resolved
@chixiaowen
Copy link
Contributor Author

super nice docs, it looks good to me. It doesnt seem like we use the json encoding anywhere i can see.

It seems that cmtbytes does not use func of MarshalJSON() nor UnmarshalJSON(), only use of the func String(), so that I do not use json encoding.

@facundomedica
Copy link
Member

facundomedica commented Feb 28, 2023

fmt.Sprintf("%X", e.Hash()) seems to be faster than strings.ToUpper(hex.EncodeToString(e.Hash())). Any objections to using fmt?

Note: hex.EncodeToString by itself is faster but because we do strings.ToUpper it's the other way around.

@chixiaowen
Copy link
Contributor Author

chixiaowen commented Mar 1, 2023

strings.ToUpper(hex.EncodeToString(e.Hash()))

@facundomedica I have made a bench test,
`func Benchmark_Fmt(b *testing.B) {
e := []byte("hello")
for i := 0; i < b.N; i++ {
fmt.Sprintf("%X", e)
}
}

func Benchmark_Hex(b *testing.B) {
e := []byte("hello")
for i := 0; i < b.N; i++ {
hex.EncodeToString(e)
}
}

func Benchmark_Shex(b *testing.B) {
e := []byte("hello")
for i := 0; i < b.N; i++ {
strings.ToUpper(hex.EncodeToString(e))
}
}`
and the result as follow:

goos: darwin
goarch: amd64
pkg: mytest/struct_01
cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz
Benchmark_Fmt
Benchmark_Fmt-16 9034696 125.7 ns/op
Benchmark_Hex
Benchmark_Hex-16 49447454 23.87 ns/op
Benchmark_Shex
Benchmark_Shex-16 18752376 64.16 ns/op
PASS

It seems that strings.ToUpper(hex.EncodeToString(e.Hash())) is faster than fmt.Sprintf("%X", e.Hash())`

@chixiaowen chixiaowen marked this pull request as ready for review March 1, 2023 05:02
@chixiaowen chixiaowen requested a review from a team as a code owner March 1, 2023 05:02
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm! We can fix the proto lint and some other things in a follow-up.
Thank you!

@@ -7,14 +7,18 @@ This guide provides instructions for upgrading to specific versions of Cosmos SD
### Migration to CometBFT (Part 2)

The Cosmos SDK has migrated in, its previous versions, to CometBFT.
Some functions have been renamed to reflect the naming change.
Some functions have been renamed to reflect the naming change. And the Cosmos SDK has removed the import of cmtbytes "github.com/cometbft/cometbft/libs/bytes".
There is something changed.Due to the import changes, this is a breaking change. Chains need to remove **entirely** their imports in their codebase, from direct and indirects imports.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
There is something changed.Due to the import changes, this is a breaking change. Chains need to remove **entirely** their imports in their codebase, from direct and indirects imports.
There is something changed. Due to the import changes, this is a breaking change. Chains need to remove **entirely** their imports in their codebase, from direct and indirects imports.

@julienrbrt julienrbrt enabled auto-merge (squash) March 1, 2023 17:28
@julienrbrt julienrbrt added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 1, 2023
@julienrbrt
Copy link
Member

I am going to merge this now (we can improve in a follow-up).
Thank you, @chixiaowen, for your contribution!

@julienrbrt julienrbrt merged commit a49151d into cosmos:main Mar 1, 2023
julienrbrt added a commit that referenced this pull request Mar 1, 2023
@julienrbrt julienrbrt mentioned this pull request Mar 1, 2023
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:CLI C:Simulations C:x/bank C:x/evidence
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor: remove bytes/HexBytes
5 participants