fix(epochs): block null epoch
#282
Conversation
WalkthroughThis pull request encompasses updates to the Dockerfile and epoch-related type validation in the project. The Dockerfile modification involves updating the Changes
Suggested Reviewers
Poem
Finishing Touches
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Dockerfile(2 hunks)x/epochs/types/genesis.go(1 hunks)x/epochs/types/identifier.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- Dockerfile
🧰 Additional context used
🪛 GitHub Actions: Tests
x/epochs/types/identifier.go
[error] Package test failed with coverage: 2.4% of statements
x/epochs/types/genesis.go
[error] Package test failed with coverage: 2.4% of statements
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: goreleaser
🔇 Additional comments (7)
x/epochs/types/genesis.go (3)
45-46: LGTM! Good refactoring of validation logic.Moving the identifier validation to a dedicated function improves code organization and reusability.
🧰 Tools
🪛 GitHub Actions: Tests
[error] Package test failed with coverage: 2.4% of statements
51-51: LGTM! Helpful clarification.Good addition of the comment clarifying that
EpochNumbercan be 0 at genesis.🧰 Tools
🪛 GitHub Actions: Tests
[error] Package test failed with coverage: 2.4% of statements
Line range hint
45-58: Add tests to improve coverage.The pipeline shows only 2.4% test coverage. Consider adding tests for:
- Validation of invalid epoch identifiers (empty, null)
- Validation of negative values for CurrentEpoch and CurrentEpochStartHeight
- Validation of non-positive Duration
Let's check existing test coverage:
🧰 Tools
🪛 GitHub Actions: Tests
[error] Package test failed with coverage: 2.4% of statements
x/epochs/types/identifier.go (4)
20-20: LGTM! Improved clarity in documentation.The comment update from "So it's" to "Since it's" improves readability.
🧰 Tools
🪛 GitHub Actions: Tests
[error] Package test failed with coverage: 2.4% of statements
40-41: LGTM! Better documentation.Good clarification that this is a convenience method for other modules.
🧰 Tools
🪛 GitHub Actions: Tests
[error] Package test failed with coverage: 2.4% of statements
45-49: LGTM! Robust validation implementation.Good implementation of the null epoch check with clear error messages. This directly addresses the PR objective of blocking the null epoch identifier.
🧰 Tools
🪛 GitHub Actions: Tests
[error] Package test failed with coverage: 2.4% of statements
53-54: LGTM! Improved readability.Good formatting of the long comment line into two lines for better readability.
🧰 Tools
🪛 GitHub Actions: Tests
[error] Package test failed with coverage: 2.4% of statements
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/epochs/types/identifier_test.go (2)
23-51: Consider adding specific error message assertions.The test cases verify error presence but don't validate the specific error messages. Consider adding assertions to verify the exact error messages to ensure the validation logic returns the expected errors.
Example improvement for the "invalid epoch identifier by type" test case:
t.Run("invalid epoch identifier by type", func(t *testing.T) { err := types.ValidateEpochIdentifierInterface(invalidEpochIDByType) - if err == nil { - t.Fatal("expected error, got nil") + if err == nil || err.Error() != "expected string type for epoch identifier" { + t.Fatalf("expected 'expected string type for epoch identifier' error, got %v", err) } })
76-127: LGTM! Comprehensive epoch validation test coverage.Both functions provide thorough coverage of epoch validation scenarios. Consider enhancing the error checking similar to the earlier suggestion to verify specific error messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
x/epochs/types/genesis_test.go(1 hunks)x/epochs/types/identifier_test.go(1 hunks)
🔇 Additional comments (4)
x/epochs/types/genesis_test.go (1)
66-66: LGTM! Error message is now more concise.The updated error message is clearer and more direct while maintaining the same validation behavior.
x/epochs/types/identifier_test.go (3)
12-12: Verify if spaces invalidEpochIDare intentional.The
validEpochIDconstant contains leading and trailing spaces. If these spaces are not part of the test requirements, consider trimming them.
53-74: LGTM! Good coverage of null epoch validation.The test cases effectively verify that the null epoch identifier is blocked, which aligns with the PR objective. The test structure is clean and covers the essential validation scenarios.
1-127: Well-organized test suite with good separation of concerns.The test file is well-structured with clear separation between different validation scenarios. Each test function focuses on a specific aspect of validation, making the test suite maintainable and easy to understand.
MaxMustermann2
requested review from
magj2006,
bwhour,
adu-web3,
cloud8little,
trestinlsd,
mikebraver,
TimmyExogenous and
leonz789
The
nullepoch is an internal epoch used to track undelegation. It should not be permitted as an identifier.Summary by CodeRabbit
Security Updates
ca-certificatespackage to version 20241121-r1 in Docker build environment and final image.Validation Improvements
Documentation
Tests