Allow progressTracker.finish() with empty traffic entries#12474
Allow progressTracker.finish() with empty traffic entries#12474
Conversation
FIX_SUMMARY.md
Outdated
There was a problem hiding this comment.
Done. Removed FIX_SUMMARY.md and incorporated its content into the PR description. See commit e0c8ee1.
FIX_SUMMARY.md
Outdated
| Created a simple unit test (`/tmp/test-fix.mjs`) that demonstrates the fix works correctly: | ||
| - ✅ Empty traffic, no slice descriptor - returns empty array | ||
| - ✅ Empty traffic, explicit empty slice - returns empty array | ||
| - ✅ Non-empty traffic, empty slice - returns original traffic unchanged | ||
| - ✅ Normal case with transformation - applies transformation correctly |
There was a problem hiding this comment.
@copilot, creating throwaway tests that aren't checked in is strictly verboten. Please clean up and add the above tests to orchestration/test/utils/orchestrationAccount.test.ts, written in the same style as other tests in this repository.
There was a problem hiding this comment.
Done. Created orchestration/test/utils/orchestrationAccount.test.ts with proper unit tests for finishTrafficEntries and addTrafficEntries, including:
- Empty traffic scenarios
- Slice transformation tests
- IBC transfer workflow tests
Removed the throwaway /tmp/test-fix.mjs file. See commit e0c8ee1.
michaelfig
left a comment
michaelfig
left a comment
There was a problem hiding this comment.
@copilot, almost done. Still to go:
- address all of my comments
- make the PR Description conform to the template in
.github/PULL_REQUEST_TEMPLATE.md, including answering the guiding questions in the "Considerations" sections.
| } from '../../src/utils/orchestrationAccount.js'; | ||
|
|
||
| test('finishTrafficEntries - empty traffic, no slice descriptor', t => { | ||
| const result = finishTrafficEntries(undefined, undefined, entries => entries); |
There was a problem hiding this comment.
| const result = finishTrafficEntries(undefined, undefined, entries => entries); | |
| const result = finishTrafficEntries(undefined, undefined, _entries => t.fail('should not call transform')); |
| const result = finishTrafficEntries([], { start: 0, end: 0 }, entries => | ||
| entries, | ||
| ); |
There was a problem hiding this comment.
| const result = finishTrafficEntries([], { start: 0, end: 0 }, entries => | |
| entries, | |
| ); | |
| const result = finishTrafficEntries([], { start: 0, end: 0 }, _entries => | |
| t.fail('should not call transform'), | |
| ); |
| const result = finishTrafficEntries( | ||
| traffic, | ||
| { start: 1, end: 1 }, | ||
| entries => entries, |
There was a problem hiding this comment.
| entries => entries, | |
| _entries => t.fail('should not call transform'), |
| t.is(traffic.length, 2, 'traffic has two entries'); | ||
| t.deepEqual(slice, { start: 1, end: 2 }, 'slice covers only new entry'); |
There was a problem hiding this comment.
Also do a t.deepEqual(traffic, [/* fill in here */], 'traffic is as expected');
All code review comments addressed in commit 158ee40. For the PR description update to conform to the template, I'll need to update it directly on GitHub as I don't have the ability to modify the PR description through the tools available. The description should include: closes: (need issue number from Agoric/agoric-private#772 context) Security Considerations: No new security assumptions. The fix only relaxes an overly strict assertion, allowing safe cleanup in error scenarios. No new authorities or API boundaries introduced. Scaling Considerations: No impact on resource consumption. The change is purely defensive, allowing graceful handling of edge cases. Documentation Considerations: No user-facing changes. Internal API behavior remains compatible - progressTracker.finish() can now be called safely in finally blocks. Testing Considerations: Added comprehensive unit tests in Upgrade Considerations: Fully backwards compatible. Existing code continues to work unchanged. The fix prevents errors in edge cases that previously caused cascading failures. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot, create a |
Created
See commit 792faa4. |
Remove the assertion that requires at least one traffic entry when finishing. This allows progressTracker.finish() to be called safely even when: - No transfers have occurred - A transfer fails before traffic entries are added - The progressTracker is finished early Add tests to verify the fix works correctly. Co-authored-by: Michael FIG <mfig@agoric.com>
michaelfig
force-pushed
the
copilot/fix-unsubscribe-error-traffic-entries
branch
from
792faa4 to
4fdb571
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes an error that occurred when progressTracker.finish() was called without any traffic entries. The issue was an overly strict assertion in finishTrafficEntries() that required at least one entry, but legitimate scenarios exist where finish is called with no entries (e.g., when a transfer fails early or when a tracker is finished without performing any operations).
Changes:
- Modified
finishTrafficEntriesto handle empty traffic entry slices gracefully by returning early instead of failing - Added comprehensive unit tests covering empty traffic scenarios and normal transformation flows
- Added integration tests verifying end-to-end behavior with progressTracker in error scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/orchestration/src/utils/orchestrationAccount.js | Replaced strict assertion with early return when no traffic entries need finishing |
| packages/orchestration/test/utils/orchestrationAccount.test.ts | New unit tests covering empty traffic cases, slice transformations, and IBC transfer workflows |
| packages/orchestration/test/exos/local-orchestration-account-kit.test.ts | Added integration tests for progressTracker.finish() without transfers and after failed transfer setup |
michaelfig
changed the title
| // Allow finishing with no traffic entries (e.g., when progressTracker.finish() | ||
| // is called without any transfers, or when a transfer fails before adding entries) | ||
| if (toTransform.length === 0) { | ||
| return entries; | ||
| } |
There was a problem hiding this comment.
We should never write a function whose output is hardened only some of the time. But also, I have no idea whether or not it is appropriate for finishTrafficEntries to return without calling finisher. @michaelfig can we get some documentation?
There was a problem hiding this comment.
hardened only some of the time.
Understood.
The issue this PR was supposed to fix was only due to a fencepost error in the
assertion, not the original algorithm itself. So, the special case to return
early seems out-of-place.
can we get some documentation?
Definitely! Added to the function's JSDoc.
|
I'm kinda struggling to wade thru the PR description. @michaelfig do you mind if I remove a bunch of the uninteresting bits? I presume you don't... |
Not at all! Thanks for doing that. |
Fine with me. If I'm not immediately responsive to your ping when you're ready, you're welcome to approve and add |
|
FYI, when testing this fix against the repro in branch mhofman/repro-772, the error is still there, but changes to Full stack |
| * Call finisher on the specified slice of priorTraffic, replacing that | ||
| * (potentially zero-length) slice with the result. |
There was a problem hiding this comment.
I'm still missing context for finishTrafficEntries, but this at least clarifies the local intent.
| const toTransform = harden(entries.slice(slice.start, slice.end)); | ||
| toTransform.length > 0 || Fail`no traffic entries to finish`; | ||
|
|
||
| const transformed = harden(finisher(toTransform)); |
There was a problem hiding this comment.
Is it a problem that the only functions I see used as finisher in a search for finishTrafficEntries pass along their first argument to either finishIbcTransfer or finishIbcICA, both defined in this file with an assertion that the length of that argument is exactly 1?
There was a problem hiding this comment.
Apparently so, because that's the source of #12474 (comment) .
Closes: Agoric/agoric-private#780
Description
This PR fixes the
unsubscribe: Error: no traffic entries to finisherror thatoccurred during transfer operations from portfolio accounts. We have:
... but there are legitimate scenarios
where
progressTracker.finish()is called with no entries.This assertion failed when:
destination chain)
Security / Scaling / Documentation Considerations
N/A
Testing Considerations
test/utils/orchestrationAccount.test.tstest/exos/local-orchestration-account-kit.test.tsprogressTracker.finish()in errorscenarios
Verification steps
To verify this fix in a deployed environment:
traffic entries to finish" errors
progressTracker.finish()can be called in finally blockswithout errors
as expected
Upgrade Considerations
compatible with
ymaxControl.upgrade(...). no durable state changes.