refactor: make mebibyte a constant #4225
Conversation
ZwangaMukwevho
requested review from
rach-id and
ninabarbakadze
and removed request for
a team
📝 WalkthroughWalkthroughThe pull request standardizes the usage of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/util/testnode/config.go (1)
25-25: Consider adding documentation for the exported constant.The constant has been correctly exported. Consider adding a doc comment to describe its purpose and usage since it's now part of the package's public API.
+// Mebibyte represents the number of bytes in one mebibyte (1024 * 1024 bytes) Mebibyte = 1_048_576 // bytesx/blob/ante/blob_share_decorator_test.go (1)
Line range hint
25-25: Great refactoring that aligns with DRY principles!The centralization of the mebibyte constant improves code maintainability by:
- Eliminating redundant declarations
- Reducing the risk of inconsistencies
- Making future updates easier to manage
Consider documenting this constant in the package documentation to help other developers understand its purpose and proper usage.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/benchmarks/benchmark_msg_send_test.go(1 hunks)app/default_overrides.go(3 hunks)app/default_overrides_test.go(3 hunks)app/test/big_blob_test.go(1 hunks)app/test/integration_test.go(1 hunks)app/test/unit_test.go(0 hunks)test/util/testnode/comet_node_test.go(1 hunks)test/util/testnode/config.go(1 hunks)x/blob/ante/blob_share_decorator_test.go(3 hunks)x/blob/ante/max_total_blob_size_ante_test.go(3 hunks)
💤 Files with no reviewable changes (1)
- app/test/unit_test.go
✅ Files skipped from review due to trivial changes (2)
- test/util/testnode/comet_node_test.go
- app/default_overrides.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (6)
app/default_overrides_test.go (1)
69-69: LGTM! Consistent usage of the centralized constant.The test assertions have been correctly updated to use
app.Mebibyteinstead of the local constant.Also applies to: 96-97
x/blob/ante/max_total_blob_size_ante_test.go (1)
47-47: LGTM! Consistent usage of the centralized constant.The test cases have been correctly updated to use
testnode.Mebibyteinstead of the local constant.Also applies to: 54-54, 72-72
x/blob/ante/blob_share_decorator_test.go (1)
44-44: LGTM! Consistent usage of the centralized constant.The test cases have been correctly updated to use
testnode.Mebibyteinstead of the local constant.Also applies to: 56-56, 62-62, 78-78
app/test/integration_test.go (1)
131-131: LGTM! Consistent usage of centralized constant.The change correctly uses the centralized
app.Mebibyteconstant for checking transaction size against CometBFT's mempool limit.app/benchmarks/benchmark_msg_send_test.go (1)
317-317: LGTM! Consistent usage of centralized constant.The change correctly uses the centralized
app.Mebibyteconstant for calculating block size in megabytes.app/test/big_blob_test.go (1)
42-42: LGTM! Consistent usage of centralized constant.The changes correctly use the centralized
app.Mebibyteconstant for configuring:
- Mempool maximum transaction bytes
- Consensus parameters maximum block bytes
The multiplication factor of 10 is preserved, maintaining the original test parameters.
Also applies to: 45-45
…a-app into refactor-mebibyte
…a-app into refactor-mebibyte
Addressed the import cycles, and other issues related to linting. |
| min := (14 * time.Second).Nanoseconds() | ||
| max := (16 * time.Second).Nanoseconds() | ||
| return time.Duration(randInRange(min, max)) | ||
| minimum := (14 * time.Second).Nanoseconds() | ||
| maximum := (16 * time.Second).Nanoseconds() | ||
| return time.Duration(randInRange(minimum, maximum)) |
There was a problem hiding this comment.
why were these also changed?
| func randInRange(min int64, max int64) int64 { | ||
| return rand.Int63n(max-min) + min | ||
| func randInRange(minimum int64, maximum int64) int64 { | ||
| return rand.Int63n(maximum-minimum) + minimum |
| @@ -36,6 +36,8 @@ import ( | |||
| coretypes "github.com/tendermint/tendermint/types" | |||
| ) | |||
|
|
|||
| const Mebibyte = 1048576 | |||
There was a problem hiding this comment.
if we want this to be a constant, then we might as well use a global constant in the constant package
There was a problem hiding this comment.
that's a good idea. Where is the constant package? you mean the one in core
There was a problem hiding this comment.
We have https://github.com/celestiaorg/celestia-app/tree/main/pkg/appconsts.
My uber nit is that we don't want Mebibyte part of the public API of any of our packages so seems fine to define it once per package as a private constant.
There was a problem hiding this comment.
got you 👍 My initial motivation is just to stop writing that magic number everywhere and give it a name
There was a problem hiding this comment.
My uber nit is that we don't want Mebibyte part of the public API of any of our packages so seems fine to define it once per package as a private constant.
I'm fine with whichever you prefer :D
Overview
This PR refactors the use of mebibyte by centralizing it as a constant to avoid redundant declarations and improve code maintainability. This aligns with DRY principles, ensuring consistency and simplifying future updates.
Issue: #4211