Revert direct pushes to main#85
Conversation
📝 WalkthroughWalkthroughThis pull request removes production dependencies ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/trades.rs (1)
27-77:⚠️ Potential issue | 🔴 CriticalMounted routes call
todo!()— requests will panic in production.Both
get_trades_by_tx(Line 38) andget_trades_by_address(Line 73) unconditionally panic viatodo!(), yet they are mounted at/v1/tradesinsrc/main.rs(Line 137), listed in the OpenAPIpaths(Lines 66–67 ofsrc/main.rs), and documented indocs/src/trades.mdas supported endpoints. Any authenticated request that passes the guards will panic the handler worker and return a raw 500 that does not flow throughApiError, contradicting the guideline that all API errors must go through theApiErrorenum.I understand this restores the pre–direct-push state, but the revert keeps these endpoints publicly reachable while non-functional. At a minimum, replace
todo!()with a properApiErrorresponse (or gate the mount behind a feature flag) until the real implementation lands.🛡️ Proposed fix — return a structured ApiError instead of panicking
async move { tracing::info!(tx_hash = ?tx_hash, "request received"); - let _raindex = shared_raindex.read().await; - todo!() + let _raindex = shared_raindex.read().await; + Err(ApiError::Internal("trades-by-tx endpoint not implemented".into())) } .instrument(span.0) .awaitasync move { tracing::info!(address = ?address, params = ?params, "request received"); - let _raindex = shared_raindex.read().await; - todo!() + let _raindex = shared_raindex.read().await; + Err(ApiError::Internal("trades-by-address endpoint not implemented".into())) } .instrument(span.0) .awaitAlternatively, un-mount
routes::trades::routes()and drop these entries fromApiDocuntil implementations exist.As per coding guidelines: "All API errors must go through the
ApiErrorenum, never return raw status codes" and Rust production code should "handle errors gracefully or exit with a message" rather than panicking viatodo!()/unwrap/expect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/trades.rs` around lines 27 - 77, Both handlers get_trades_by_tx and get_trades_by_address currently call todo!() which will panic; replace those todo!() calls with returning a proper ApiError (i.e., return Err(ApiError::...) from the async block) so the error flows through your API error handling instead of crashing the worker; include a trace/info log mentioning the endpoint and tx_hash/address, and ensure the returned error uses an appropriate variant (e.g., an “unimplemented” or “internal” ApiError) so callers receive a structured ApiError response for TradesByTxResponse/TradesByAddressResponse paths until the real implementations (which will use SharedRaindexProvider) are added.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/routes/trades.rs`:
- Around line 27-77: Both handlers get_trades_by_tx and get_trades_by_address
currently call todo!() which will panic; replace those todo!() calls with
returning a proper ApiError (i.e., return Err(ApiError::...) from the async
block) so the error flows through your API error handling instead of crashing
the worker; include a trace/info log mentioning the endpoint and
tx_hash/address, and ensure the returned error uses an appropriate variant
(e.g., an “unimplemented” or “internal” ApiError) so callers receive a
structured ApiError response for TradesByTxResponse/TradesByAddressResponse
paths until the real implementations (which will use SharedRaindexProvider) are
added.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 959d32fb-5e27-4440-97ec-9eddfa5087ee
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlkeys.nixos.nixsrc/cache.rssrc/direct_trades.rssrc/error.rssrc/main.rssrc/raindex/config.rssrc/routes/order/get_order.rssrc/routes/orders/get_by_owner.rssrc/routes/orders/get_by_token.rssrc/routes/orders/mod.rssrc/routes/trades.rssrc/test_helpers.rssrc/types/orders.rssrc/types/trades.rs
💤 Files with no reviewable changes (5)
- Cargo.toml
- src/types/trades.rs
- src/error.rs
- src/direct_trades.rs
- src/cache.rs
How to use the Graphite Merge QueueAdd the label add-to-gt-merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Merge activity
|
## Motivation Three commits were pushed directly to `main` without the normal PR review flow: - `543a103` - Add order caching and fix trades-by-address timeout - `50e7fd9` - Add metaboard neutralization, order caching, direct trades, quote chunking, and nginx hardening - `b1ebea3` - Skip RPC quotes for zero-balance orders and add stage timing Branch protection currently prevents rewriting `main`, so this PR restores the reviewed tree state with a normal revert commit instead. The original changes are preserved for review in #82, #83, and #84. ## Solution - Revert the three direct-pushed commits in newest-to-oldest order. - Fold the revert into a single commit for review clarity. - Leave the original changes available in the stacked review PRs. ## Checks - `nix develop -c cargo fmt` - `nix develop -c rainix-rs-static` Note: `rainix-rs-static` passed, with existing warnings reported in the nested `rain.orderbook` crate. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Removed Features** * Disabled trade-related API endpoints (`GET /trades/by-tx`, `GET /trades/by-address`, batch trade queries). * Removed in-memory caching layer for order listings. * **Improvements** * Simplified application architecture by removing unnecessary caching mechanisms and dependencies. * Streamlined order retrieval logic. * **Infrastructure** * Updated server configuration and system settings. * Removed local database storage layer integration. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
graphite-app
Bot
force-pushed
the
revert/alastair-direct-main-pushes
branch
from
bb989eb to
5951bd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/routes/trades.rs (1)
28-77:⚠️ Potential issue | 🟠 MajorHeads up: both handlers will panic at runtime via
todo!().Both
get_trades_by_tx(line 38) andget_trades_by_address(line 73) are still mounted at/v1/tradesinsrc/main.rs(line 137) and advertised in OpenAPI (src/main.rslines 66-67 with a200response shape). Any request to either endpoint will:
- Acquire the
shared_raindexread lock for no reason, then- Hit
todo!(), which panics — Rocket will surface a500to the client instead of a properly typedApiError.This conflicts with the coding guideline that all API errors go through the
ApiErrorenum and that production code should handle errors gracefully (the panic-on-hit semantics oftodo!()are in the same family asunwrap/expect).I understand this PR is a deliberate revert restoring the prior state, and the real implementations live in PRs
#82/#83/#84 — but until one of those lands, consider either (a) returning an explicitApiError(e.g.,ApiError::Internal("trades endpoint not yet implemented".into())or a503-mapped variant) so the OpenAPI contract is honored, or (b) temporarily un-mounting these routes inmain.rsand dropping them from the OpenAPIpaths(...)list to avoid advertising a surface that 500s.As per coding guidelines: "All API errors must go through the
ApiErrorenum, never return raw status codes" and "Never useexpectorunwrapin production code; handle errors gracefully or exit with a message".🛡️ Sketch: graceful placeholder until the real implementations land
pub async fn get_trades_by_tx( _global: GlobalRateLimit, _key: AuthenticatedKey, - shared_raindex: &State<crate::raindex::SharedRaindexProvider>, + _shared_raindex: &State<crate::raindex::SharedRaindexProvider>, span: TracingSpan, tx_hash: ValidatedFixedBytes, ) -> Result<Json<TradesByTxResponse>, ApiError> { async move { tracing::info!(tx_hash = ?tx_hash, "request received"); - let _raindex = shared_raindex.read().await; - todo!() + Err(ApiError::Internal( + "trades-by-tx endpoint not yet implemented".into(), + )) } .instrument(span.0) .await }(Apply the analogous change to
get_trades_by_address.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/trades.rs` around lines 28 - 77, Both handlers get_trades_by_tx and get_trades_by_address contain todo!() which will panic at runtime and hold the shared_raindex read lock; replace the todo!() with an explicit ApiError return so the endpoints fail gracefully (e.g., return Err(ApiError::Internal("trades endpoint not yet implemented".into())) or another 503-mapped ApiError variant), ensuring the function signatures (get_trades_by_tx and get_trades_by_address) return a proper Err(ApiError) instead of panicking; optionally remove the unnecessary shared_raindex.read().await if you don’t need the lock for the placeholder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@os.nix`:
- Around line 128-131: Confirm which DigitalOcean block device is actually
attached to the production droplet and update the mount entry accordingly: check
the node for the by-id name (e.g., whether it is
"scsi-0DO_Volume_st0x-rest-api-data-v2" or
"scsi-0DO_Volume_st0x-rest-api-data"), then in the Nix expression update the
fileSystems."/mnt/data" device value to the exact by-id string that exists (or
have ops reattach the non-v2 volume before merging); ensure the updated device
string replaces the current device value in the fileSystems."/mnt/data" block so
systemd mount units will succeed.
---
Outside diff comments:
In `@src/routes/trades.rs`:
- Around line 28-77: Both handlers get_trades_by_tx and get_trades_by_address
contain todo!() which will panic at runtime and hold the shared_raindex read
lock; replace the todo!() with an explicit ApiError return so the endpoints fail
gracefully (e.g., return Err(ApiError::Internal("trades endpoint not yet
implemented".into())) or another 503-mapped ApiError variant), ensuring the
function signatures (get_trades_by_tx and get_trades_by_address) return a proper
Err(ApiError) instead of panicking; optionally remove the unnecessary
shared_raindex.read().await if you don’t need the lock for the placeholder.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eed0b89a-058e-4349-aaa5-1bc76cb7a10e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
Cargo.tomlkeys.nixos.nixsrc/cache.rssrc/direct_trades.rssrc/error.rssrc/main.rssrc/raindex/config.rssrc/routes/order/get_order.rssrc/routes/orders/get_by_owner.rssrc/routes/orders/get_by_token.rssrc/routes/orders/mod.rssrc/routes/trades.rssrc/test_helpers.rssrc/types/orders.rssrc/types/trades.rs
💤 Files with no reviewable changes (5)
- Cargo.toml
- src/error.rs
- src/types/trades.rs
- src/direct_trades.rs
- src/cache.rs
✅ Files skipped from review due to trivial changes (2)
- keys.nix
- src/raindex/config.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/routes/orders/mod.rs
- src/types/orders.rs
| fileSystems."/mnt/data" = { | ||
| device = "/dev/disk/by-id/scsi-0DO_Volume_st0x-rest-api-data-v2"; | ||
| device = "/dev/disk/by-id/scsi-0DO_Volume_st0x-rest-api-data"; | ||
| fsType = "ext4"; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find any other references to the old/new volume identifiers to confirm which is canonical in this repo.
rg -nP -C2 'DO_Volume_st0x-rest-api-data(-v2)?'Repository: ST0x-Technology/st0x.rest.api
Length of output: 252
🏁 Script executed:
#!/bin/bash
# Check git history of os.nix line 129 to see what was changed
git log -p --follow -S 'DO_Volume_st0x-rest-api-data' -- os.nix | head -100Repository: ST0x-Technology/st0x.rest.api
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Also search for any -v2 references in the entire codebase
rg 'st0x-rest-api-data-v2|DO_Volume.*v2' --type nix --type txt --type mdRepository: ST0x-Technology/st0x.rest.api
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Check git diff to see what this PR actually changes
git diff HEAD~1 os.nix 2>/dev/null || git diff os.nix 2>/dev/null || echo "No git diff available"Repository: ST0x-Technology/st0x.rest.api
Length of output: 55
🏁 Script executed:
#!/bin/bash
# List recent commits affecting os.nix with one-line summaries
git log --oneline -20 -- os.nixRepository: ST0x-Technology/st0x.rest.api
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Show the full diff of the last commit that touched os.nix
git log -1 -p -- os.nix | head -150Repository: ST0x-Technology/st0x.rest.api
Length of output: 4581
🏁 Script executed:
#!/bin/bash
# Check git status and any staged changes
git status os.nix
git diff --cached os.nix 2>/dev/null || echo "No staged changes"Repository: ST0x-Technology/st0x.rest.api
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Search for any mention of v2 in os.nix or related files
cat os.nix | grep -i v2 || echo "No v2 found in os.nix"Repository: ST0x-Technology/st0x.rest.api
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Show the three commits that were reverted
git show 543a103:os.nix 2>/dev/null | grep -A5 'mnt/data' || echo "os.nix not in 543a103"
git show 50e7fd9:os.nix 2>/dev/null | grep -A5 'mnt/data' || echo "os.nix not in 50e7fd9"
git show b1ebea3:os.nix 2>/dev/null | grep -A5 'mnt/data' || echo "os.nix not in b1ebea3"Repository: ST0x-Technology/st0x.rest.api
Length of output: 1903
🏁 Script executed:
#!/bin/bash
# Check git log for all commits and their affected files
git log --oneline --all -30Repository: ST0x-Technology/st0x.rest.api
Length of output: 1931
🏁 Script executed:
#!/bin/bash
# Show which files were modified by the three direct pushes
git show --name-status 543a103 | head -20
git show --name-status 50e7fd9 | head -20
git show --name-status b1ebea3 | head -20Repository: ST0x-Technology/st0x.rest.api
Length of output: 2429
Verify the reverted /mnt/data device by-id matches the currently attached DigitalOcean volume.
This revert switches the mount target from scsi-0DO_Volume_st0x-rest-api-data-v2 (introduced in commit 50e7fd9) back to scsi-0DO_Volume_st0x-rest-api-data. If the production droplet's attached volume is still the -v2 variant, the mount unit will fail at activation, breaking /mnt/data and cascading failures for logs, systemd ReadWritePaths, and logrotate/tmpfiles rules. Confirm with ops which volume is currently attached and either keep the -v2 identifier or ensure the non-v2 volume is attached before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@os.nix` around lines 128 - 131, Confirm which DigitalOcean block device is
actually attached to the production droplet and update the mount entry
accordingly: check the node for the by-id name (e.g., whether it is
"scsi-0DO_Volume_st0x-rest-api-data-v2" or
"scsi-0DO_Volume_st0x-rest-api-data"), then in the Nix expression update the
fileSystems."/mnt/data" device value to the exact by-id string that exists (or
have ops reattach the non-v2 volume before merging); ensure the updated device
string replaces the current device value in the fileSystems."/mnt/data" block so
systemd mount units will succeed.
3 participants
Motivation
Three commits were pushed directly to
mainwithout the normal PR review flow:543a103- Add order caching and fix trades-by-address timeout50e7fd9- Add metaboard neutralization, order caching, direct trades, quote chunking, and nginx hardeningb1ebea3- Skip RPC quotes for zero-balance orders and add stage timingBranch protection currently prevents rewriting
main, so this PR restores the reviewed tree state with a normal revert commit instead. The original changes are preserved for review in #82, #83, and #84.Solution
Checks
nix develop -c cargo fmtnix develop -c rainix-rs-staticNote:
rainix-rs-staticpassed, with existing warnings reported in the nestedrain.orderbookcrate.Summary by CodeRabbit
Removed Features
Behavior Changes
Improvements
Infrastructure