-
Notifications
You must be signed in to change notification settings - Fork 1
New Worker Design #44
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
ci: keep single Claude review comment
Claude was using `gh api` instead of `gh pr comment` to post reviews, but `gh api` is not in the allowed tools list, causing the comment to fail silently. Changes: - Add explicit warning to use `gh pr comment` and not `gh api` - Clarify the command format with a proper code block Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Claude was using `gh api` instead of `gh pr comment` to post reviews, but `gh api` is not in the allowed tools list, causing the comment to fail silently. Changes: - Add explicit warning to use `gh pr comment` and not `gh api` - Clarify the command format with a proper code block Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Pull Request Review: New Worker Design📊 OverviewThis PR introduces a significant architectural upgrade, transitioning from a single interval-based worker to a dual-worker system (SchedulerHandler + WorkerHandler) that enables parallel request processing with crash recovery. The changes are well-documented and thoughtfully designed. Summary:
🎯 Major Strengths1. Excellent Architecture DesignThe dual-worker pattern is a clean solution:
2. Robust Crash RecoveryThe panic detection mechanism in
3. Proper Use of Blockchain AtomicityThe code correctly leverages transaction atomicity: FlowYieldVaultsEVMWorkerOps.scheduledRequests.remove(key: requestId)
processResult = worker.processRequest(request) // if this panics, removal reverts tooThis pattern is safe and correct per blockchain execution model. 4. Preprocessing ValidationEarly validation in
|
…rkflow ci: restore Claude PR commenting and enforce sticky output test: trigger claude review on navid branch
5f6fdbe to
ed3c0aa
Compare
| let future = getCurrentBlock().timestamp + 1.0 | ||
|
|
||
| // Estimate fees and withdraw payment | ||
| let estimate = FlowTransactionScheduler.estimate( |
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.
estimate call can be replaced by a call to cheaper calculateFee, same is possible for other estimate calls here.
https://github.com/onflow/flow-core-contracts/blob/2cbc73095e4a2f2d777190e939b813c704f69897/contracts/FlowTransactionScheduler.cdc#L1465
Currently the flow cli has an older version of FlowTransactionScheduler without that function, but it can be copied:
https://github.com/onflow/flow-core-contracts/blob/2cbc73095e4a2f2d777190e939b813c704f69897/contracts/FlowTransactionScheduler.cdc#L699
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 suggestion! I didn't know about it
To verify my understanding I would pass dataSizeInMB:
0for whendataisnil0.000032for when I passrequestId (UInt256)because it's 32 bytes
Or is it calculated differently?
🚀 Dual-Worker Architecture
📋 Summary
This PR migrates the FlowYieldVaultsEVM worker design from a single interval-based worker that processed one request at a time to a dual-worker architecture. The new system enables parallel processing of multiple requests with automatic scheduling and crash recovery.
🔄 What Changed
Before
After
maxProcessingRequests(default: 3) requests processed concurrently✨ New Features
maxProcessingRequests)maxProcessingRequests: 3)🏗️ Architecture Changes
Before: Single Worker
After: Dual-Worker System
Request Processing Flow:
FlowYieldVaultsRequestsescrows funds (status: PENDING)maxProcessingRequests - current_in_flightpreprocessRequests()to validate and transition PENDING → PROCESSINGprocessRequest()completeProcessing()to mark COMPLETED/FAILED on EVMscheduledRequeststracking🧪 Testing
New Test Suite:
run_worker_tests.shComprehensive E2E tests covering:
maxProcessingRequestsrespected)Run Tests:
./local/setup_and_run_emulator.sh && ./local/deploy_full_stack.sh ./local/run_worker_tests.sh./local/run_worker_tests.sh.📖 Documentation Updates
✅ Issues Resolved