Lint generators and commit dirty changes #507
Conversation
WalkthroughThis pull request updates several GitHub Actions workflows and mock files. In the workflows, the version of the Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Actions Runner
participant Diff as diff-check Action
Runner->>Runner: Start lint-go Job
Runner->>Diff: Execute golines step ([email protected])
Runner->>Diff: Run SQL code generation command (go tool -modfile=tools/go.mod sqlc generate)
Runner->>Diff: Run code generation command (go generate ./...)
Runner->>Diff: Run mock generation command (go tool -modfile=tools/go.mod mockery)
sequenceDiagram
participant Runner as GitHub Actions Runner
participant Diff as diff-check Action
Runner->>Runner: Initialize Solidity job with updated cache keys and paths
Runner->>Diff: Execute abigen step (contracts/dev/generate)
Possibly related PRs
Suggested reviewers
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 ignored due to path filters (1)
tools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
.github/workflows/lint-go.yml(1 hunks).github/workflows/solidity.yml(1 hunks)contracts/pkg/groupmessages/GroupMessages.go(1 hunks)contracts/pkg/identityupdates/IdentityUpdates.go(1 hunks)contracts/pkg/nodes/Nodes.go(1 hunks)pkg/mocks/authn/mock_JWTVerifier.go(1 hunks)pkg/mocks/blockchain/mock_ChainClient.go(1 hunks)pkg/mocks/blockchain/mock_IBlockchainPublisher.go(1 hunks)pkg/mocks/indexer/mock_ChainReorgHandler.go(1 hunks)pkg/mocks/indexer/mock_IBlockTracker.go(1 hunks)pkg/mocks/metadata_api/mock_MetadataApiClient.go(1 hunks)pkg/mocks/mls_validationv1/mock_ValidationApiClient.go(1 hunks)pkg/mocks/mlsvalidate/mock_MLSValidationService.go(1 hunks)pkg/mocks/registry/mock_NodeRegistry.go(1 hunks)pkg/mocks/registry/mock_NodesContract.go(1 hunks)pkg/mocks/storer/mock_LogStorer.go(1 hunks)tools/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (13)
- pkg/mocks/registry/mock_NodesContract.go
- pkg/mocks/storer/mock_LogStorer.go
- pkg/mocks/indexer/mock_ChainReorgHandler.go
- pkg/mocks/indexer/mock_IBlockTracker.go
- pkg/mocks/registry/mock_NodeRegistry.go
- pkg/mocks/mlsvalidate/mock_MLSValidationService.go
- pkg/mocks/metadata_api/mock_MetadataApiClient.go
- pkg/mocks/authn/mock_JWTVerifier.go
- pkg/mocks/blockchain/mock_IBlockchainPublisher.go
- pkg/mocks/mls_validationv1/mock_ValidationApiClient.go
- contracts/pkg/nodes/Nodes.go
- pkg/mocks/blockchain/mock_ChainClient.go
- contracts/pkg/identityupdates/IdentityUpdates.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test (Node)
🔇 Additional comments (5)
.github/workflows/lint-go.yml (3)
35-38: Ensure SQL Code Generation Command Accuracy
Thesqlcstep uses the diff-check action to run SQL code generation with the command:go tool -modfile=tools/go.mod sqlc generateMake sure that this command works correctly with the current toolchain and that the dependency versions (especially related to Ethereum and SQLC) in
tools/go.modare compatible with your project requirements.
39-42: Validate Go Generate Step Integration
Thegeneratestep, which runsgo generate ./...via the diff-check action, is properly set up to trigger code generation. Verify that its execution does not introduce unexpected dirty changes in the repository (or that such changes are managed as intended by the workflow).
43-46: Review Mockery Generation Command
Themockerystep employs the diff-check action to execute:go tool -modfile=tools/go.mod mockeryConfirm that this command remains effective with the updated dependency configuration in
tools/go.modand that the regenerated mocks correctly reflect the current interfaces.tools/go.mod (1)
132-132: Confirm Ethereum Dependency Downgrade
The dependency ongithub.com/ethereum/go-ethereumhas been downgraded to versionv1.14.12. Please ensure that this downgrade does not introduce regressions compared to the previously used version (v1.15.2) and that it is fully compatible with the project’s requirements.contracts/pkg/groupmessages/GroupMessages.go (1)
1-3:❓ Verification inconclusive
Verify that bytecode changes are generated through proper tooling.
Since this is a generated file (as indicated by the header comment), ensure that the bytecode changes in the
Binfield are generated through the proper contract compilation and binding generation process, not manually edited.Run the following script to verify the bytecode changes:
Also applies to: 34-35
🏁 Script executed:
#!/bin/bash # Description: Verify that the bytecode changes are generated through proper tooling. # Test: Check if the file is generated using abigen by looking for its signature patterns rg -A 5 'Code generated - DO NOT EDIT' contracts/pkg/groupmessages/GroupMessages.go # Test: Check if there are any manual edits to the file by looking at git history git log -p contracts/pkg/groupmessages/GroupMessages.go | rg -v 'Code generated'Length of output: 170
Attention: File Not Found – Verify Generated File Integration
The expected file ("contracts/pkg/groupmessages/GroupMessages.go") could not be located in the repository. Please verify the following manually:
- Confirm that the generated binding file is produced using the proper tooling (e.g., via abigen) and that the generation process is intact.
- Check if the file has been renamed or relocated in the repository.
- Ensure that the bytecode changes in the
Binfield come from an automatic recompilation and binding generation process rather than from manual alterations.Once the file’s presence and correct generation are validated, update this review accordingly.
.github/workflows/solidity.yml
Outdated
| - name: abigen | ||
| uses: nickcharlton/diff-check@main | ||
| with: | ||
| command: contracts/dev/generate |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Pin the action version for better reproducibility.
Using @main for the diff-check action version can lead to reproducibility issues as the action could change without notice. Consider pinning to a specific version.
Apply this diff to pin the action version:
- uses: nickcharlton/diff-check@main
+ uses: nickcharlton/diff-check@v1Additionally, consider adding error handling and output verification for the generation process to ensure it completes successfully.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: abigen | |
| uses: nickcharlton/diff-check@main | |
| with: | |
| command: contracts/dev/generate | |
| - name: abigen | |
| uses: nickcharlton/diff-check@v1 | |
| with: | |
| command: contracts/dev/generate |
mkysel
force-pushed
the
mkysel/lint-generators
branch
2 times, most recently
from
4d80f00 to
886ea59
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/solidity.yml (1)
167-170: 🛠️ Refactor suggestionPin the diff-check Action Version
The
abigenstep usesnickcharlton/diff-check@main. For better reproducibility and to adhere to our previous recommendations, please pin the version (for example, tov1):- uses: nickcharlton/diff-check@main + uses: nickcharlton/diff-check@v1🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 170-170: no new line character at the end of file
(new-line-at-end-of-file)
🧹 Nitpick comments (1)
.github/workflows/solidity.yml (1)
170-170: Ensure a Newline at the End of FileThe static analysis tool flagged that there is no newline character at the end of this file. Please add a newline character after line 170 to comply with YAML linting guidelines.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 170-170: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/solidity.yml(2 hunks)contracts/dev/generate(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/dev/generate
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/solidity.yml
[error] 170-170: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: abis
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Test (Node)
- GitHub Check: Lint-Go
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (1)
.github/workflows/solidity.yml (1)
46-46: Cache Key Generation – Verify Directory HashingThe updated command now hashes the entire
contractsdirectory usinghashFiles('contracts'). Please confirm that this change intentionally covers the full directory instead of using a more specific glob pattern (e.g.,"contracts/**/*.sol") if you only want to capture Solidity files.
mkysel
force-pushed
the
mkysel/lint-generators
branch
from
c6b1af1 to
9655f5c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/lint-go.yml (1)
46-46: File Ending Newline
YAMLlint reported that there is no newline character at the end of the file. Adding a newline at the end of the file improves compatibility with some tools and adheres to best practices.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 46-46: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/solidity.yml (3)
46-46: Review Cache Key Generation Pattern
The cache key is set usinghashFiles('contracts'), which might not capture changes in nested subdirectories or individual files if the intention is to hash all contents. Consider using a glob pattern (for example,hashFiles('contracts/**/*')) to ensure that any change inside the contracts folder invalidates the cache appropriately.
167-170: Consolidated Abigen Step & Error Handling
This new step usesnickcharlton/[email protected]for ABI generation, which is excellent for reproducibility. However, adding error handling or output verification after executingcontracts/dev/generatecould improve robustness. Explicitly catching or logging failures would help diagnose issues during ABI generation.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 170-170: no new line character at the end of file
(new-line-at-end-of-file)
170-170: Add Newline at End of File
YAMLlint indicates that there is no newline at the end of the file. Adding a newline will satisfy the linter and adhere to best practices for file formatting.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 170-170: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
.github/workflows/lint-go.yml(1 hunks).github/workflows/solidity.yml(2 hunks)contracts/dev/generate(3 hunks)pkg/mocks/authn/mock_JWTVerifier.go(1 hunks)pkg/mocks/blockchain/mock_ChainClient.go(1 hunks)pkg/mocks/blockchain/mock_IBlockchainPublisher.go(1 hunks)pkg/mocks/indexer/mock_ChainReorgHandler.go(1 hunks)pkg/mocks/indexer/mock_IBlockTracker.go(1 hunks)pkg/mocks/metadata_api/mock_MetadataApiClient.go(1 hunks)pkg/mocks/mls_validationv1/mock_ValidationApiClient.go(1 hunks)pkg/mocks/mlsvalidate/mock_MLSValidationService.go(1 hunks)pkg/mocks/registry/mock_NodeRegistry.go(1 hunks)pkg/mocks/registry/mock_NodesContract.go(1 hunks)pkg/mocks/storer/mock_LogStorer.go(1 hunks)tools/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/mocks/registry/mock_NodesContract.go
- pkg/mocks/blockchain/mock_IBlockchainPublisher.go
- pkg/mocks/indexer/mock_ChainReorgHandler.go
- pkg/mocks/storer/mock_LogStorer.go
- pkg/mocks/mls_validationv1/mock_ValidationApiClient.go
- pkg/mocks/authn/mock_JWTVerifier.go
- pkg/mocks/mlsvalidate/mock_MLSValidationService.go
- contracts/dev/generate
- pkg/mocks/indexer/mock_IBlockTracker.go
- pkg/mocks/metadata_api/mock_MetadataApiClient.go
- tools/go.mod
- pkg/mocks/registry/mock_NodeRegistry.go
- pkg/mocks/blockchain/mock_ChainClient.go
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/solidity.yml
[error] 170-170: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/lint-go.yml
[error] 46-46: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint-Go
- GitHub Check: Test (Node)
- GitHub Check: Build pre-baked anvil-xmtpd
🔇 Additional comments (4)
.github/workflows/lint-go.yml (4)
31-35: Golines Step Update Verification
The "golines" step now usesnickcharlton/[email protected], which replaces the previous reference (presumably "main"). Please confirm that this specific version meets your stability and functionality requirements in your environment.
35-38: SQL Code Generation Step Addition
The new "sqlc" step leveragesgo tool -modfile=tools/go.mod sqlc generatewith the diff-check action. This addition helps ensure that SQL code is generated as part of the lint workflow. Verify that the command produces the desired output and integrates well with your CI process.
39-42: Go Generate Step Addition
The "generate" step now invokesgo generate ./...via the diff-check action. This helps catch generated code that might become stale. Please ensure that this change covers all your required modules and that any side effects (such as file modifications) are properly handled by your workflow.
43-46: Mockery Step Addition
A new "mockery" step has been added to rungo tool -modfile=tools/go.mod mockery, aligning with the updated mock generation tool (v2.52.2). Double-check that this command correctly picks up the new version and regenerates mocks as expected.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 46-46: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
.github/workflows/solidity.yml (2)
104-110: 🛠️ Refactor suggestionInconsistent Cache Restoration in Lint Job
The Lint job still restores cache with
path: contracts, which is inconsistent with the updated approach used elsewhere. Consider updating this to match the refined subdirectories for better granularity and consistency.Proposed diff:
- path: contracts + path: | + contracts/build + contracts/cache + contracts/dependencies + contracts/out
128-134: 🛠️ Refactor suggestionInconsistent Cache Restoration in Slither Job
Similar to the Lint job, the Slither job uses
path: contractswhen restoring the cache. For consistency, updating it to use the specific subdirectories (as done in other jobs) is recommended.Proposed diff:
- path: contracts + path: | + contracts/build + contracts/cache + contracts/dependencies + contracts/out
🧹 Nitpick comments (1)
.github/workflows/solidity.yml (1)
184-184: Minor Formatting: Add Trailing NewlineA newline at the end of the file is missing. Adding one will prevent potential issues with certain linters or tools.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 184-184: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/solidity.yml(6 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.github/workflows/solidity.yml
[error] 184-184: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Lint-Go
- GitHub Check: Test (Node)
🔇 Additional comments (4)
.github/workflows/solidity.yml (4)
46-46: Cache Key Generation: Validate Hashing ScopeUsing
hashFiles('contracts/**/*')ensures that all files under the contracts directory are included when generating the cache key. Please verify that this glob pattern meets your caching requirements compared to the previous pattern (which targeted Solidity files only).
57-61: Cache Paths: Enhanced PrecisionThe cache paths are now scoped to specific subdirectories (
contracts/build,contracts/cache,contracts/dependencies, andcontracts/out), which should help improve the caching efficiency by excluding unnecessary files. Ensure that these directories cover all the necessary build artifacts.
77-88: Test Job Cache Restore: Correct Directory SpecifiedThe Test job now restores the cache using the refined subdirectories instead of the entire contracts folder. This aligns with the changes in the Init job and should result in more efficient caching.
181-184: Abigen Step: Properly Pinning Diff-Check ActionThe new abigen step correctly pins the
nickcharlton/diff-checkaction tov1.0.0, ensuring reproducibility in ABI generation. Double-check that the commandcontracts/dev/generateis valid and that any potential errors are handled appropriately.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 184-184: no new line character at the end of file
(new-line-at-end-of-file)
Summary by CodeRabbit