doc, refactor(apps/hermes/server): add code guidelines, enable extra clippy lints #2777
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
ali-behjati
approved these changes
Jun 11, 2025
Comment on lines
+681
to
+682
| .publish_time() | ||
| - i64::try_from(window_seconds).context("window size overflow")? |
There was a problem hiding this comment.
can we ban unchecked math operations?
There was a problem hiding this comment.
We should do it but I think it's better to do it a bit later because it requires more work to do it properly.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors various parts of the Hermes server code to improve linting, enforce stricter error context handling, and update integer types for consistency. Key changes include:
- Updating error handling by adding context to error messages using anyhow’s Context.
- Changing cache size types from u64 to usize and modifying related arithmetic.
- Adding extensive clippy lint configurations in Cargo.toml and updating allow attributes with explicit reasons.
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/hermes/server/src/state/wormhole.rs | Enhanced error context for VAA timestamp parsing and address slicing. |
| apps/hermes/server/src/state/metrics.rs | Replaced unwrap with error logging when encoding metrics. |
| apps/hermes/server/src/state/cache.rs | Updated cache size type and simplified while loops. |
| apps/hermes/server/src/state/benchmarks.rs | Added error context during URL construction. |
| apps/hermes/server/src/state/aggregate/wormhole_merkle.rs | Added clippy allow attributes for tests. |
| apps/hermes/server/src/state/aggregate/metrics.rs | Improved error handling in slot clearing with explicit expect. |
| apps/hermes/server/src/state/aggregate.rs | Updated integer conversion and error contexts during message state construction. |
| apps/hermes/server/src/state.rs | Changed parameter type from u64 to usize for cache size. |
| apps/hermes/server/src/serde.rs | Refactored hex deserialization to use strip_prefix for clarity. |
| apps/hermes/server/src/network/wormhole.rs | Enriched allow attributes with explicit reasons for unused code. |
| apps/hermes/server/src/main.rs | Updated shutdown signal and default argument handling with context. |
| apps/hermes/server/src/config/cache.rs | Changed cache size type to usize. |
| apps/hermes/server/src/config.rs | Added explicit reasons to clippy allow attributes. |
| apps/hermes/server/src/api/types.rs | Applied clippy test allow attributes. |
| apps/hermes/server/src/api/rest/get_vaa_ccip.rs | Simplified slicing by storing params.data in a temporary variable. |
| apps/hermes/server/src/api/rest.rs | Added clippy allow attributes to tests. |
| apps/hermes/server/src/api.rs | Allowed deprecated endpoints with explicit rationale. |
| apps/hermes/server/src/build.rs | Added allow for expect usage with the provided rationale. |
| apps/hermes/server/Cargo.toml | Introduced extensive clippy lint configuration. |
Comments suppressed due to low confidence (1)
apps/hermes/server/src/state/aggregate.rs:496
- Using unwrap_or_else to silently convert a retrieval error into an empty vector may hide underlying issues. Consider propagating the error instead of suppressing it to improve error traceability.
.await.unwrap_or_else(|err| {
tejasbadadare
approved these changes
Jun 11, 2025
apps/hermes/server/Cargo.toml
Outdated
| unsafe_code = "deny" | ||
|
|
||
| [lints.clippy] | ||
| # See https://www.notion.so/pyth-network/Rust-code-guidelines-20a2eecaaac98060b6b2d9a52c9f9141 |
There was a problem hiding this comment.
should we have a link to our private notion in this public repo? maybe you can publish the notion doc as a webpage and link that if you want to include it?
There was a problem hiding this comment.
Added guides directly to the crosschain repo for better visibility.
Riateche
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
We want to add better linting throughout our codebase. I also put together this guide to help deal with these new lints.
Rationale
To improve code quality: avoid panics, overflows and other suspicious code patterns.
How has this been tested?