Conversation
WalkthroughThe PR introduces configurable timeout settings for transaction submissions by adding Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/submit-timeout.test.ts (1)
25-93: Consider adding tests for config-based timeout overrides.The existing tests verify default timeout behavior. Consider adding test cases that pass the new
configparameter with customglobalTimeoutMsandpollingTimeoutMsvalues to ensure the override mechanism works as expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/submit-timeout.test.ts` around lines 25 - 93, Add unit tests that exercise the config-based timeout overrides for submitWithFeeBumpAndWait: update or add tests using makeMocks() to call submitWithFeeBumpAndWait with a third/fifth (as appropriate) config argument containing custom globalTimeoutMs and pollingTimeoutMs values, then assert transactionWait was called with opts.timeout reflecting pollingTimeoutMs (capped by the computed remaining time) and that the immediate-reject cases respect a reduced globalTimeoutMs (i.e., when elapsed >= custom globalTimeoutMs - TIMEOUT.BUFFER_MS it throws WAIT_TIMEOUT and transactionWait is not called); reference submitWithFeeBumpAndWait, makeMocks, TIMEOUT and POLLING constants to locate where to pass and validate these overrides.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/submit-timeout.test.ts`:
- Around line 25-93: Add unit tests that exercise the config-based timeout
overrides for submitWithFeeBumpAndWait: update or add tests using makeMocks() to
call submitWithFeeBumpAndWait with a third/fifth (as appropriate) config
argument containing custom globalTimeoutMs and pollingTimeoutMs values, then
assert transactionWait was called with opts.timeout reflecting pollingTimeoutMs
(capped by the computed remaining time) and that the immediate-reject cases
respect a reduced globalTimeoutMs (i.e., when elapsed >= custom globalTimeoutMs
- TIMEOUT.BUFFER_MS it throws WAIT_TIMEOUT and transactionWait is not called);
reference submitWithFeeBumpAndWait, makeMocks, TIMEOUT and POLLING constants to
locate where to pass and validate these overrides.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32ea66d7-7b37-45de-a0bb-e0af0a0a72c6
⛔ Files ignored due to path filters (1)
.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (5)
src/plugin/config.tssrc/plugin/constants.tssrc/plugin/handler.tssrc/plugin/submit.tstest/submit-timeout.test.ts
There was a problem hiding this comment.
Pull request overview
This PR makes the plugin’s global submission timeout budget and transaction polling timeout configurable via the loaded plugin config (with defaults preserved), and updates the timeout-related unit test accordingly.
Changes:
- Rename timeout constants to
DEFAULT_*and thread configurable timeout values through config. - Update
submitWithFeeBumpAndWaitto compute timeouts using config overrides when provided. - Update handler to pass
ctx.configthrough to submission/wait logic.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/submit-timeout.test.ts | Updates assertions to match renamed default timeout constants. |
| src/plugin/submit.ts | Adds optional config overrides for global/polling timeouts when computing transactionWait timeout. |
| src/plugin/handler.ts | Passes loaded config into submitWithFeeBumpAndWait. |
| src/plugin/constants.ts | Renames timeout constants to DEFAULT_GLOBAL_TIMEOUT_MS / DEFAULT_TIMEOUT_MS. |
| src/plugin/config.ts | Adds env parsing + config fields for globalTimeoutMs and pollingTimeoutMs. |
| .DS_Store | Adds an OS metadata file (should not be committed). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| PluginAPI, | ||
| } from '@openzeppelin/relayer-sdk'; | ||
| import { HTTP_STATUS, TIMEOUT, POLLING } from './constants'; | ||
| import { ChannelAccountsConfig } from './config'; |
There was a problem hiding this comment.
ChannelAccountsConfig is only used as a type here. Use a type-only import (import type) to avoid emitting an unnecessary runtime dependency and to match the existing pattern in this codebase (e.g. handler.ts / management.ts use import type { ChannelAccountsConfig }).
| import { ChannelAccountsConfig } from './config'; | |
| import type { ChannelAccountsConfig } from './config'; |
| const globalTimeout = config?.globalTimeoutMs ?? TIMEOUT.DEFAULT_GLOBAL_TIMEOUT_MS; | ||
| const pollingTimeout = config?.pollingTimeoutMs ?? POLLING.DEFAULT_TIMEOUT_MS; | ||
| const elapsedMs = Date.now() - startTime; | ||
| const remainingMs = TIMEOUT.GLOBAL_TIMEOUT_MS - TIMEOUT.BUFFER_MS - elapsedMs; | ||
| const remainingMs = globalTimeout - TIMEOUT.BUFFER_MS - elapsedMs; |
There was a problem hiding this comment.
New behavior: globalTimeoutMs / pollingTimeoutMs can override the default timeouts, but test/submit-timeout.test.ts only covers the default path. Add a test that passes a custom config to submitWithFeeBumpAndWait and asserts the computed transactionWait timeout reflects those overrides.
| function parseGlobalTimeoutMs(): number { | ||
| const raw = process.env.PLUGIN_GLOBAL_TIMEOUT_MS; | ||
| if (!raw) return TIMEOUT.DEFAULT_GLOBAL_TIMEOUT_MS; | ||
| const n = Number(raw); | ||
| return Number.isFinite(n) && n > 0 ? Math.floor(n) : TIMEOUT.DEFAULT_GLOBAL_TIMEOUT_MS; | ||
| } | ||
|
|
||
| function parsePollingTimeoutMs(): number { | ||
| const raw = process.env.PLUGIN_POLLING_TIMEOUT_MS; | ||
| if (!raw) return POLLING.DEFAULT_TIMEOUT_MS; | ||
| const n = Number(raw); | ||
| return Number.isFinite(n) && n > 0 ? Math.floor(n) : POLLING.DEFAULT_TIMEOUT_MS; | ||
| } |
There was a problem hiding this comment.
PLUGIN_GLOBAL_TIMEOUT_MS / PLUGIN_POLLING_TIMEOUT_MS parsing is new and isn’t covered by existing test/config.test.ts cases. Add tests for defaulting, valid numeric values (including flooring), and invalid/non-positive values falling back to defaults.
zeljkoX
left a comment
There was a problem hiding this comment.
LGTM
Seems like few tests missing and commited .DS_Store
Summary by CodeRabbit
PLUGIN_GLOBAL_TIMEOUT_MSandPLUGIN_POLLING_TIMEOUT_MSenvironment variables.