feat: Add key-value storage support for plugins #463
Conversation
Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
WalkthroughMigrates plugin handlers to a single-parameter PluginContext ({ api, params, kv }), adds a per-plugin Redis-backed KV store with locking, updates TypeScript CLI/loader to accept pluginId and wire KV into plugin execution, adds KV examples/tests, and propagates plugin_id through Rust runner and script executor APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Service as PluginService
participant Runner as PluginRunner
participant Exec as ScriptExecutor
participant CLI as TS Plugin CLI
participant User as User Plugin
participant KV as PluginKVStore (Redis)
Service->>Runner: run(plugin_id, socket_path, script_path, timeout, params)
Runner->>Exec: execute_typescript(plugin_id, script_path, socket_path, params)
Exec->>CLI: spawn ts-node with argv (pluginId, params, scriptPath)
CLI->>KV: DefaultPluginKVStore(pluginId).connect()
CLI->>User: invoke handler({ api, params, kv })
alt Modern context handler
User->>KV: kv.get/set/withLock/scan/...
User-->>CLI: handler result
else Legacy 2‑arg handler
User-->>CLI: handler result
end
CLI->>KV: disconnect()
CLI-->>Exec: ScriptResult
Exec-->>Runner: ScriptResult
Runner-->>Service: ScriptResult
note right of KV: Keys are namespaced per plugin_id (plugin_kv:{pluginId}:...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.
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 (2)
plugins/lib/executor.ts (1)
84-101: Bug: process exits with code 0 even on failures.You call process.exit(1) in catch and process.exit(0) in finally. finally runs regardless, overwriting the failure exit code.
Apply this diff to set the exit code once:
async function main(): Promise<void> { const logInterceptor = new LogInterceptor(); + let exitCode = 0; try { // Start intercepting all console output at the executor level // This provides better backward compatibility with existing scripts logInterceptor.start(); // Extract and validate CLI arguments including plugin ID const { socketPath, pluginId, paramsJson, userScriptPath } = extractCliArguments(); // Parse plugin parameters const pluginParams = parsePluginParameters(paramsJson); // Pass plugin ID as separate argument const result = await runUserPlugin(socketPath, pluginId, pluginParams, userScriptPath); // Add the result to LogInterceptor output logInterceptor.addResult(serializeResult(result)); } catch (error) { process.stderr.write(`Plugin executor failed: ${error instanceof Error ? error.message : error}\n`); - process.exit(1); + exitCode = 1; } finally { logInterceptor.stop(); - process.exit(0); + process.exit(exitCode); } }src/services/plugins/script_executor.rs (1)
42-46: Validate and sanitize plugin_id before passing it to the executorVerification: search shows no validation; plugin_id is accepted as web::Path and forwarded unchanged.
- Enforce a whitelist (allowed characters) and max length at the API boundary: src/api/routes/plugin.rs.
- Sanitize/escape plugin_id before using it as a KV namespace or in any path/file context: src/services/plugins/script_executor.rs (function around lines 42–46).
- Add unit/integration tests and update OpenAPI docs accordingly: src/api/routes/docs/plugin_docs.rs.
🧹 Nitpick comments (19)
plugins/tsconfig.json (1)
36-36: Scope Jest types to tests to avoid polluting prod type space.Having "jest" in root "types" makes Jest globals available everywhere. Prefer a dedicated tsconfig for tests or use "ts-jest" config to inject types only in tests.
Example: create plugins/tsconfig.test.json extending this file with
"types": ["node", "jest"]and point ts-jest to it.plugins/package.json (1)
19-27: Double‑check ioredis and mock feature coverage; consider small hygiene tweaks.
- Ensure ioredis‑mock supports UNLINK, EVAL, PX+NX used by the KV (tests depend on it). If not, pin a mock version that does or polyfill in tests.
- Consider moving "@types/node" to devDependencies.
- Add "license" and "author" to avoid publish warnings.
- Optionally set "engines": { "node": ">=18" } to match ioredis v5 expectations.
plugins/lib/executor.ts (1)
36-41: Validate pluginId format early.Reject pluginId with whitespace or wildcard chars to avoid odd connection names and log noise. Keep it aligned with KV key regex.
Example:
if (!/^[A-Za-z0-9:_-]{1,128}$/.test(pluginId)) throw new Error("Invalid plugin ID");plugins/lib/kv.ts (4)
33-40: Harden Redis client options (timeouts/backoff).Add explicit connect timeout and a slightly less aggressive retry strategy to reduce noisy failures and quick thrashing on transient outages.
this.client = new IORedis(url, { connectionName: `plugin_kv:${pluginId}`, lazyConnect: true, enableOfflineQueue: false, enableAutoPipelining: true, maxRetriesPerRequest: 1, - retryStrategy: (n) => Math.min(n * 50, 1000), + connectTimeout: 5000, + retryStrategy: (n) => Math.min(n * 100, 2000), });
58-63: Key validation is strict; document or widen.Regex forbids dots and slashes. If users expect keys like user.profile or path-like keys, consider allowing '.' and '/' or document the constraint prominently.
101-124: clear() also deletes lock keys; consider safety guard.If clear() is run while a lock is held, it will unlink the lock, breaking mutual exclusion guarantees. Either document as admin‑only/maintenance, or filter to data keys by default and add an option to include locks.
- async clear(): Promise<number> { + async clear(opts?: { includeLocks?: boolean }): Promise<number> { let cursor = '0'; let deleted = 0; do { - const [next, keys] = await this.client.scan(cursor, 'MATCH', `${this.ns}:*`, 'COUNT', 1000); + const seg = opts?.includeLocks ? '*' : 'data:*'; + const [next, keys] = await this.client.scan(cursor, 'MATCH', `${this.ns}:${seg}`, 'COUNT', 1000);
126-146: Lock TTL can expire mid‑work; add optional heartbeat or warn.If fn runs longer than ttlSec, the lock can lapse and allow concurrent execution. Provide an optional heartbeat/auto‑extend or at least document the hazard.
I can add an optional keepAliveMs to periodically PEXPIRE the lock until fn completes.
plugins/tests/lib/kv.test.ts (3)
31-41: Reduce flakiness in TTL test.Use fake timers to avoid real sleeps and timing drift in CI.
- // Wait for expiry - await new Promise((resolve) => setTimeout(resolve, 1100)); + jest.useFakeTimers(); + await jest.advanceTimersByTimeAsync(1100); + jest.useRealTimers();
111-138: Assert both branches deterministically.When testing lock contention, also assert that one rejection reason is exactly 'lock busy' and the fulfilled value is as expected to prevent false positives.
168-191: Ensure secondary KV instances are cleaned up on failure.The try/finally handles cleanup, but if kv1.connect() succeeds and kv2.connect() throws, kv1 may remain connected. Consider connecting both first, then operating.
plugins/examples/kv-storage.ts (3)
27-32: Add input validation for the value parameter in set actionThe
setaction doesn't validate thevalueparameter. While the KV store implementation checks forundefined, consider validating at the handler level for better error messages.case 'set': { const { key, value, ttlSec } = params ?? {}; assertString(key, 'key'); + if (value === undefined) { + throw new Error('value parameter is required for set action'); + } const ok = await kv.set(key, value, { ttlSec: toInt(ttlSec) }); return { ok }; }
56-59: Consider exposing batch size limits in documentationThe
scanoperation uses a default batch size of 500, but this isn't documented. Consider adding a comment about reasonable batch size limits to prevent potential memory issues with large key sets.case 'scan': { const { pattern, batch } = params ?? {}; + // Note: batch size defaults to 500, recommended range: 10-1000 const keys = await kv.scan(pattern ?? '*', toInt(batch, 500)); return { keys }; }
125-128: Improve toInt function robustnessThe
toIntfunction silently returns the default for invalid inputs. Consider throwing an error for completely invalid inputs while still supporting the default behavior for missing values.function toInt(v: unknown, def = 0): number { + if (v !== undefined && v !== null && v !== '') { + const n = typeof v === 'string' ? parseInt(v, 10) : typeof v === 'number' ? Math.floor(v) : NaN; + if (!Number.isFinite(n)) { + throw new Error(`Invalid numeric value: ${v}`); + } + return n > 0 ? n : def; + } - const n = typeof v === 'string' ? parseInt(v, 10) : typeof v === 'number' ? Math.floor(v) : def; - return Number.isFinite(n) && n > 0 ? n : def; + return def; }docs/modules/ROOT/pages/plugins.adoc (1)
35-41: Import statement could be more specificThe import shows
Speed, PluginContextbut the example doesn't define what other types might be available. Consider showing the complete import or adding a comment about available exports.[source,typescript] ---- /// Required imports. +/// Speed: Transaction speed enum (FAST, STANDARD, SLOW) +/// PluginContext: Contains api, params, and kv properties import { Speed, PluginContext } from "@openzeppelin/relayer-sdk";plugins/lib/plugin.ts (1)
276-280: Consider adding KV parameter to function documentationThe JSDoc comment doesn't document the
kvparameter that was added to the function signature./** * Helper function that loads and executes a user plugin script * @param userScriptPath - Path to the user's plugin script * @param api - Plugin API instance + * @param kv - KV store instance for persistent plugin state * @param params - Plugin parameters */plugins/README.md (3)
88-88: Consider clarifying the timeout default discrepancy.The documentation states "default timeout of 300 seconds (5 minutes)" but should clarify whether this applies when the timeout field is omitted from the config.
91-91: Inconsistent quote usage in YAML example.The YAML configuration uses single quotes throughout, which is valid but inconsistent with the double quotes used in the curl example below (lines 99-107). Consider using double quotes for consistency with JSON-style formatting.
-{ 'plugins': [{ 'id': 'example', 'path': 'examples/example.ts', 'timeout': 30 }] } +{ "plugins": [{ "id": "example", "path": "examples/example.ts", "timeout": 30 }] }
187-195: Consider documenting error handling for KV operations.The method signatures are clear, but consider adding documentation about potential error conditions (e.g., Redis connection failures, serialization errors) and recommended error handling patterns.
Would you like me to generate comprehensive error handling examples for the KV store operations?
src/services/plugins/runner.rs (1)
187-189: Consider extracting test plugin ID to a constant.Both tests use the hardcoded string "test-plugin". Consider extracting this to a test constant for better maintainability.
+ const TEST_PLUGIN_ID: &str = "test-plugin"; + #[tokio::test] async fn test_run() { // ... setup code ... - let plugin_id = "test-plugin".to_string(); + let plugin_id = TEST_PLUGIN_ID.to_string();And similarly for the timeout test:
- let plugin_id = "test-plugin".to_string(); + let plugin_id = TEST_PLUGIN_ID.to_string();Also applies to: 238-240
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
plugins/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
docs/modules/ROOT/pages/plugins.adoc(13 hunks)plugins/README.md(3 hunks)plugins/examples/kv-storage.ts(1 hunks)plugins/lib/executor.ts(3 hunks)plugins/lib/kv.ts(1 hunks)plugins/lib/plugin.ts(5 hunks)plugins/package.json(1 hunks)plugins/tests/lib/kv.test.ts(1 hunks)plugins/tsconfig.json(1 hunks)src/services/plugins/mod.rs(2 hunks)src/services/plugins/runner.rs(5 hunks)src/services/plugins/script_executor.rs(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
plugins/tests/lib/kv.test.ts (1)
plugins/lib/kv.ts (1)
DefaultPluginKVStore(23-147)
plugins/examples/kv-storage.ts (2)
plugins/lib/plugin.ts (1)
PluginContext(190-194)plugins/lib/kv.ts (2)
key(59-62)exists(85-87)
plugins/lib/plugin.ts (4)
plugins/lib/kv.ts (2)
PluginKVStore(4-21)DefaultPluginKVStore(23-147)plugins/examples/kv-storage.ts (1)
handler(23-123)examples/basic-example-plugin/test-plugin/index.ts (1)
handler(48-116)plugins/examples/example.ts (1)
handler(16-44)
plugins/lib/executor.ts (1)
plugins/lib/plugin.ts (1)
runUserPlugin(493-516)
src/services/plugins/runner.rs (2)
src/services/plugins/socket.rs (1)
socket_path(94-96)src/services/plugins/script_executor.rs (1)
execute_typescript(41-89)
⏰ 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). (8)
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: msrv
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: Analyze (rust)
🔇 Additional comments (17)
src/services/plugins/script_executor.rs (1)
65-69: LGTM! Correct parameter ordering for KV support.The updated argument ordering correctly passes
plugin_idto the executor, enabling per-plugin KV namespacing as designed.src/services/plugins/mod.rs (1)
116-123: LGTM! Correctly propagates plugin_id for KV namespacing.The implementation properly passes
plugin.id.clone()as the first parameter to the runner, enabling per-plugin KV store isolation.docs/modules/ROOT/pages/plugins.adoc (2)
97-100: Clear deprecation notice for legacy patternsGood job providing a clear deprecation warning that explains the migration path and the limitation (no KV access) of legacy patterns.
379-434: Comprehensive KV documentation with good examplesThe KV storage section provides excellent documentation covering configuration, usage patterns, available methods, and practical examples. The automatic namespacing per plugin ID prevents collisions effectively.
plugins/lib/plugin.ts (3)
189-194: Well-designed PluginContext interfaceThe
PluginContextinterface cleanly encapsulates all plugin requirements - API access, parameters, and KV storage - in a single, extensible structure.
315-324: Smart handler signature detection for backward compatibilityExcellent implementation using
handler.lengthto detect the signature pattern. This allows seamless migration while maintaining backward compatibility - modern single-parameter handlers get KV access while legacy two-parameter handlers continue to work without it.
499-515: Confirm kv.disconnect() is safe when kv.connect() failsplugins/lib/plugin.ts (lines 499–515) calls kv.disconnect() in a finally even if kv.connect() throws; repo search returned no implementation matches for DefaultPluginKVStore.disconnect — verify disconnect is idempotent/no-throw when not connected, or wrap/guard the call (try/catch or connected flag).
plugins/README.md (5)
12-16: LGTM! Clean API migration to PluginContext.The updated imports and type definitions properly demonstrate the new single-parameter pattern with strongly-typed Params.
23-41: Well-structured handler with KV integration.The handler properly destructures the PluginContext and demonstrates the KV store usage. The flow from transaction sending to persistence is clear and follows best practices.
44-68: Clear deprecation notice for legacy patterns.Good documentation of deprecated patterns with explicit warnings about the lack of KV access. This helps developers understand the migration path.
151-185: Comprehensive KV store documentation with practical examples.Excellent documentation of the KV store API with clear examples including atomic operations with locking. The TTL and concurrency patterns are well demonstrated.
172-181: Verify distributed lock behavior across plugin instances.Found: plugins/lib/kv.ts implements the lock with ioredis SET ... NX + PX (UUID token) and an atomic Lua UNLINK; namespace uses plugin_kv:{} (hash tag). Unit tests (plugins/tests/lib/kv.test.ts) use ioredis-mock and only cover single-process/mocked concurrency; docker-compose.yaml starts a single Redis instance.
- Risk: current approach is correct for a single Redis node but can fail in multi-master/cluster HA setups (and there's no lock renewal if fn runs longer than ttl).
- Action items: (1) confirm your deployment model (single Redis vs Redis Cluster/HA); (2) if multi-node/cluster, switch to a proven distributed-lock algorithm/library (e.g., Redlock) or otherwise document that a single Redis is required; (3) consider lock renewal/longer TTL for long critical sections; (4) add an integration test that runs two plugin instances against a real Redis to validate behavior.
src/services/plugins/runner.rs (5)
33-57: LGTM! Trait signature properly updated for plugin ID propagation.The addition of
plugin_idas the first parameter enables proper namespacing for KV storage operations.
62-86: Proper async trait implementation with plugin ID.The implementation correctly adopts the updated trait signature and maintains all generic constraints.
99-104: Correct parameter ordering for ScriptExecutor.The plugin_id is properly passed as the first argument to ScriptExecutor::execute_typescript, aligning with the updated executor signature shown in the relevant code snippets.
187-198: Test properly updated with plugin ID.The test correctly constructs and passes the plugin_id parameter with proper string conversion for paths.
238-245: Consistent test updates for timeout scenario.The timeout test is properly updated with the same plugin_id pattern, maintaining consistency across all test cases.
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
LGTM
Thanks!
Left few comments
Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
plugins/lib/kv.ts (2)
1-21: Good addition: top-level docs.Addresses earlier ask to document the module purpose and guarantees.
70-71: Add a graceful shutdown and avoid connecting when unused.Expose a
close()that quits if connected, otherwisedisconnect()to avoid establishing a connection solely to close it. This also enables the caller to cleanly release resources.@@ export interface PluginKVStore { @@ clear(): Promise<number>; + + /** + * Close the underlying Redis client. No-op if never connected. + */ + close(): Promise<void>; @@ export class DefaultPluginKVStore implements PluginKVStore { @@ private readonly UNLOCK_SCRIPT = 'if redis.call("GET", KEYS[1]) == ARGV[1] then return redis.call("UNLINK", KEYS[1]) else return 0 end'; @@ } @@ } + +// Add at end of class body (just before closing brace) + async close(): Promise<void> { + const status = (this.client as any).status; // 'wait' | 'connecting' | 'connect' | 'ready' | 'close' | 'end' | 'reconnecting' + if (status === 'wait' || status === 'end' || status === 'close') { + this.client.disconnect(); + return; + } + try { + await this.client.quit(); + } catch { + this.client.disconnect(); + } + }Also applies to: 96-103, 277-277
plugins/lib/plugin.ts (1)
499-507: Close the KV client in finally to avoid leaking connections.With
lazyConnect, no connect occurs if KV isn’t used, but when it is, we should close it. Pair this withPluginKVStore.close().export async function runUserPlugin<T = any, R = any>( socketPath: string, pluginId: string, pluginParams: T, userScriptPath: string ): Promise<R> { const plugin = new DefaultPluginAPI(socketPath); const kv = new DefaultPluginKVStore(pluginId); try { const result: R = await loadAndExecutePlugin<T, R>(userScriptPath, plugin, kv, pluginParams); return result; } finally { - plugin.close(); + await kv.close().catch(() => {}); + plugin.close(); } }
🧹 Nitpick comments (4)
plugins/lib/kv.ts (2)
145-149: Optional: guard JSON.parse to surface data corruption clearly.If non‑JSON data is stored (by another client),
JSON.parsewill throw and obscure which key failed. Consider wrapping with a parse try/catch to annotate the key.async get<T = unknown>(key: string): Promise<T | null> { const v = await this.client.get(this.key('data', key)); if (v == null) return null; - return JSON.parse(v) as T; + try { + return JSON.parse(v) as T; + } catch (e) { + throw new Error(`Failed to parse JSON for key "${key}": ${(e as Error).message}`); + } }
151-162: Optional: BigInt values will throw on JSON.stringify.If plugins store BigInt (common in chain data), JSON.stringify throws. Either document JSON‑serializable constraint explicitly or add a replacer (e.g., convert BigInt to string).
Would you like me to add an optional
jsonReplacerto the store and wire a BigInt-to-string default?plugins/examples/kv-storage.ts (1)
55-59: Consider renaming action 'scan' → 'listKeys' for API consistency.The KV API exposes
listKeys; aligning the action name helps discoverability.- case 'scan': { + case 'listKeys': { const { pattern, batch } = params ?? {}; const keys = await kv.listKeys(pattern ?? '*', toInt(batch, 500)); return { keys }; }plugins/lib/plugin.ts (1)
276-281: Dispatch logic is fine; handle both handler styles.Length-based dispatch is pragmatic. Keep in mind arrow funcs with defaulted params can affect
.length, but current heuristic is acceptable.Please ensure tests cover:
- modern handler (context param),
- legacy handler (api, params),
- absence of handler (direct execution).
Also applies to: 315-324
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/examples/kv-storage.ts(1 hunks)plugins/lib/kv.ts(1 hunks)plugins/lib/plugin.ts(5 hunks)plugins/tests/lib/kv.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/tests/lib/kv.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
plugins/examples/kv-storage.ts (2)
plugins/lib/plugin.ts (1)
PluginContext(190-194)plugins/lib/kv.ts (2)
key(133-136)exists(188-190)
plugins/lib/plugin.ts (3)
plugins/lib/kv.ts (2)
PluginKVStore(28-88)DefaultPluginKVStore(96-277)plugins/examples/kv-storage.ts (1)
handler(23-123)examples/basic-example-plugin/test-plugin/index.ts (1)
handler(48-116)
⏰ 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). (9)
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: msrv
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Analyze (rust)
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: semgrep/ci
🔇 Additional comments (2)
plugins/examples/kv-storage.ts (1)
82-121: Example looks solid.Covers set/get with TTL, exists/del, listKeys, and withLock flow.
plugins/lib/plugin.ts (1)
187-194: Good public types: PluginContext and dual-signature handler.Clear separation between legacy and modern styles, and KV exposure only for modern context.
Also applies to: 199-202
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #463 +/- ##
======================================
Coverage 93.0% 93.0%
======================================
Files 224 224
Lines 77164 77434 +270
======================================
+ Hits 71818 72087 +269
- Misses 5346 5347 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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)
src/services/plugins/script_executor.rs (1)
64-76: Add an execution timeout to prevent hung plugin scriptsAn untrusted/buggy plugin can hang indefinitely; add a bounded timeout.
Apply:
- let output = Command::new("ts-node") - .arg(executor_path) // Execute executor script - .arg(socket_path) // Socket path (argv[2]) - .arg(plugin_id) // Plugin ID (argv[3]) - .arg(script_params) // Plugin parameters (argv[4]) - .arg(script_path) // User script path (argv[5]) - .stdin(Stdio::null()) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .output() - .await - .map_err(|e| PluginError::SocketError(format!("Failed to execute script: {}", e)))?; + let output_fut = Command::new("ts-node") + .arg(executor_path) // Execute executor script + .arg(socket_path) // Socket path (argv[2]) + .arg(plugin_id) // Plugin ID (argv[3]) + .arg(script_params) // Plugin parameters (argv[4]) + .arg(script_path) // User script path (argv[5]) + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .output(); + + let output = tokio::time::timeout(std::time::Duration::from_secs(30), output_fut) + .await + .map_err(|_| PluginError::PluginExecutionError("Plugin executor timed out".into()))? + .map_err(|e| PluginError::SocketError(format!("Failed to execute script: {}", e)))?;Add imports:
+use std::time::Duration; +use tokio::time;
🧹 Nitpick comments (5)
src/services/plugins/script_executor.rs (5)
47-56: Tighten the ts-node presence checkCurrent check ignores non‑zero exit status. Consider also validating
status.success()or drop the precheck and rely on spawn failure; cache the result to avoid running it on every call.
95-101: Be tolerant to stray whitespace/CRLF when parsing logsTrim and skip empty lines to avoid false negatives on Windows/CI noise.
Apply:
- for log in logs { - let log: LogEntry = serde_json::from_str(&log).map_err(|e| { + for raw in logs { + let line = raw.trim(); + if line.is_empty() { + continue; + } + let log: LogEntry = serde_json::from_str(line).map_err(|e| { PluginError::PluginExecutionError(format!("Failed to parse log: {}", e)) })?;
134-148: tsconfig in temp dir isn’t used by ts-node here
tsconfig.jsonwritten undertemp_diris ignored because the executor runs from the repo CWD. Either setTS_NODE_PROJECTso ts-node picks it up, or delete the write to avoid confusion.Option A (use it):
fs::write(ts_config.clone(), TS_CONFIG.as_bytes()).unwrap(); + std::env::set_var("TS_NODE_PROJECT", ts_config.display().to_string());(Repeat in other tests if you want consistent behavior.)
149-154: Call site updates for plugin_id look goodAll test invocations pass the new
plugin_idarg in the correct position.Optional: dedupe
"test-plugin-1"via aconst PLUGIN_ID: &strat module scope.Also applies to: 190-196, 221-226, 257-263
232-235: Relax brittle error-string assertionAsserting the exact phrase ties tests to executor wording. Keep the semantic check (non-empty + includes “logger”) for stability.
Apply:
- assert!(!result.error.is_empty()); - assert!(result.error.contains("Plugin executor failed")); - assert!(result.error.contains("logger")); + assert!(!result.error.is_empty()); + assert!(result.error.to_lowercase().contains("logger"));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/services/plugins/script_executor.rs(7 hunks)
⏰ 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). (8)
- GitHub Check: clippy
- GitHub Check: test
- GitHub Check: msrv
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- GitHub Check: Redirect rules - openzeppelin-relayer
- GitHub Check: Header rules - openzeppelin-relayer
- GitHub Check: Pages changed - openzeppelin-relayer
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
src/services/plugins/script_executor.rs (2)
67-70: Arg order matches executor.ts — approvedConfirmed: executor.ts maps argv[2]=socketPath, argv[3]=pluginId, argv[4]=paramsJson, argv[5]=userScriptPath; the Rust invocation passes plugin_id, script_params, script_path in the same order. Keep the regression check.
41-46: Validate plugin_id (non-empty) to prevent silent misroutingGuard against empty/whitespace-only IDs before invoking the executor.
impl ScriptExecutor { pub async fn execute_typescript( plugin_id: String, script_path: String, socket_path: String, script_params: String, ) -> Result<ScriptResult, PluginError> { + if plugin_id.trim().is_empty() { + return Err(PluginError::PluginExecutionError( + "plugin_id must not be empty".into(), + )); + }Ran ast-grep to find legacy 3‑arg calls; it produced no output — manual verification required to confirm there are no remaining 3‑arg call sites.
zeljkoX
left a comment
zeljkoX
left a comment
There was a problem hiding this comment.
LGTM
Thanks
Seems like minor leftovers for scan -> listKeys references in docs
plugins/README.md
Outdated
| - `set(key: string, value: unknown, opts?: { ttlSec?: number }): Promise<boolean>` | ||
| - `del(key: string): Promise<boolean>` | ||
| - `exists(key: string): Promise<boolean>` | ||
| - `scan(pattern?: string, batch?: number): Promise<string[]>` |
There was a problem hiding this comment.
let's rename to listKeys and check if some other doc reference or example needs to be updated.
Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/README.md (1)
189-196: Method name consistency with prior feedbackThe list uses listKeys — good. Elsewhere (AsciiDoc mock) still shows scan. Please update remaining references to listKeys.
🧹 Nitpick comments (8)
plugins/README.md (4)
86-92: Config snippet format: use valid JSON (not YAML-with-JSON-inline)The fence is yaml but the example is JSON-like with single quotes. Suggest switching to a proper JSON block for consistency with docs.
Apply this diff:
-```yaml -{ 'plugins': [{ 'id': 'example', 'path': 'examples/example.ts', 'timeout': 30 }] } -``` +```json +{ + "plugins": [ + { "id": "example", "path": "examples/example.ts", "timeout": 30 } + ] +} +```
118-149: Inconsistent response fields vs. earlier example
- return_value shows a string ("done!") but the main example returns { success, transactionId }.
- traces field key uses relayerId here, while the AsciiDoc page uses relayer_id.
- value type is a number here (1) but a string elsewhere (“1000000000000000”).
Please align examples to the new handler shape and use one canonical casing and type for amounts (recommend string).
Apply this diff to align with the modern example:
- "return_value": "\"done!\"", + "return_value": { "success": true, "transactionId": "tx-123456" }, ... - "value": 1 + "value": "1"Also confirm whether traces should use relayerId or relayer_id and update both docs consistently.
49-58: Legacy import path clarityThis imports runPlugin from a relative ../lib/plugin path. For external users, confirm whether runPlugin is publicly exported from @openzeppelin/relayer-sdk; if not, call out that this path is internal-only and not recommended for new code.
171-181: Document withLock default behaviorwithLock documents onBusy but not the default (throw or skip). Please state the default and whether the function returns null on skip.
docs/modules/ROOT/pages/plugins.adoc (4)
166-197: KV mock interface drift: replace scan with listKeys; remove/connect disconnect or document
- Rename scan to listKeys to match the published API.
- connect/disconnect aren’t documented methods; either document them in Available Methods or drop from the mock.
Apply this diff:
kv: { set: async () => true, get: async () => null, del: async () => true, exists: async () => false, - scan: async () => [], + listKeys: async () => [], clear: async () => 0, withLock: async (_k: string, fn: () => Promise<any>) => fn(), - connect: async () => {}, - disconnect: async () => {} }
324-370: traces field naming: relayer_id vs relayerIdThis block uses relayer_id, while README uses relayerId. Please pick one canonical casing and update all examples.
Apply this diff if camelCase is canonical:
- "relayer_id": "my-relayer", + "relayerId": "my-relayer",
380-433: KV section is solid; add listKeys usage snippet and clarify withLock defaults
- Add a short example showing listKeys(pattern) usage.
- State the default for withLock onBusy and its return semantics on skip.
Example to append under Usage:
// List keys under a prefix const keys = await kv.listKeys('counter:*');
433-434: Key format constraint: cite or verifyYou state keys must match [A-Za-z0-9:_-]{1,512}. Please confirm this is enforced by the implementation and reference it, or soften the language to “should”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/modules/ROOT/pages/plugins.adoc(13 hunks)plugins/README.md(3 hunks)plugins/lib/kv.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/lib/kv.ts
⏰ 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). (9)
- GitHub Check: test
- GitHub Check: clippy
- GitHub Check: msrv
- GitHub Check: boostsecurity - boostsecurityio/semgrep-pro
- 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 (3)
plugins/README.md (1)
23-41: Good shift to single-parameter PluginContext handlerExample is clear, uses context.kv, and waits on the tx. LGTM.
docs/modules/ROOT/pages/plugins.adoc (2)
35-41: Modern handler/API imports look goodClear guidance on single-context handler with PluginContext. LGTM.
234-290: End-to-end example reads wellGood parameter validation, structured result, and KV persistence. LGTM.
dylankilkenny
deleted the
plat-6929-add-support-for-kv-storage-to-plugins
branch
Summary
Testing Process
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests
Chores