fix: Fix SDK breaking change for calling plugins method #607
Conversation
WalkthroughThe PR refactors plugin route handling by migrating the route parameter from the URL path to a query parameter while maintaining backwards compatibility. It also adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openapi.json (1)
927-1073: Clarify “route via URL path” vs what the OpenAPI actually defines (and improveroutequery-param ergonomics).Right now the descriptions advertise
/api/v1/plugins/{plugin_id}/call/verify, but this spec only defines/api/v1/plugins/{plugin_id}/call. If the path-suffix routing is intentionally supported-but-undocumented to prevent SDK churn, I’d explicitly say so (or remove the/call/verifyexample entirely). Also consider settingallowReserved: trueand adding anexample, since route values commonly include/.Proposed diff
"/api/v1/plugins/{plugin_id}/call": { "get": { @@ - "description": "This endpoint is disabled by default. To enable it for a given plugin, set\n`allow_get_invocation: true` in the plugin configuration.\n\nWhen invoked via GET:\n- `params` is an empty object (`{}`)\n- query parameters are passed to the plugin handler via `context.query`\n- wildcard route routing is supported the same way as POST (see `doc_call_plugin`)\n- Use the `route` query parameter or append the route to the URL path", + "description": "This endpoint is disabled by default. To enable it for a given plugin, set\n`allow_get_invocation: true` in the plugin configuration.\n\nWhen invoked via GET:\n- `params` is an empty object (`{}`)\n- query parameters are passed to the plugin handler via `context.query`\n- wildcard route routing is supported the same way as POST (see `doc_call_plugin`)\n- Use the `route` query parameter for custom routing (recommended; this OpenAPI spec documents query-based routing only).", @@ { "name": "route", "in": "query", "description": "Optional route suffix for custom routing (e.g., '/verify'). Alternative to appending the route to the URL path.", "required": false, + "allowReserved": true, "schema": { - "type": "string" + "type": "string", + "example": "/verify" } } ], @@ "post": { @@ - "description": "Logs and traces are only returned when the plugin is configured with `emit_logs` / `emit_traces`.\nPlugin-provided errors are normalized into a consistent payload (`code`, `details`) and a derived\nmessage so downstream clients receive a stable shape regardless of how the handler threw.\n\nThe endpoint supports wildcard route routing, allowing plugins to implement custom routing logic:\n- `/api/v1/plugins/{plugin_id}/call` - Default endpoint (route = \"\")\n- `/api/v1/plugins/{plugin_id}/call?route=/verify` - Custom route via query parameter\n- `/api/v1/plugins/{plugin_id}/call/verify` - Custom route via URL path (route = \"/verify\")\n\nThe route is passed to the plugin handler via the `context.route` field.\nYou can specify a custom route either by appending it to the URL path or by using the `route` query parameter.", + "description": "Logs and traces are only returned when the plugin is configured with `emit_logs` / `emit_traces`.\nPlugin-provided errors are normalized into a consistent payload (`code`, `details`) and a derived\nmessage so downstream clients receive a stable shape regardless of how the handler threw.\n\nThe endpoint supports wildcard route routing, allowing plugins to implement custom routing logic:\n- `/api/v1/plugins/{plugin_id}/call` - Default endpoint (route = \"\")\n- `/api/v1/plugins/{plugin_id}/call?route=/verify` - Custom route via query parameter\n\nThe route is passed to the plugin handler via the `context.route` field.", @@ { "name": "route", "in": "query", "description": "Optional route suffix for custom routing (e.g., '/verify'). Alternative to appending the route to the URL path.", "required": false, + "allowReserved": true, "schema": { - "type": "string" + "type": "string", + "example": "/verify" } } ],
🧹 Nitpick comments (2)
src/api/routes/plugin.rs (1)
60-74: Logic is correct; consider filteringroutefrom forwarded query params.The precedence logic (path route > query param) is sound. However, when
routeis specified via query parameter, it will also appear in thequeryfield passed to plugins (sinceextract_query_paramsis called separately in the handlers). This could be redundant or confusing for plugin authors.Consider filtering out the
routekey from the query params before passing to plugins, or document this behavior as intentional.openapi.json (1)
4500-4503:forward_logs: document default / presence in responses (and consider sensitivity note).
forward_logsis added as a boolean, but the schema doesn’t indicate whether it’s always present and what the default is. If it’s defaulted server-side, adding adefault(and optionally an explicit “may contain sensitive data; don’t log secrets” note) makes the contract clearer.Proposed diff
"forward_logs": { "type": "boolean", - "description": "Whether to forward plugin logs into the relayer's tracing output" + "description": "Whether to forward plugin logs into the relayer's tracing output", + "default": false },"forward_logs": { "type": "boolean", - "description": "Whether to forward plugin logs into the relayer's tracing output" + "description": "Whether to forward plugin logs into the relayer's tracing output", + "default": false },Please confirm whether
forward_logsis always returned inPluginModelresponses; if yes, consider adding it to the relevantrequiredlists to match runtime behavior.Also applies to: 6991-6994
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
openapi.jsonsrc/api/routes/docs/plugin_docs.rssrc/api/routes/plugin.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust_standards.mdc)
**/*.rs: Follow official Rust style guidelines using rustfmt (edition = 2021, max_width = 100)
Follow Rust naming conventions: snake_case for functions/variables, PascalCase for types, SCREAMING_SNAKE_CASE for constants and statics
Order imports alphabetically and group by: std, external crates, local crates
Include relevant doc comments (///) on public functions, structs, and modules. Use comments for 'why', not 'what'. Avoid redundant doc comments
Document lifetime parameters when they're not obvious
Avoid unsafe code unless absolutely necessary; justify its use in comments
Write idiomatic Rust: Prefer Result over panic, use ? operator for error propagation
Avoid unwrap; handle errors explicitly with Result and custom Error types for all async operations
Prefer header imports over function-level imports of dependencies
Prefer borrowing over cloning when possible
Use &str instead of &String for function parameters
Use &[T] instead of &Vec for function parameters
Avoid unnecessary .clone() calls
Use Vec::with_capacity() when size is known in advance
Use explicit lifetimes only when necessary; infer where possible
Always use Tokio runtime for async code. Await futures eagerly; avoid blocking calls in async contexts
Streams: Use futures::StreamExt for processing streams efficiently
Always use serde for JSON serialization/deserialization with #[derive(Serialize, Deserialize)]
Use type aliases for complex types to improve readability
Implement common traits (Debug, Clone, PartialEq) where appropriate, using derive macros when possible
Implement Display for user-facing types
Prefer defining traits when implementing services to make mocking and testing easier
For tests, prefer existing utils for creating mocks instead of duplicating code
Use assert_eq! and assert! macros appropriately
Keep tests minimal, deterministic, and fast
Use tracing for structured logging, e.g., tracing::info! for request and error logs
When optimizing, prefer clarity first, then performance
M...
Files:
src/api/routes/plugin.rssrc/api/routes/docs/plugin_docs.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: Unit Tests
- GitHub Check: clippy
- GitHub Check: Properties Tests
- GitHub Check: msrv
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: Analyze (rust)
- GitHub Check: semgrep/ci
🔇 Additional comments (6)
src/api/routes/plugin.rs (4)
129-130: LGTM!The handler correctly uses
resolve_routeto determine the effective route from path or query parameter.
150-151: LGTM!Consistent with the POST handler implementation.
225-227: LGTM!Test helpers correctly updated to mirror the production route resolution logic.
931-993: Good test coverage for route resolution.These tests effectively validate:
- Route extraction from query parameter when path route is empty
- Path route takes precedence over query parameter
This covers the key behaviors of the new
resolve_routefunction.src/api/routes/docs/plugin_docs.rs (2)
14-31: Documentation accurately reflects the new routing behavior.The documentation clearly explains both methods:
- Query parameter:
?route=/verify- URL path suffix:
/call/verifyThe path change from wildcard to base path with optional query param is cleaner for OpenAPI consumers while maintaining full backwards compatibility in the implementation.
132-144: LGTM!GET endpoint documentation is consistent with the POST endpoint, correctly documenting both routing methods.
Codecov Report❌ Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. @@ Coverage Diff @@
## main #607 +/- ##
==========================================
- Coverage 91.31% 91.30% -0.02%
==========================================
Files 257 257
Lines 91015 91028 +13
==========================================
- Hits 83111 83110 -1
- Misses 7904 7918 +14
🚀 New features to boost your workflow:
|
Summary
This PR will fix recently merged change that causes sdk callPlugin breaking change by removing route param from api docs.
SDK PR: OpenZeppelin/openzeppelin-relayer-sdk#240
Testing Process
Checklist
Note
If you are using Relayer in your stack, consider adding your team or organization to our list of Relayer Users in the Wild!
Summary by CodeRabbit
Release Notes
New Features
forward_logsconfiguration field to control plugin log outputChanges
✏️ Tip: You can customize this high-level summary in your review settings.