fix(PE-8643): setup patch for balances and primary names in hb patch system #442
Conversation
…stem' into PE-8643-hb-patch-system
WalkthroughAdds a global Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Handler as Message Handler
participant Main as main.lua
participant HB as hb.lua
participant HS as HyperbeamSync
Handler->>Main: Execute handler (mutates state)
Main->>HS: Set HyperbeamSync flags/entries
Main->>Main: shouldPatchHbState = true
Note over Main,HB: After handler completes (not "continue")
Main->>HB: hb.patchHyperbeamState()
HB->>HB: createPrimaryNamesPatch()
alt primary-names patch exists
HB->>HB: send primary-names patch message
end
HB->>HB: createBalancesPatch()
alt balances patch exists
HB->>HB: send balances patch message
end
HB->>HB: resetHyperbeamSync()
HB->>HS: Clear tracking state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #442 +/- ##
========================================
Coverage 91.36% 91.36%
========================================
Files 10 10
Lines 3218 3230 +12
========================================
+ Hits 2940 2951 +11
- Misses 278 279 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| --- For simplicity we always include the protocol balance in the patch message | ||
| --- this also prevents us from sending an empty patch message and deleting the entire hyperbeam balances table\ | ||
| --- this also prevents us from sending an empty patch message and deleting the entire hyperbeam balances table |
There was a problem hiding this comment.
I think we have enough experience with the system now to stop doing this. Let's just make sure we never send an empty balances list unless there's truly no balances.
|
|
||
| local balancesPatch = hb.createBalancesPatch() | ||
| if balancesPatch then | ||
| patchMessageFields["balances"] = balancesPatch |
There was a problem hiding this comment.
TBD: One patch message or multiple. Wait until we get confirmation from Sam.
src/hb.lua
Outdated
|
|
||
| local balancesPatch = hb.createBalancesPatch() | ||
| if balancesPatch then | ||
| patchMessageFields["balances"] = balancesPatch |
There was a problem hiding this comment.
TBD: Whether or not to send patch messages together in one message.
atticusofsparta
changed the title
atticusofsparta
changed the base branch from
PE-8643-hb-patch-system
to
develop
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
tests/tick.test.mjs (1)
372-387: Make assertions order‑agnostic while keeping the count at 3Relying on fixed indices for Messages is brittle. Keep the length check, but locate notices by Action to avoid future flakes.
- // Note: Balance patch is no longer sent if only protocol balance changes - assert.equal(newEpochTick.Messages.length, 3); - // updated demand factor is sent - assert.equal( - newEpochTick.Messages[0].Tags.find((tag) => tag.name === 'Action').value, - 'Demand-Factor-Updated-Notice', - ); - // epoch created notice is sent - assert.equal( - newEpochTick.Messages[1].Tags.find((tag) => tag.name === 'Action').value, - 'Epoch-Created-Notice', - ); - assert.equal( - newEpochTick.Messages[2].Tags.find((tag) => tag.name === 'Action').value, - 'Tick-Notice', - ); + // Note: Balance patch is no longer sent if only protocol balance changes + assert.equal(newEpochTick.Messages.length, 3); + const byAction = (a) => + newEpochTick.Messages.find((m) => + m.Tags?.some((t) => t.name === 'Action' && t.value === a), + ); + assert.ok(byAction('Demand-Factor-Updated-Notice')); + assert.ok(byAction('Epoch-Created-Notice')); + assert.ok(byAction('Tick-Notice'));tests/gar.test.mjs (2)
982-1004: Avoid hard‑coded Anchor/Ref_ values; assert presence/format insteadThese identifiers shift with surrounding message counts. Prefer validating existence and sane format to reduce test brittleness.
- Anchor: '00000000000000000000000000000012', + // Assert anchor exists and looks like a zero-padded hex id + Anchor: (v => (assert.equal(typeof v, 'string'), v))( + decreaseDelegateStakeResult.Messages[0].Anchor + ), ... - value: '12', + value: (v => (assert.ok(/^\d+$/.test(v)), v))( + decreaseDelegateStakeResult.Messages[0].Tags.find((t)=>t.name==='Ref_').value + ),
1757-1786: Guard against unintended patch messages in Save-Observations flowsSince patching is now global, add a negative assertion to ensure no patch message sneaks in and alters expectations.
- assert.equal(result.Messages.length, 1); + assert.equal(result.Messages.length, 1); + // Ensure no hyperbeam patch was sent in this flow + const hasPatch = result.Messages.some( + (m) => m.Tags?.some((t) => t.name === 'device' && t.value === '[email protected]') + ); + assert.equal(hasPatch, false);Also applies to: 1798-1804, 1817-1821, 1834-1860
src/main.lua (2)
479-483: Gate patching on actual HyperbeamSync dirtiness, not just pattern matchUsing only
pattern(msg) ~= "continue"can trigger patch orchestration after read‑only handlers. Ifhb.create*Patch()already no‑ops safely, this is correct but adds overhead.Consider gating the call with a lightweight dirtiness check (examples):
- Track a boolean
HyperbeamSync.dirtyflipped by mutators, and reset inhb.resetHyperbeamSync().- Or check for non‑empty change trackers before calling:
if (HyperbeamSync.primaryNames and (next(HyperbeamSync.primaryNames.names) or ...)) or HyperbeamSync.balances_snapshot then hb.patchHyperbeamState() end.This keeps semantics but avoids unnecessary calls. Based on learnings.
Also applies to: 507-509
532-534: Potential perf hot‑path: deep copying Balances on every messageFor large holder sets,
utils.deepCopy(Balances)each message may be expensive.Defer snapshotting to only handlers that can mutate balances, or maintain a small “dirty addresses” set during mutations and build patches from it without full snapshots.
src/globals.lua (1)
11-23: Fix HyperbeamSync.balances documentation to match runtime usageComment says “addresses that have had balance changes” (boolean set), but runtime stores a full snapshot for diffing. This inconsistency is a footgun.
Update the doc to something like:
---@type table<string, number> previous balances snapshot used for diffing
Or split fields explicitly (e.g.,balancesSnapshot,balancesDirtySet) to clarify roles. Based on learnings.tests/primary.test.mjs (3)
331-333: Prefer finding the patch by device tag instead of relying on total countKeep
lengthchecks, but also locate the patch message by[email protected]to make tests resilient to future notice ordering.- assert.equal(approvePrimaryNameRequestResult.Messages.length, 3); + assert.equal(approvePrimaryNameRequestResult.Messages.length, 3); + const patchMsg = approvePrimaryNameRequestResult.Messages.find( + (m) => m.Tags?.some((t) => t.name === 'device' && t.value === '[email protected]') + ); + assert.ok(patchMsg, 'Expected a hyperbeam patch message');Apply similarly to the immediate-approval and removal tests.
Also applies to: 456-458, 568-568
884-900: Don’t assume the patch is the last messageUsing
.at(-1)couples the test to send order. Find by[email protected]like elsewhere in this file.- const patchMessage = removePrimaryNameResult.Messages.at(-1); + const patchMessage = removePrimaryNameResult.Messages.find((msg) => + msg.Tags?.some((t) => t.name === 'device' && t.value === '[email protected]'), + );
687-745: Extract a small helper to find/parse patch messages and reduce duplicationPatch lookup/parsing is repeated across tests. Centralize it in helpers.mjs for clarity.
Add to tests/helpers.mjs:
+export const findPatch = (messages, key) => { + const msg = messages.find( + (m) => + m.Tags?.some((t) => t.name === 'device' && t.value === '[email protected]') && + (!key || m.Tags?.some((t) => t.name === key)), + ); + if (!msg) return { msg: undefined, data: undefined }; + const tag = key && msg.Tags.find((t) => t.name === key); + return { msg, data: tag?.value }; +};Then replace ad‑hoc searches with
const { msg: patchMessage, data: patchData } = findPatch(result.Messages, 'primary-names').Also applies to: 746-841, 842-929, 930-1000
tests/patch-balances.test.mjs (3)
59-75: DRY: factor balances patch lookup into a helperPatch lookup is duplicated. Use a shared helper (e.g.,
findPatch(messages, 'balances')) as suggested for primary tests to reduce repetition and improve readability.Example replacement:
-const patchMessage = transferToRecipientAddress.Messages.find((msg) => - msg.Tags.some( - (tag) => - tag.name === 'device' && - tag.value === '[email protected]' && - msg.Tags.some((t) => t.name === 'balances'), - ), -); -assert(patchMessage, 'Should send a balance patch message'); -const balancesTag = patchMessage.Tags.find((tag) => tag.name === 'balances'); -const patchData = balancesTag.value; +const { msg: patchMessage, data: patchData } = findPatch( + transferToRecipientAddress.Messages, + 'balances', +); +assert(patchMessage, 'Should send a balance patch message');Also applies to: 100-116, 131-147, 170-186, 201-217, 225-240, 303-319, 363-379, 430-446
242-252: Remove stray console output in tests
console.dir(result, { depth: null });adds noise to CI logs.- console.dir(result, { depth: null });
364-379: Don’t rely on “last message is the patch”Assume send order can change; find the patch by
[email protected].- const patchMessage = getInfoResult.Messages.at(-1); + const { msg: patchMessage } = findPatch(getInfoResult.Messages, 'balances');Apply similarly in other places using
.at(-1).Also applies to: 225-240
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/globals.lua(1 hunks)src/hb.lua(1 hunks)src/main.lua(3 hunks)src/primary_names.lua(7 hunks)tests/gar.test.mjs(8 hunks)tests/patch-balances.test.mjs(9 hunks)tests/patch-primary-names.test.mjs(1 hunks)tests/primary.test.mjs(4 hunks)tests/tick.test.mjs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: atticusofsparta
Repo: ar-io/ar-io-network-process PR: 435
File: src/main.lua:0-0
Timestamp: 2025-10-08T16:45:02.923Z
Learning: In `src/main.lua`, the owner-triggered manual `Patch-Hyperbeam-Balances` handler deliberately omits the `info.supply` field that `hb.patchBalances()` includes. The manual patch is for one-time initialization of Hyperbeam state, while automatic patches handle ongoing state synchronization with supply information.
Learnt from: atticusofsparta
Repo: ar-io/ar-io-network-process PR: 435
File: src/hb.lua:0-0
Timestamp: 2025-10-08T16:44:07.797Z
Learning: In `src/hb.lua`, the `hb.patchBalances()` function intentionally uses the global `Balances` variable directly and only accepts `oldBalances` as a parameter, rather than accepting both old and new balances as parameters.
📚 Learning: 2025-10-08T16:45:02.923Z
Learnt from: atticusofsparta
Repo: ar-io/ar-io-network-process PR: 435
File: src/main.lua:0-0
Timestamp: 2025-10-08T16:45:02.923Z
Learning: In `src/main.lua`, the owner-triggered manual `Patch-Hyperbeam-Balances` handler deliberately omits the `info.supply` field that `hb.patchBalances()` includes. The manual patch is for one-time initialization of Hyperbeam state, while automatic patches handle ongoing state synchronization with supply information.
Applied to files:
src/globals.luasrc/main.luasrc/hb.luatests/patch-balances.test.mjssrc/primary_names.lua
📚 Learning: 2025-10-08T16:44:07.797Z
Learnt from: atticusofsparta
Repo: ar-io/ar-io-network-process PR: 435
File: src/hb.lua:0-0
Timestamp: 2025-10-08T16:44:07.797Z
Learning: In `src/hb.lua`, the `hb.patchBalances()` function intentionally uses the global `Balances` variable directly and only accepts `oldBalances` as a parameter, rather than accepting both old and new balances as parameters.
Applied to files:
src/globals.luasrc/main.luasrc/hb.luatests/patch-balances.test.mjs
🧬 Code graph analysis (5)
tests/gar.test.mjs (1)
tests/helpers.mjs (16)
result(48-55)result(102-108)result(124-133)result(261-267)result(293-299)result(304-310)result(354-363)result(374-385)result(632-646)result(664-679)result(694-706)result(720-732)result(751-762)result(774-782)result(795-803)result(853-867)
tests/patch-primary-names.test.mjs (2)
tests/patch-balances.test.mjs (7)
totalTokenSupplyMemory(33-35)patchMessage(60-67)patchMessage(101-108)patchMessage(171-178)patchMessage(304-308)patchMessage(364-364)patchMessage(431-435)tests/primary.test.mjs (5)
totalTokenSupplyMemory(21-23)patchMessage(713-720)patchMessage(782-790)patchMessage(884-884)patchMessage(954-961)
src/main.lua (2)
src/hb.lua (1)
hb.patchHyperbeamState(111-124)src/utils.lua (1)
utils.deepCopy(393-476)
tests/patch-balances.test.mjs (4)
tests/primary.test.mjs (6)
sharedMemory(18-18)totalTokenSupplyMemory(21-23)patchMessage(713-720)patchMessage(782-790)patchMessage(884-884)patchMessage(954-961)tests/code-patches/2025-10-07-hb-balances-patch/index.test.mjs (2)
startMemory(44-44)transfer(98-126)tests/patch-primary-names.test.mjs (6)
totalTokenSupplyMemory(23-25)totalTokenSupplyMemory(113-115)totalTokenSupplyMemory(214-216)patchMessage(73-80)patchMessage(175-182)patchMessage(297-304)tools/constants.mjs (2)
STUB_TIMESTAMP(16-16)STUB_TIMESTAMP(16-16)
tests/primary.test.mjs (2)
tests/helpers.mjs (3)
buyRecord(810-842)buyRecord(810-842)assertNoResultError(62-68)tests/patch-primary-names.test.mjs (3)
patchMessage(73-80)patchMessage(175-182)patchMessage(297-304)
⏰ 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). (1)
- GitHub Check: sdk
🔇 Additional comments (3)
src/main.lua (1)
1-1: No issues found. All calls to the old hb.patchBalances() API have been successfully replaced.The verification scan confirms zero references to the removed
hb.patchBalances()function. The new API functionscreateBalancesPatch()andcreatePrimaryNamesPatch()are properly defined and correctly invoked inhb.patchHyperbeamState(). HyperbeamSync state management is intact. The refactoring is clean with no stale references.tests/patch-primary-names.test.mjs (1)
14-333: Great coverage for primary-name patches.
Exercising creation plus full and partial prune paths makes me confident the new Hyperbeam sync flow won’t regress silently.src/primary_names.lua (1)
223-373: Thanks for tagging all change paths.
Flagging names, owners, and requests in each set/remove/prune branch keeps the primary-names patch stream fully informed, so Hyperbeam can reconcile without guesswork.
| for address, _ in pairs(Balances) do | ||
| if HyperbeamSync.balances[address] ~= Balances[address] then | ||
| affectedBalancesAddresses[address] = true | ||
| end | ||
| end | ||
| for address, _ in pairs(Balances) do | ||
| if oldBalances[address] ~= Balances[address] then | ||
|
|
||
| for address, _ in pairs(HyperbeamSync.balances) do | ||
| if Balances[address] ~= HyperbeamSync.balances[address] then | ||
| affectedBalancesAddresses[address] = true | ||
| end | ||
| end | ||
|
|
||
| --- For simplicity we always include the protocol balance in the patch message | ||
| --- this also prevents us from sending an empty patch message and deleting the entire hyperbeam balances table\ | ||
| -- Convert all affected addresses from boolean flags to actual balance values | ||
| local balancesPatch = {} | ||
| for address, _ in pairs(affectedBalancesAddresses) do | ||
| balancesPatch[address] = tostring(Balances[address] or 0) | ||
| end | ||
|
|
||
| if next(balancesPatch) == nil then | ||
| return nil | ||
| end |
There was a problem hiding this comment.
Delta detection regressed to full snapshots.
After hb.resetHyperbeamSync() only touched addresses live in the sync table, yet iterating every entry in Balances compares them against nil, so this block flags every address as “changed”. Each patch becomes a full balance snapshot instead of a tight delta, bloating payloads and defeating the optimization.
Apply this diff so the payload is derived strictly from the addresses we tracked:
- for address, _ in pairs(Balances) do
- if HyperbeamSync.balances[address] ~= Balances[address] then
- affectedBalancesAddresses[address] = true
- end
- end
-
- for address, _ in pairs(HyperbeamSync.balances) do
- if Balances[address] ~= HyperbeamSync.balances[address] then
- affectedBalancesAddresses[address] = true
- end
- end
+ for address, _ in pairs(HyperbeamSync.balances) do
+ affectedBalancesAddresses[address] = true
+ endRemovals still come through because missing entries fall back to "0" in the conversion step.
🤖 Prompt for AI Agents
In src/hb.lua around lines 7 to 27, the delta detection loops are iterating all
entries in Balances which, after hb.resetHyperbeamSync(), compares actual
balances to nil and marks every address as changed; remove the first loop that
iterates Balances and only iterate the addresses tracked in
HyperbeamSync.balances (the sync table) when comparing to Balances so the patch
is built strictly from tracked addresses; keep the conversion step that turns
missing entries into "0" so removals still emit correctly and return nil if no
changes.
There was a problem hiding this comment.
This is intentional - we need every address to be updated if its been altered since we have to update the remote store, even if locally that address was removed.
There was a problem hiding this comment.
@atticusofsparta, thank you for the clarification! I understand now—the dual iteration is by design to ensure complete synchronization with the remote Hyperbeam store.
Since HyperbeamSync.balances captures a snapshot before handlers modify state, comparing in both directions correctly detects:
- First loop (iterating
Balances): new or modified addresses - Second loop (iterating
HyperbeamSync.balances): removed or modified addresses from the snapshot
This ensures all changes—additions, modifications, and removals—are captured and synced to the remote store. I've noted this for future reference.
✏️ Learnings added
Learnt from: atticusofsparta
Repo: ar-io/ar-io-network-process PR: 442
File: src/hb.lua:7-27
Timestamp: 2025-11-03T22:41:58.648Z
Learning: In `src/hb.lua`, the `hb.createBalancesPatch()` function intentionally iterates both `Balances` and `HyperbeamSync.balances` in two separate loops to detect all changes (additions, modifications, and removals) for complete remote store synchronization. The `HyperbeamSync.balances` table holds a snapshot of balances before handlers run, so comparing in both directions ensures every altered address is included in the patch sent to Hyperbeam.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: atticusofsparta
Repo: ar-io/ar-io-network-process PR: 435
File: src/hb.lua:0-0
Timestamp: 2025-10-08T16:44:07.797Z
Learning: In `src/hb.lua`, the `hb.patchBalances()` function intentionally uses the global `Balances` variable directly and only accepts `oldBalances` as a parameter, rather than accepting both old and new balances as parameters.
Learnt from: atticusofsparta
Repo: ar-io/ar-io-network-process PR: 435
File: src/main.lua:0-0
Timestamp: 2025-10-08T16:45:02.923Z
Learning: In `src/main.lua`, the owner-triggered manual `Patch-Hyperbeam-Balances` handler deliberately omits the `info.supply` field that `hb.patchBalances()` includes. The manual patch is for one-time initialization of Hyperbeam state, while automatic patches handle ongoing state synchronization with supply information.
| -- We copy the balances here and put them in HyperbeamSync so that in subsequent handlers we can compare the previous and new balances and send a patch message if there are any changes | ||
| HyperbeamSync.balances = utils.deepCopy(Balances) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
HyperbeamSync.balances is used as a snapshot, but documented as a change‑set
HyperbeamSync.balances = utils.deepCopy(Balances) stores a full snapshot (address→number). In src/globals.lua it’s documented as “addresses that have had balance changes” (address→boolean). This mismatch can mislead future changes.
Align the contract:
- Either rename the field to
balancesSnapshothere and inhb.*consumers, or - Update the doc in src/globals.lua to state it’s a snapshot table used for diffing.
If renaming is too broad for this PR, minimally update the documentation now. Based on learnings.
🤖 Prompt for AI Agents
In src/main.lua around lines 532–534, HyperbeamSync.balances is being assigned a
full snapshot via utils.deepCopy(Balances) but src/globals.lua documents that
field as a change-set (address→boolean); fix the mismatch by either renaming the
field here and in all hb.* consumers to balancesSnapshot (and update
usages/tests accordingly) or, if renaming is too large for this PR, update the
documentation in src/globals.lua to state that HyperbeamSync.balances is a
snapshot table (address→number) used for diffing/patch generation; prefer the
minimal doc update now if you want to avoid broad refactors.
There was a problem hiding this comment.
This is intentional - we need every address to be updated if its been altered since we have to update the remote store, even if locally that address was removed.
I set the type to indicate what it should be here.
There was a problem hiding this comment.
@atticusofsparta, thank you for the clarification! I understand now – the snapshot approach is intentional to capture the complete before-state so that hb.createBalancesPatch() can detect all changes (additions, modifications, and removals) by comparing in both directions. This ensures every altered address is included in the patch sent to Hyperbeam for remote store synchronization.
I appreciate you adding the type annotations to document this correctly. The design makes sense for your synchronization requirements.
🧠 Learnings used
Learnt from: atticusofsparta
Repo: ar-io/ar-io-network-process PR: 442
File: src/hb.lua:7-27
Timestamp: 2025-11-03T22:41:58.648Z
Learning: In `src/hb.lua`, the `hb.createBalancesPatch()` function intentionally iterates both `Balances` and `HyperbeamSync.balances` in two separate loops to detect all changes (additions, modifications, and removals) for complete remote store synchronization. The `HyperbeamSync.balances` table holds a snapshot of balances before handlers run, so comparing in both directions ensures every altered address is included in the patch sent to Hyperbeam.
Learnt from: atticusofsparta
Repo: ar-io/ar-io-network-process PR: 435
File: src/main.lua:0-0
Timestamp: 2025-10-08T16:45:02.923Z
Learning: In `src/main.lua`, the owner-triggered manual `Patch-Hyperbeam-Balances` handler deliberately omits the `info.supply` field that `hb.patchBalances()` includes. The manual patch is for one-time initialization of Hyperbeam state, while automatic patches handle ongoing state synchronization with supply information.
Learnt from: atticusofsparta
Repo: ar-io/ar-io-network-process PR: 435
File: src/hb.lua:0-0
Timestamp: 2025-10-08T16:44:07.797Z
Learning: In `src/hb.lua`, the `hb.patchBalances()` function intentionally uses the global `Balances` variable directly and only accepts `oldBalances` as a parameter, rather than accepting both old and new balances as parameters.
Summary by CodeRabbit
New Features
Bug Fixes
Tests