-
Notifications
You must be signed in to change notification settings - Fork 5
Refactor: Introduce standardized RESTError and QueryError types
#343
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
base: main
Are you sure you want to change the base?
Conversation
…or consistent error handling across modules, replacing raw strings or `anyhow::Error`
…dant `RESTResponse` implementation
…ndlers, streamline `RESTError` methods for flexibility and consistency across modules
…ity in `accounts_state.rs` and `accounts.rs`
…mprove code consistency, and reintroduce `/drdd` and `/spdd` REST endpoints
…und` and `QueryFailed`, standardize error handling with `internal_error`, and update method signatures across modules
…ove error handling formatting across modules
c103432 to
6aa66de
Compare
3ba464a to
1bc9e65
Compare
# Conflicts: # modules/accounts_state/src/accounts_state.rs # modules/address_state/src/address_state.rs # modules/assets_state/src/assets_state.rs # modules/chain_store/src/chain_store.rs # modules/drep_state/src/drep_state.rs # modules/epochs_state/src/epochs_state.rs # modules/governance_state/src/governance_state.rs # modules/historical_accounts_state/src/historical_accounts_state.rs # modules/rest_blockfrost/src/handlers/accounts.rs # modules/rest_blockfrost/src/handlers/addresses.rs # modules/rest_blockfrost/src/handlers/assets.rs # modules/rest_blockfrost/src/handlers/blocks.rs # modules/rest_blockfrost/src/handlers/epochs.rs # modules/rest_blockfrost/src/handlers/governance.rs # modules/rest_blockfrost/src/handlers/pools.rs # modules/spo_state/src/spo_state.rs # modules/utxo_state/src/utxo_state.rs
8cb0a23 to
189edfb
Compare
… from `anyhow::Error` to `QueryError`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR standardizes error handling across REST endpoints by introducing RESTError and updating QueryError types. The changes eliminate anyhow::Result in favor of strongly-typed error handling, providing more consistent HTTP status codes and error messages.
Key changes:
- Handler signatures updated from
Result<RESTResponse>toResult<RESTResponse, RESTError> - Query state closures now return
QueryErrorinstead of wrapping errors inanyhow - Standardized parameter validation using
RESTError::invalid_param(),RESTError::param_missing(), etc.
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/spdd_state/src/rest.rs | Updated SPDD handler to use RESTError |
| modules/rest_blockfrost/src/rest_blockfrost.rs | Updated handler registration signatures and cleaned up imports |
| modules/rest_blockfrost/src/handlers/pools.rs | Refactored pool handlers to use RESTError with proper error conversion |
| modules/rest_blockfrost/src/handlers/governance.rs | Updated governance handlers to use RESTError and standardized message bus error handling |
| modules/rest_blockfrost/src/handlers/epochs.rs | Updated epoch handlers with RESTError and improved error messages |
| modules/rest_blockfrost/src/handlers/blocks.rs | Simplified block handlers by removing QueryError::NotFound handling |
| modules/rest_blockfrost/src/handlers/assets.rs | Refactored asset handlers to use RESTError consistently |
| modules/rest_blockfrost/src/handlers/addresses.rs | Updated address handlers with RESTError and improved error context |
| modules/rest_blockfrost/src/handlers/accounts.rs | Refactored account handlers with RESTError and improved error handling |
| modules/drdd_state/src/rest.rs | Updated DRDD handler to use RESTError with storage_disabled check |
| common/src/rest_helper.rs | Updated REST helper functions to handle RESTError properly |
| common/src/rest_error.rs | Fixed param_missing message format |
| common/src/queries/utils.rs | Updated query_state and rest_query_state to use QueryError |
| common/src/queries/errors.rs | Added Fromanyhow::Error implementation for QueryError |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
# Conflicts: # modules/rest_blockfrost/src/handlers/accounts.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
modules/rest_blockfrost/src/handlers/accounts.rs:534
- This error handling wasn't updated to use the new error pattern. It should be:
let json = serde_json::to_string_pretty(&rest_response)?;
Ok(RESTResponse::with_json(200, &json))This is inconsistent with the rest of the refactoring where serde_json errors are automatically converted to RESTError via the From trait.
match serde_json::to_string_pretty(&rest_response) {
Ok(json) => Ok(RESTResponse::with_json(200, &json)),
Err(e) => Ok(RESTResponse::with_text(
500,
&format!("Internal server error while serializing addresses: {e}"),
)),
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…actor error conversion
| Err(resp) => return Ok(resp), | ||
| }; | ||
| ) -> Result<RESTResponse, RESTError> { | ||
| let account = parse_stake_address(¶ms)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
| &format!("Internal server error while retrieving account info: {e}"), | ||
| )), | ||
| } | ||
| let json = serde_json::to_string_pretty(&rest_response)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also really nice!
whankinsiv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice job Matt! I really like how this PR cleans up the parameter parsing and serialization error handling.
Description
This PR refactors the REST handler error handling to use
RESTErrorconsistently instead ofanyhow. This takes the work fromQueryErrorin the state modules and creates what I think is a better error handling flow.Changes:
Result<RESTResponse>toResult<RESTResponse, RESTError>query_stateclosures to returnQueryErrorinstead of wrapping inanyhowQueryErrortoRESTErrorvia theFromtraitRESTError::invalid_param(),RESTError::param_missing(), etc.Related Issue(s)
Part of #313
How was this tested?
Checklist
Impact / Side effects
Error responses maintain the same HTTP status codes and structure.