Conversation
WalkthroughRefactors snapshot system to epoch-aligned timestamps, removes Changes
Sequence Diagram(s)sequenceDiagram
participant EventHandler
participant Aggregator
participant SnapshotShared
participant ContextStore
EventHandler->>Aggregator: call update*(diff, current, context, timestamp)
Aggregator->>Aggregator: apply diffs, compute updated entity
Aggregator->>SnapshotShared: shouldSnapshot(lastSnapshotTimestamp, timestamp)?
alt should snapshot
Aggregator->>SnapshotShared: create*Snapshot(entity, timestamp) -> snapshot
Aggregator->>SnapshotShared: wrap SnapshotForPersist(type, snapshot)
SnapshotShared->>ContextStore: persistSnapshot(snapshotForPersist, context)
Aggregator->>Aggregator: set lastSnapshotTimestamp = getSnapshotEpoch(timestamp)
end
Aggregator->>ContextStore: write updated entity/state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
FonsSicca
commented
Feb 18, 2026
Comment on lines
+43
to
+50
| if (shouldSnapshot(current.lastSnapshotTimestamp, timestamp)) { | ||
| setALMLPWrapperSnapshot(updated, timestamp, context); | ||
| updated = { | ||
| ...updated, | ||
| lastSnapshotTimestamp: getSnapshotEpoch(timestamp), | ||
| }; | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
ALM wrapper snapshot implementation
FonsSicca
commented
Feb 18, 2026
Comment on lines
+50
to
+56
| if (shouldSnapshot(current.lastSnapshotTimestamp, timestamp)) { | ||
| setNonFungiblePositionSnapshot(nonFungiblePosition, timestamp, context); | ||
| nonFungiblePosition = { | ||
| ...nonFungiblePosition, | ||
| lastSnapshotTimestamp: getSnapshotEpoch(timestamp), | ||
| }; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
NFP snapshot implementation
FonsSicca
commented
Feb 18, 2026
Comment on lines
+385
to
+392
| if (shouldSnapshot(current.lastSnapshotTimestamp, timestamp)) { | ||
| setUserStatsPerPoolSnapshot(updated, timestamp, context); | ||
| updated = { | ||
| ...updated, | ||
| lastSnapshotTimestamp: getSnapshotEpoch(timestamp), | ||
| }; | ||
| } | ||
|
|
Contributor
Author
There was a problem hiding this comment.
User snapshot implementation
FonsSicca
commented
Feb 18, 2026
Comment on lines
+66
to
+73
|
|
||
| if (shouldSnapshot(current.lastSnapshotTimestamp, timestamp)) { | ||
| setVeNFTStateSnapshot(veNFTState, timestamp, context); | ||
| veNFTState = { | ||
| ...veNFTState, | ||
| lastSnapshotTimestamp: getSnapshotEpoch(timestamp), | ||
| }; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
veNF snapshot implementation
FonsSicca
commented
Feb 18, 2026
Comment on lines
+159
to
+167
| setTokenPriceSnapshot( | ||
| token.address, | ||
| chainId, | ||
| blockNumber, | ||
| new Date(blockTimestampMs), | ||
| currentPrice, | ||
| token.isWhitelisted, | ||
| context, | ||
| ); |
Contributor
Author
There was a problem hiding this comment.
Token snapshot implementation
WooSungD
reviewed
Feb 19, 2026
| }; | ||
|
|
||
| if (shouldSnapshot(current.lastSnapshotTimestamp, timestamp)) { | ||
| setNonFungiblePositionSnapshot(nonFungiblePosition, timestamp, context); |
Contributor
There was a problem hiding this comment.
[discussion; non-blocking] making the current set function into a get function and setting the context inside this function could be better for future debugging i.e. all context.<>.set happens in one place.
Example
nonFungiblePositionSnapshot = getNonFungiblePositionSnapshot(nonFungiblePosition, timestamp, context);
context.NonFungiblePositionSnapshot.set(nonFungiblePositionSnapshot);
Contributor
Author
There was a problem hiding this comment.
Made this commit following your suggestion
WooSungD
approved these changes
Feb 19, 2026
FonsSicca
force-pushed
the
dan/snapshots-initial-setup
branch
from
0f55a70 to
2ffe329
Compare
FonsSicca
force-pushed
the
dan/snapshot-implementation
branch
from
207cbcf to
b6c4569
Compare
![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 (13)
test/EventHandlers/Pool/PoolBurnAndMintLogic.test.ts (1)
38-38:new Date()in a fixture violates the test guideline — prefer a fixed timestamp from a shared helper.
TIMESTAMP_DATEis constructed withnew Date(TIMESTAMP * 1000)and is passed directly toprocessPoolLiquidityEvent, which now drives the new epoch-aligned snapshot logic. The guideline says to avoidnew Date()in fixtures unless using fake timers and to prefer fixed timestamps from shared helpers.♻️ Suggested refactor
- const TIMESTAMP = 1000000; - const TIMESTAMP_DATE = new Date(TIMESTAMP * 1000); + const TIMESTAMP = 1000000; + // Derive a stable Date from the fixed epoch constant instead of calling new Date() directly. + const TIMESTAMP_DATE = new Date(1000000 * 1000); // or import from a shared helperIf
common.ts(or another shared helper) already exports a canonicalTIMESTAMP/TIMESTAMP_DATE, import and reuse it here instead.As per coding guidelines: "Avoid
new Date()in assertions and fixtures unless using fake timers; prefer fixed timestamps from shared helpers."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/EventHandlers/Pool/PoolBurnAndMintLogic.test.ts` at line 38, Replace the local construction of TIMESTAMP_DATE using new Date(TIMESTAMP * 1000) with the canonical fixed timestamp/date provided by the shared test helper (e.g., import and use the exported TIMESTAMP or TIMESTAMP_DATE from common.ts), and pass that fixed value into processPoolLiquidityEvent (reference: TIMESTAMP_DATE and processPoolLiquidityEvent) so the test uses the shared, deterministic timestamp fixture instead of constructing a Date inside the test.src/EventHandlers/VotingReward/BribesVotingReward.ts (1)
52-57: Timestamp propagation is correct.The
new Date(data.timestamp * 1000)conversion matches the pattern used at line 45 for the pool update. Minor note: both lines create separateDateinstances for the same timestamp — could be extracted to a sharedconst timestamp = new Date(data.timestamp * 1000)at the top of the handler for clarity, but this is cosmetic.♻️ Optional: extract shared timestamp
+ const timestamp = new Date(data.timestamp * 1000); + await Promise.all([ result.poolDiff ? updateLiquidityPoolAggregator( result.poolDiff, loadedData.poolData.liquidityPoolAggregator, - new Date(data.timestamp * 1000), + timestamp, context, event.chainId, data.blockNumber, ) : Promise.resolve(), result.userDiff ? updateUserStatsPerPool( result.userDiff, loadedData.userData, context, - new Date(data.timestamp * 1000), + timestamp, ) : Promise.resolve(), ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EventHandlers/VotingReward/BribesVotingReward.ts` around lines 52 - 57, The two places creating Date objects for the same timestamp (the call to updateUserStatsPerPool and the earlier pool update around line 45) should reuse a single Date instance for clarity; extract const timestamp = new Date(data.timestamp * 1000) at the start of the handler and pass timestamp to updateUserStatsPerPool (and to the pool update call) instead of creating new Date(...) inline.test/Aggregators/VeNFTState.test.ts (1)
259-279: PrefergetSnapshotEpochover hardcoded interval for thelastSnapshotTimestampassertion.Line 262 hardcodes
60 * 60 * 1000as the snapshot interval, while line 266 correctly usesgetSnapshotEpoch(timestamp). IfSNAPSHOT_INTERVAL_IN_MSever changes, the assertion on line 262 would silently drift. Consider usinggetSnapshotEpochfor both assertions for consistency.♻️ Suggested change
- expect(updated.lastSnapshotTimestamp?.getTime()).toBe( - Math.floor(timestamp.getTime() / (60 * 60 * 1000)) * (60 * 60 * 1000), - ); + const expectedEpoch = getSnapshotEpoch(timestamp); + expect(updated.lastSnapshotTimestamp?.getTime()).toBe( + expectedEpoch.getTime(), + );Then the
const expectedEpochdeclaration on line 266 can be removed since it's already defined above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Aggregators/VeNFTState.test.ts` around lines 259 - 279, The first assertion that checks updated.lastSnapshotTimestamp currently computes the epoch with a hardcoded 60*60*1000; change it to use getSnapshotEpoch(timestamp) (i.e., compare updated.lastSnapshotTimestamp or its .getTime() to expectedEpoch or expectedEpoch.getTime()) so both assertions use the same helper; remove the redundant const expectedEpoch if you end up reusing the one declared later and ensure the VeNFTStateSnapshot.set expectation continues to use VeNFTStateSnapshotId, chainId, tokenId, owner, locktime, lastUpdatedTimestamp, totalValueLocked, isAlive and timestamp as before.src/EventHandlers/NFPM/NFPMDecreaseLiquidityLogic.ts (1)
75-81: Optional: redundantnew Date(event.block.timestamp * 1000)construction.
calculateDecreaseLiquidityDiffalready computesconst blockDatetime = new Date(event.block.timestamp * 1000)internally (line 24) and stores it in the diff'slastUpdatedTimestamp. The newtimestampat line 75 creates a second, identicalDateobject. Consider returning theDatefromcalculateDecreaseLiquidityDiffor accepting it as a parameter so both usages share a single object.♻️ Example refactor
-export function calculateDecreaseLiquidityDiff( - event: NFPM_DecreaseLiquidity_event, -): Partial<NonFungiblePositionDiff> { - const blockDatetime = new Date(event.block.timestamp * 1000); - ... - return nonFungiblePositionDiff; -} +export function calculateDecreaseLiquidityDiff( + event: NFPM_DecreaseLiquidity_event, + timestamp: Date, +): Partial<NonFungiblePositionDiff> { + ... + const nonFungiblePositionDiff = { + incrementalLiquidity: -event.params.liquidity, + lastUpdatedTimestamp: timestamp, + }; + return nonFungiblePositionDiff; +}+ const timestamp = new Date(event.block.timestamp * 1000); - const nonFungiblePositionDiff = calculateDecreaseLiquidityDiff(event); + const nonFungiblePositionDiff = calculateDecreaseLiquidityDiff(event, timestamp); - const timestamp = new Date(event.block.timestamp * 1000); updateNonFungiblePosition(nonFungiblePositionDiff, position, context, timestamp);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EventHandlers/NFPM/NFPMDecreaseLiquidityLogic.ts` around lines 75 - 81, The code creates a duplicate Date by calling new Date(event.block.timestamp * 1000) before updateNonFungiblePosition even though calculateDecreaseLiquidityDiff already computed and stored the timestamp in nonFungiblePositionDiff.lastUpdatedTimestamp; change the call site to reuse that existing Date (nonFungiblePositionDiff.lastUpdatedTimestamp) or adjust calculateDecreaseLiquidityDiff to return the Date explicitly and then pass that returned Date into updateNonFungiblePosition instead of constructing a new one so both uses share the single Date object (references: calculateDecreaseLiquidityDiff, nonFungiblePositionDiff.lastUpdatedTimestamp, updateNonFungiblePosition).test/Aggregators/NonFungiblePosition.test.ts (1)
112-117: Good snapshot epoch assertion — consider adding a "no snapshot within same epoch" test.The assertion correctly verifies that
lastSnapshotTimestampis set togetSnapshotEpoch(timestamp)and that the snapshot was written. However, all three test cases start withlastSnapshotTimestamp: undefined, so only the "first snapshot" path is covered. A test wherelastSnapshotTimestampis already set to the current epoch (asserting the snapshot is not taken) would strengthen confidence in theshouldSnapshotgate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Aggregators/NonFungiblePosition.test.ts` around lines 112 - 117, Add a new unit test in NonFungiblePosition.test.ts that seeds result.lastSnapshotTimestamp to getSnapshotEpoch(timestamp) (i.e., same epoch as the incoming timestamp) and asserts that no new snapshot is taken: call the same function/handler used in the other tests with that timestamp, then verify result.lastSnapshotTimestamp remains unchanged and mockContext.NonFungiblePositionSnapshot?.set was NOT called; use getSnapshotEpoch(timestamp) to compute the pre-seeded epoch and reference the same timestamp/result/mockContext variables used in the existing tests to keep setup consistent.test/EventHandlers/VeNFT/VeNFTLogic.test.ts (1)
388-397: Extract repeated timestamp construction to a named constant.
new Date(mockTransferEvent.block.timestamp * 1000)is constructed inline four times in assertions (lines 388, 396, 467, 528). Per coding guidelines, timestamps in fixtures/assertions should use fixed named values rather than inline constructions.♻️ Proposed refactor
+ const transferTimestamp = new Date(mockTransferEvent.block.timestamp * 1000); + expect(updateUserStatsSpy).toHaveBeenCalledWith( expect.objectContaining({ incrementalVeNFTamountStaked: -50n, }), previousOwnerStats, mockContext, - new Date(mockTransferEvent.block.timestamp * 1000), + transferTimestamp, ); expect(updateUserStatsSpy).toHaveBeenCalledWith( expect.objectContaining({ incrementalVeNFTamountStaked: 50n, }), newOwnerStats, mockContext, - new Date(mockTransferEvent.block.timestamp * 1000), + transferTimestamp, );Apply the same extraction in the
updatePreviousOwnerUserStatsOnTransferandupdateNewOwnerUserStatsOnTransferdescribe blocks (lines 467, 528). Based on learnings, the coding guideline specifies: "Avoid new Date() in assertions and fixtures unless using fake timers; prefer fixed timestamps from shared helpers."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/EventHandlers/VeNFT/VeNFTLogic.test.ts` around lines 388 - 397, The tests repeatedly construct new Date(mockTransferEvent.block.timestamp * 1000) inline; extract that into a single named constant (e.g., const expectedTransferDate = new Date(mockTransferEvent.block.timestamp * 1000)) and use that constant in all assertions that call updateUserStatsSpy and any other assertions in the updatePreviousOwnerUserStatsOnTransfer and updateNewOwnerUserStatsOnTransfer describe blocks so the timestamp is fixed and reused across expect calls involving updateUserStatsSpy, newOwnerStats, and related assertions.src/EventHandlers/Pool.ts (1)
90-138: Optional: extracttimestampbeforePromise.allto avoid computingnew Date(event.block.timestamp * 1000)twice.In both
Pool.Fees(lines 106 and 121) andPool.Swap(lines 157 and 172),new Date(event.block.timestamp * 1000)is constructed twice — once inline insideloadOrCreateUserDataand again as a namedtimestampconst afterwards. The named const can be hoisted above thePromise.alland reused in both places.♻️ Proposed refactor for Pool.Fees (same pattern applies to Pool.Swap)
Pool.Fees.handler(async ({ event, context }) => { + const timestamp = new Date(event.block.timestamp * 1000); const [poolData, userData] = await Promise.all([ loadPoolData( event.srcAddress, event.chainId, context, event.block.number, event.block.timestamp, ), loadOrCreateUserData( event.params.sender, event.srcAddress, event.chainId, context, - new Date(event.block.timestamp * 1000), + timestamp, ), ]); ... - const timestamp = new Date(event.block.timestamp * 1000); - // Update pool and user entities in parallel🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EventHandlers/Pool.ts` around lines 90 - 138, Hoist the computed timestamp into a const before the Promise.all so it’s created once and reused; specifically, compute const timestamp = new Date(event.block.timestamp * 1000) above the Promise.all that calls loadOrCreateUserData and then pass that timestamp into loadOrCreateUserData (instead of creating a new Date inline) and reuse the same timestamp when calling updateLiquidityPoolAggregator and updateUserStatsPerPool; update the Pool.Fees handler (functions: loadOrCreateUserData, updateLiquidityPoolAggregator, updateUserStatsPerPool) accordingly so the Date allocation is not duplicated.test/Aggregators/UserStatsPerPoolLiquidity.test.ts (2)
28-35:UserStatsPerPool.setshould bejest.fn()for consistency and debuggability.
async () => {}is an untracked no-op. If a future test needs to assert onUserStatsPerPool.set, it silently provides no call records — inconsistent withUserStatsPerPoolSnapshot: { set: jest.fn() }on the very next line.♻️ Proposed fix
- UserStatsPerPool: { set: async () => {} }, + UserStatsPerPool: { set: jest.fn() },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Aggregators/UserStatsPerPoolLiquidity.test.ts` around lines 28 - 35, Replace the untracked no-op used for UserStatsPerPool.set with a Jest mock so tests can assert calls; specifically, in the beforeEach where mockContext is created via setupCommon(), change UserStatsPerPool: { set: async () => {} } to use jest.fn() (e.g., UserStatsPerPool: { set: jest.fn() }) so the mockContext.UserStatsPerPool.set records invocations similar to UserStatsPerPoolSnapshot.set.
56-65: Snapshot assertion missing from multi-step tests.Every test calls
createMockUserStats()which produces an entity withlastSnapshotTimestamp: undefined.shouldSnapshot(undefined, mockTimestamp)returnstrueon the firstupdateUserStatsPerPoolcall in every test, soUserStatsPerPoolSnapshot.setis triggered in all tests — but only "should handle positive liquidity addition correctly" (line 56) asserts it. Consider adding a snapshot assertion (or at minimum a.not.toHaveBeenCalledTimes(0)check) to the other first-call tests for complete coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Aggregators/UserStatsPerPoolLiquidity.test.ts` around lines 56 - 65, The tests call createMockUserStats() which yields an entity with lastSnapshotTimestamp: undefined so shouldSnapshot(undefined, mockTimestamp) returns true on the first updateUserStatsPerPool call and always triggers UserStatsPerPoolSnapshot.set; update the other tests (besides "should handle positive liquidity addition correctly") to assert snapshot behavior as well—either assert UserStatsPerPoolSnapshot.set was called with the expected snapshot object (using expect.objectContaining with userAddress, poolAddress, chainId, etc.) or at minimum add a negative/positive call-count check like expect(mockContext.UserStatsPerPoolSnapshot.set).not.toHaveBeenCalledTimes(0) to reflect the first-call snapshot; update references to createMockUserStats, shouldSnapshot, updateUserStatsPerPool, and UserStatsPerPoolSnapshot.set when adding the assertions.test/Snapshots/Shared.test.ts (1)
78-227: ExtractsetupCommon()tobeforeEachto avoid repeating the call in every test.
const common = setupCommon()is identically repeated at the top of all 7persistSnapshottests. Per the guideline on minimizing duplication, it should be lifted tobeforeEach.♻️ Proposed refactor
describe("persistSnapshot", () => { const oneHourMs = SNAPSHOT_INTERVAL_IN_MS; + let common: ReturnType<typeof setupCommon>; + beforeEach(() => { + common = setupCommon(); + }); it("should call LiquidityPoolAggregatorSnapshot.set when type is LiquidityPoolAggregator", () => { - const common = setupCommon(); const context = common.createMockContext({ LiquidityPoolAggregatorSnapshot: { set: jest.fn() }, }); // ... rest of test unchangedBased on learnings: "Minimize duplication by reusing existing test helpers; if logic repeats across 2+ tests, extract it into a shared helper."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Snapshots/Shared.test.ts` around lines 78 - 227, The tests in the "persistSnapshot" describe block repeatedly call setupCommon(); extract that call into a beforeEach that assigns the result to a local variable used by each test (e.g., declare let common; in the describe and set common = setupCommon() inside beforeEach). Update each test to remove the const common = setupCommon() lines and use the shared common variable; keep all existing mocks and assertions referencing common, context, and persistSnapshot as-is.src/EventHandlers/CLPool.ts (1)
120-170:CollectFees,Flash, andSwapstill create duplicatenew Date(event.block.timestamp * 1000)— inconsistent with theCollectrefactor.
CLPool.Collect(line 71) correctly hoiststimestampbefore thePromise.alland reuses it in bothloadOrCreateUserDataandupdateUserStatsPerPool. The other three handlers still passnew Date(event.block.timestamp * 1000)inline toloadOrCreateUserDataand then redeclare an identicalconst timestampa few lines later.♻️ Proposed pattern (CollectFees shown; apply same to Flash and Swap)
CLPool.CollectFees.handler(async ({ event, context }) => { + const timestamp = new Date(event.block.timestamp * 1000); const [poolData, userData] = await Promise.all([ loadPoolData( event.srcAddress, event.chainId, context, event.block.number, event.block.timestamp, ), loadOrCreateUserData( event.params.recipient, event.srcAddress, event.chainId, context, - new Date(event.block.timestamp * 1000), + timestamp, ), ]); ... - const timestamp = new Date(event.block.timestamp * 1000); - // Update pool and user entities await Promise.all([Also applies to: 172-219, 332-407
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EventHandlers/CLPool.ts` around lines 120 - 170, The CollectFees handler (CLPool.CollectFees.handler) creates duplicate Date objects: it passes new Date(event.block.timestamp * 1000) into loadOrCreateUserData and then constructs an identical timestamp later; hoist a single const timestamp = new Date(event.block.timestamp * 1000) before calling Promise.all so both loadOrCreateUserData and updateUserStatsPerPool use the same timestamp instance, and apply the same change to the Flash and Swap handlers (they also call loadOrCreateUserData with inline new Date(...) and later redeclare timestamp); update CLPool.CollectFees.handler, CLPool.Flash.handler, and CLPool.Swap.handler to compute timestamp once and reuse it in both the concurrent loadOrCreateUserData call and the later updateUserStatsPerPool/updateLiquidityPoolAggregator calls.src/Snapshots/LiquidityPoolAggregatorSnapshot.ts (1)
104-117: Nit: inconsistent parameter naming insetLiquidityPoolAggregatorSnapshot.Other
setwrappers (VeNFTState, NonFungiblePosition, UserStatsPerPool, ALMLPWrapper) name the first parameterentity, while this one usesliquidityPoolAggregator. Trivial, but aligning would improve consistency.♻️ Suggested rename
export function setLiquidityPoolAggregatorSnapshot( - liquidityPoolAggregator: LiquidityPoolAggregator, + entity: LiquidityPoolAggregator, timestamp: Date, context: handlerContext, ): void { const snapshotForPersist: SnapshotForPersist = { type: SnapshotType.LiquidityPoolAggregator, snapshot: createLiquidityPoolAggregatorSnapshot( - liquidityPoolAggregator, + entity, timestamp, ), }; persistSnapshot(snapshotForPersist, context); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Snapshots/LiquidityPoolAggregatorSnapshot.ts` around lines 104 - 117, The parameter name for setLiquidityPoolAggregatorSnapshot is inconsistent with other setters; rename the first parameter liquidityPoolAggregator to entity to match conventions used in functions like setVeNFTState, setNonFungiblePosition, setUserStatsPerPool, and setALMLPWrapper; update all references within setLiquidityPoolAggregatorSnapshot (including the call to createLiquidityPoolAggregatorSnapshot) and keep the rest of the function logic unchanged so the SnapshotForPersist construction and persistSnapshot call continue to work.test/Aggregators/Users.test.ts (1)
50-68: Consider extracting the repeatedObject.assignmock setup into a helper.Every test case within
updateUserStatsPerPoolrepeats the sameObject.assign(mockContext.UserStatsPerPool, { set: ... })pattern with the samesavedUserStatscapture variable. A small helper (e.g.,captureUserStats()) could reduce the boilerplate and improve readability.♻️ Example helper extraction
function captureSet(mockContext: handlerContext) { let saved: UserStatsPerPool | undefined; Object.assign(mockContext.UserStatsPerPool, { set: async (userStats: UserStatsPerPool) => { saved = userStats; }, }); return () => saved; }Usage in each test:
const getSaved = captureSet(mockContext); // ... act ... expect(getSaved()).toEqual(result);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Aggregators/Users.test.ts` around lines 50 - 68, Several tests repeat Object.assign(mockContext.UserStatsPerPool, { set: ... }) and a savedUserStats capture; extract that into a small helper (e.g., captureSet or captureUserStats) placed inside the describe("updateUserStatsPerPool") scope that takes mockContext (or mockContext.UserStatsPerPool), installs a set: async (userStats) => { saved = userStats; } and returns a getter function for the saved value; then replace each test's Object.assign block with const getSaved = captureSet(mockContext) and assert getSaved() equals the expected result; reference the existing mockContext and the UserStatsPerPool.set method when implementing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/EventHandlers/NFPM/NFPMIncreaseLiquidityLogic.test.ts`:
- Around line 358-404: Replace the manual ID template and non-deterministic Date
in the nonMatchingMintEvent fixture: use the CLPoolMintEventId(...) helper to
build the id (instead of the inline
`${chainId}_${poolAddress}_${transactionHash}_${increaseLogIndex - 1}`) and
replace createdAt: new Date() with a fixed timestamp (e.g., new Date(0) or the
shared baseTimestamp used elsewhere) in the nonMatchingMintEvent object so the
fixture matches other mint-event fixtures and is deterministic.
In `@test/EventHandlers/Pool/PoolBurnAndMintLogic.test.ts`:
- Line 70: Add assertions in the three processPoolLiquidityEvent tests to verify
that the mocked UserStatsPerPoolSnapshot.set was invoked through the
updateUserStatsPerPool → setUserStatsPerPoolSnapshot path: locate the tests that
call processPoolLiquidityEvent and after the invocation add expects against the
mock UserStatsPerPoolSnapshot.set (e.g., toHaveBeenCalled or
toHaveBeenCalledWith with the expected snapshot payload/keys), ensuring each
test asserts whether the snapshot was set or not according to that test's
conditions.
---
Nitpick comments:
In `@src/EventHandlers/CLPool.ts`:
- Around line 120-170: The CollectFees handler (CLPool.CollectFees.handler)
creates duplicate Date objects: it passes new Date(event.block.timestamp * 1000)
into loadOrCreateUserData and then constructs an identical timestamp later;
hoist a single const timestamp = new Date(event.block.timestamp * 1000) before
calling Promise.all so both loadOrCreateUserData and updateUserStatsPerPool use
the same timestamp instance, and apply the same change to the Flash and Swap
handlers (they also call loadOrCreateUserData with inline new Date(...) and
later redeclare timestamp); update CLPool.CollectFees.handler,
CLPool.Flash.handler, and CLPool.Swap.handler to compute timestamp once and
reuse it in both the concurrent loadOrCreateUserData call and the later
updateUserStatsPerPool/updateLiquidityPoolAggregator calls.
In `@src/EventHandlers/NFPM/NFPMDecreaseLiquidityLogic.ts`:
- Around line 75-81: The code creates a duplicate Date by calling new
Date(event.block.timestamp * 1000) before updateNonFungiblePosition even though
calculateDecreaseLiquidityDiff already computed and stored the timestamp in
nonFungiblePositionDiff.lastUpdatedTimestamp; change the call site to reuse that
existing Date (nonFungiblePositionDiff.lastUpdatedTimestamp) or adjust
calculateDecreaseLiquidityDiff to return the Date explicitly and then pass that
returned Date into updateNonFungiblePosition instead of constructing a new one
so both uses share the single Date object (references:
calculateDecreaseLiquidityDiff, nonFungiblePositionDiff.lastUpdatedTimestamp,
updateNonFungiblePosition).
In `@src/EventHandlers/Pool.ts`:
- Around line 90-138: Hoist the computed timestamp into a const before the
Promise.all so it’s created once and reused; specifically, compute const
timestamp = new Date(event.block.timestamp * 1000) above the Promise.all that
calls loadOrCreateUserData and then pass that timestamp into
loadOrCreateUserData (instead of creating a new Date inline) and reuse the same
timestamp when calling updateLiquidityPoolAggregator and updateUserStatsPerPool;
update the Pool.Fees handler (functions: loadOrCreateUserData,
updateLiquidityPoolAggregator, updateUserStatsPerPool) accordingly so the Date
allocation is not duplicated.
In `@src/EventHandlers/VotingReward/BribesVotingReward.ts`:
- Around line 52-57: The two places creating Date objects for the same timestamp
(the call to updateUserStatsPerPool and the earlier pool update around line 45)
should reuse a single Date instance for clarity; extract const timestamp = new
Date(data.timestamp * 1000) at the start of the handler and pass timestamp to
updateUserStatsPerPool (and to the pool update call) instead of creating new
Date(...) inline.
In `@src/Snapshots/LiquidityPoolAggregatorSnapshot.ts`:
- Around line 104-117: The parameter name for setLiquidityPoolAggregatorSnapshot
is inconsistent with other setters; rename the first parameter
liquidityPoolAggregator to entity to match conventions used in functions like
setVeNFTState, setNonFungiblePosition, setUserStatsPerPool, and setALMLPWrapper;
update all references within setLiquidityPoolAggregatorSnapshot (including the
call to createLiquidityPoolAggregatorSnapshot) and keep the rest of the function
logic unchanged so the SnapshotForPersist construction and persistSnapshot call
continue to work.
In `@test/Aggregators/NonFungiblePosition.test.ts`:
- Around line 112-117: Add a new unit test in NonFungiblePosition.test.ts that
seeds result.lastSnapshotTimestamp to getSnapshotEpoch(timestamp) (i.e., same
epoch as the incoming timestamp) and asserts that no new snapshot is taken: call
the same function/handler used in the other tests with that timestamp, then
verify result.lastSnapshotTimestamp remains unchanged and
mockContext.NonFungiblePositionSnapshot?.set was NOT called; use
getSnapshotEpoch(timestamp) to compute the pre-seeded epoch and reference the
same timestamp/result/mockContext variables used in the existing tests to keep
setup consistent.
In `@test/Aggregators/Users.test.ts`:
- Around line 50-68: Several tests repeat
Object.assign(mockContext.UserStatsPerPool, { set: ... }) and a savedUserStats
capture; extract that into a small helper (e.g., captureSet or captureUserStats)
placed inside the describe("updateUserStatsPerPool") scope that takes
mockContext (or mockContext.UserStatsPerPool), installs a set: async (userStats)
=> { saved = userStats; } and returns a getter function for the saved value;
then replace each test's Object.assign block with const getSaved =
captureSet(mockContext) and assert getSaved() equals the expected result;
reference the existing mockContext and the UserStatsPerPool.set method when
implementing.
In `@test/Aggregators/UserStatsPerPoolLiquidity.test.ts`:
- Around line 28-35: Replace the untracked no-op used for UserStatsPerPool.set
with a Jest mock so tests can assert calls; specifically, in the beforeEach
where mockContext is created via setupCommon(), change UserStatsPerPool: { set:
async () => {} } to use jest.fn() (e.g., UserStatsPerPool: { set: jest.fn() })
so the mockContext.UserStatsPerPool.set records invocations similar to
UserStatsPerPoolSnapshot.set.
- Around line 56-65: The tests call createMockUserStats() which yields an entity
with lastSnapshotTimestamp: undefined so shouldSnapshot(undefined,
mockTimestamp) returns true on the first updateUserStatsPerPool call and always
triggers UserStatsPerPoolSnapshot.set; update the other tests (besides "should
handle positive liquidity addition correctly") to assert snapshot behavior as
well—either assert UserStatsPerPoolSnapshot.set was called with the expected
snapshot object (using expect.objectContaining with userAddress, poolAddress,
chainId, etc.) or at minimum add a negative/positive call-count check like
expect(mockContext.UserStatsPerPoolSnapshot.set).not.toHaveBeenCalledTimes(0) to
reflect the first-call snapshot; update references to createMockUserStats,
shouldSnapshot, updateUserStatsPerPool, and UserStatsPerPoolSnapshot.set when
adding the assertions.
In `@test/Aggregators/VeNFTState.test.ts`:
- Around line 259-279: The first assertion that checks
updated.lastSnapshotTimestamp currently computes the epoch with a hardcoded
60*60*1000; change it to use getSnapshotEpoch(timestamp) (i.e., compare
updated.lastSnapshotTimestamp or its .getTime() to expectedEpoch or
expectedEpoch.getTime()) so both assertions use the same helper; remove the
redundant const expectedEpoch if you end up reusing the one declared later and
ensure the VeNFTStateSnapshot.set expectation continues to use
VeNFTStateSnapshotId, chainId, tokenId, owner, locktime, lastUpdatedTimestamp,
totalValueLocked, isAlive and timestamp as before.
In `@test/EventHandlers/Pool/PoolBurnAndMintLogic.test.ts`:
- Line 38: Replace the local construction of TIMESTAMP_DATE using new
Date(TIMESTAMP * 1000) with the canonical fixed timestamp/date provided by the
shared test helper (e.g., import and use the exported TIMESTAMP or
TIMESTAMP_DATE from common.ts), and pass that fixed value into
processPoolLiquidityEvent (reference: TIMESTAMP_DATE and
processPoolLiquidityEvent) so the test uses the shared, deterministic timestamp
fixture instead of constructing a Date inside the test.
In `@test/EventHandlers/VeNFT/VeNFTLogic.test.ts`:
- Around line 388-397: The tests repeatedly construct new
Date(mockTransferEvent.block.timestamp * 1000) inline; extract that into a
single named constant (e.g., const expectedTransferDate = new
Date(mockTransferEvent.block.timestamp * 1000)) and use that constant in all
assertions that call updateUserStatsSpy and any other assertions in the
updatePreviousOwnerUserStatsOnTransfer and updateNewOwnerUserStatsOnTransfer
describe blocks so the timestamp is fixed and reused across expect calls
involving updateUserStatsSpy, newOwnerStats, and related assertions.
In `@test/Snapshots/Shared.test.ts`:
- Around line 78-227: The tests in the "persistSnapshot" describe block
repeatedly call setupCommon(); extract that call into a beforeEach that assigns
the result to a local variable used by each test (e.g., declare let common; in
the describe and set common = setupCommon() inside beforeEach). Update each test
to remove the const common = setupCommon() lines and use the shared common
variable; keep all existing mocks and assertions referencing common, context,
and persistSnapshot as-is.
coderabbit suggestions
FonsSicca
force-pushed
the
dan/snapshot-implementation
branch
from
ee5de64 to
e611d1a
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/EventHandlers/Pool.ts (1)
276-320: 🛠️ Refactor suggestion | 🟠 MajorInconsistent timestamp handling in
Pool.Claimcompared to other handlers.Other handlers (
Fees,Swap) extractconst timestamp = new Date(event.block.timestamp * 1000)at the top and reuse it for bothloadOrCreateUserDataandupdateUserStatsPerPool. Here,loadOrCreateUserDatareceives an inlinenew Date(...)(line 290), andupdateUserStatsPerPoolreceivesresult.poolDiff.lastUpdatedTimestamp as Date(line 308) — a different source.Two concerns:
- The inline
new Date(...)creates a distinct object from whateverprocessPoolClaimproduces, breaking the single-source-of-truth pattern.- The
as Dateassertion on line 308 masks a potentialundefinediflastUpdatedTimestampis optional in the diff type.Consider aligning with the other handlers:
Proposed fix
Pool.Claim.handler(async ({ event, context }) => { + const timestamp = new Date(event.block.timestamp * 1000); const [poolData, userData] = await Promise.all([ loadPoolData( event.srcAddress, event.chainId, context, event.block.number, event.block.timestamp, ), loadOrCreateUserData( event.params.sender, event.srcAddress, event.chainId, context, - new Date(event.block.timestamp * 1000), + timestamp, ), ]); if (!poolData) { return; } const { liquidityPoolAggregator, token0Instance, token1Instance } = poolData; const result = processPoolClaim( event, event.params.sender, liquidityPoolAggregator.gaugeAddress ?? "", token0Instance, token1Instance, ); - const timestamp = result.poolDiff.lastUpdatedTimestamp as Date; - await Promise.all([ updateLiquidityPoolAggregator( result.poolDiff, liquidityPoolAggregator, timestamp, context, event.chainId, event.block.number, ), updateUserStatsPerPool(result.userDiff, userData, context, timestamp), ]); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EventHandlers/Pool.ts` around lines 276 - 320, Compute a single timestamp once at the top of Pool.Claim.handler (e.g., const timestamp = new Date(event.block.timestamp * 1000)) and pass that same timestamp into loadOrCreateUserData instead of constructing a new Date inline, and into updateUserStatsPerPool instead of using result.poolDiff.lastUpdatedTimestamp as Date; do not use the `as Date` assertion — either set result.poolDiff.lastUpdatedTimestamp = timestamp after processPoolClaim or pass timestamp directly to updateLiquidityPoolAggregator and updateUserStatsPerPool so both use the same verified Date. Reference: Pool.Claim.handler, loadOrCreateUserData, processPoolClaim, result.poolDiff.lastUpdatedTimestamp, updateUserStatsPerPool, updateLiquidityPoolAggregator.
🧹 Nitpick comments (4)
src/EventHandlers/CLPool.ts (1)
236-249: Minor: duplicatenew Date(...)inIncreaseObservationCardinalityNext.Lines 238 and 244 each create a separate
new Date(event.block.timestamp * 1000). While functionally equivalent, extracting a singleconst timestampat the top (as the other handlers now do) would be more consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/EventHandlers/CLPool.ts` around lines 236 - 249, In IncreaseObservationCardinalityNext, avoid creating duplicate Date objects by extracting a single const timestamp = new Date(event.block.timestamp * 1000) at the top of the handler and use that single timestamp when building cardinalityDiff.lastUpdatedTimestamp and when calling updateLiquidityPoolAggregator (replace both new Date(event.block.timestamp * 1000) occurrences); update references to use the timestamp variable alongside existing symbols cardinalityDiff and updateLiquidityPoolAggregator.test/Snapshots/Shared.test.ts (1)
78-227: Good coverage of allpersistSnapshotbranches.Each snapshot type is verified, plus the exhaustiveness guard. Tests correctly use
setupCommon()builders and fixed timestamps.Consider
test.eachto reduce the boilerplate across the six per-type tests — each follows an identical arrange/act/assert shape differing only in the type, entity builder, and context key. As per coding guidelines: "Use test.each for table-driven tests with scenario matrices."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Snapshots/Shared.test.ts` around lines 78 - 227, The tests duplicate the same arrange/act/assert for each SnapshotType; replace the six per-type it blocks with a single table-driven test using test.each that iterates rows of { type: SnapshotType.X, contextKey: "XSnapshot", entityBuilder: common.createMockX / data, snapshotCreator: createXSnapshot, timestamp: new Date(...) }, call persistSnapshot({ type, snapshot }, context) inside the test, and assert context[contextKey].set was called once with snapshot; keep the existing exhaustiveness test as-is. Reference persistSnapshot, SnapshotType, the various create*Snapshot helpers (createLiquidityPoolAggregatorSnapshot, createUserStatsPerPoolSnapshot, createNonFungiblePositionSnapshot, createALMLPWrapperSnapshot, createVeNFTStateSnapshot, createTokenPriceSnapshot) and the context keys (LiquidityPoolAggregatorSnapshot, UserStatsPerPoolSnapshot, NonFungiblePositionSnapshot, ALM_LP_WrapperSnapshot, VeNFTStateSnapshot, TokenPriceSnapshot) when implementing the table and assertions.src/Snapshots/TokenPriceSnapshot.ts (1)
20-37: Consider an options object to reduce positional parameter fragility.Both
createTokenPriceSnapshotandsetTokenPriceSnapshotaccept 6–7 positional parameters, two of which (chainIdandblockNumber) share the same type (number). This makes call sites prone to silent argument-swap bugs. An options/params object would add clarity and safety.♻️ Sketch
+interface TokenPriceSnapshotParams { + address: string; + chainId: number; + blockNumber: number; + lastUpdatedTimestamp: Date; + pricePerUSDNew: bigint; + isWhitelisted: boolean; +} + -export function createTokenPriceSnapshot( - address: string, - chainId: number, - blockNumber: number, - lastUpdatedTimestamp: Date, - pricePerUSDNew: bigint, - isWhitelisted: boolean, -): TokenPriceSnapshot { +export function createTokenPriceSnapshot( + params: TokenPriceSnapshotParams, +): TokenPriceSnapshot { + const { address, chainId, blockNumber, lastUpdatedTimestamp, pricePerUSDNew, isWhitelisted } = params; const snapshotId = TokenIdByBlock(chainId, address, blockNumber); ... }Also applies to: 50-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Snapshots/TokenPriceSnapshot.ts` around lines 20 - 37, The factory createTokenPriceSnapshot (and similarly setTokenPriceSnapshot) is fragile due to multiple same-typed positional params (chainId, blockNumber); change the signature to accept a single options object (e.g. { address, chainId, blockNumber, lastUpdatedTimestamp, pricePerUSDNew, isWhitelisted }) and update the function body to destructure that object and still call TokenIdByBlock(chainId, address, blockNumber) to produce the id; then update all call sites to pass a single options object instead of positional args to prevent silent argument-swaps while preserving the TokenPriceSnapshot return shape.test/Snapshots/VeNFTStateSnapshot.test.ts (1)
54-95: Minor overlap between the twosetVeNFTStateSnapshotepoch-alignment tests.Lines 54–73 ("should compute snapshot epoch correctly") and lines 75–95 ("should set snapshot with epoch-aligned timestamp and correct id") both exercise
setVeNFTStateSnapshotand assert epoch-alignedid+timestamp. They differ only in which epoch boundary they target. Consider consolidating into a singletest.eachwith multiple timestamp/epoch pairs, or dropping one to reduce duplication.♻️ Example consolidation with test.each
- it("should compute snapshot epoch correctly (floor timestamp to interval boundary)", () => { - ... - const midEpochTimestamp = new Date( - SNAPSHOT_INTERVAL_IN_MS * 3 + 25 * 60 * 1000, - ); - const expectedEpochMs = SNAPSHOT_INTERVAL_IN_MS * 3; - ... - }); - - it("should set snapshot with epoch-aligned timestamp and correct id", () => { - ... - const timestamp = new Date(baseTimestamp.getTime() + 10 * 60 * 1000); - ... - const expectedEpochMs = SNAPSHOT_INTERVAL_IN_MS * 2; - ... - }); + it.each([ + { label: "mid-epoch (25min into 3rd hour)", timestampMs: SNAPSHOT_INTERVAL_IN_MS * 3 + 25 * 60 * 1000, expectedEpochMs: SNAPSHOT_INTERVAL_IN_MS * 3 }, + { label: "10min past baseTimestamp", timestampMs: SNAPSHOT_INTERVAL_IN_MS * 2 + 10 * 60 * 1000, expectedEpochMs: SNAPSHOT_INTERVAL_IN_MS * 2 }, + ])("should set epoch-aligned snapshot ($label)", ({ timestampMs, expectedEpochMs }) => { + const context = common.createMockContext({ VeNFTStateSnapshot: { set: jest.fn() } }); + const entity = common.createMockVeNFTState(); + setVeNFTStateSnapshot(entity, new Date(timestampMs), context); + const setArg = (context.VeNFTStateSnapshot.set as jest.Mock).mock.calls[0][0]; + expect(setArg.timestamp.getTime()).toBe(expectedEpochMs); + expect(setArg.id).toBe(VeNFTStateSnapshotId(entity.chainId, entity.tokenId, expectedEpochMs)); + });As per coding guidelines: "Use test.each for table-driven tests with scenario matrices".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Snapshots/VeNFTStateSnapshot.test.ts` around lines 54 - 95, The two tests both exercise setVeNFTStateSnapshot and assert epoch-aligned id/timestamp, causing duplication; refactor them into a single table-driven test using test.each that supplies multiple {name, inputTimestamp, expectedEpochMs, entityOverrides} rows (e.g., one row for the 3rd-hour midEpochTimestamp case and one for the baseTimestamp+10min case), and inside the test call setVeNFTStateSnapshot(entity, inputTimestamp, context) and assert the same id/timestamp expectations using VeNFTStateSnapshotId and SNAPSHOT_INTERVAL_IN_MS; keep references to setVeNFTStateSnapshot, VeNFTStateSnapshotId, SNAPSHOT_INTERVAL_IN_MS and the mock context so the assertions remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/EventHandlers/Pool.ts`:
- Around line 276-320: Compute a single timestamp once at the top of
Pool.Claim.handler (e.g., const timestamp = new Date(event.block.timestamp *
1000)) and pass that same timestamp into loadOrCreateUserData instead of
constructing a new Date inline, and into updateUserStatsPerPool instead of using
result.poolDiff.lastUpdatedTimestamp as Date; do not use the `as Date` assertion
— either set result.poolDiff.lastUpdatedTimestamp = timestamp after
processPoolClaim or pass timestamp directly to updateLiquidityPoolAggregator and
updateUserStatsPerPool so both use the same verified Date. Reference:
Pool.Claim.handler, loadOrCreateUserData, processPoolClaim,
result.poolDiff.lastUpdatedTimestamp, updateUserStatsPerPool,
updateLiquidityPoolAggregator.
---
Nitpick comments:
In `@src/EventHandlers/CLPool.ts`:
- Around line 236-249: In IncreaseObservationCardinalityNext, avoid creating
duplicate Date objects by extracting a single const timestamp = new
Date(event.block.timestamp * 1000) at the top of the handler and use that single
timestamp when building cardinalityDiff.lastUpdatedTimestamp and when calling
updateLiquidityPoolAggregator (replace both new Date(event.block.timestamp *
1000) occurrences); update references to use the timestamp variable alongside
existing symbols cardinalityDiff and updateLiquidityPoolAggregator.
In `@src/Snapshots/TokenPriceSnapshot.ts`:
- Around line 20-37: The factory createTokenPriceSnapshot (and similarly
setTokenPriceSnapshot) is fragile due to multiple same-typed positional params
(chainId, blockNumber); change the signature to accept a single options object
(e.g. { address, chainId, blockNumber, lastUpdatedTimestamp, pricePerUSDNew,
isWhitelisted }) and update the function body to destructure that object and
still call TokenIdByBlock(chainId, address, blockNumber) to produce the id; then
update all call sites to pass a single options object instead of positional args
to prevent silent argument-swaps while preserving the TokenPriceSnapshot return
shape.
In `@test/Snapshots/Shared.test.ts`:
- Around line 78-227: The tests duplicate the same arrange/act/assert for each
SnapshotType; replace the six per-type it blocks with a single table-driven test
using test.each that iterates rows of { type: SnapshotType.X, contextKey:
"XSnapshot", entityBuilder: common.createMockX / data, snapshotCreator:
createXSnapshot, timestamp: new Date(...) }, call persistSnapshot({ type,
snapshot }, context) inside the test, and assert context[contextKey].set was
called once with snapshot; keep the existing exhaustiveness test as-is.
Reference persistSnapshot, SnapshotType, the various create*Snapshot helpers
(createLiquidityPoolAggregatorSnapshot, createUserStatsPerPoolSnapshot,
createNonFungiblePositionSnapshot, createALMLPWrapperSnapshot,
createVeNFTStateSnapshot, createTokenPriceSnapshot) and the context keys
(LiquidityPoolAggregatorSnapshot, UserStatsPerPoolSnapshot,
NonFungiblePositionSnapshot, ALM_LP_WrapperSnapshot, VeNFTStateSnapshot,
TokenPriceSnapshot) when implementing the table and assertions.
In `@test/Snapshots/VeNFTStateSnapshot.test.ts`:
- Around line 54-95: The two tests both exercise setVeNFTStateSnapshot and
assert epoch-aligned id/timestamp, causing duplication; refactor them into a
single table-driven test using test.each that supplies multiple {name,
inputTimestamp, expectedEpochMs, entityOverrides} rows (e.g., one row for the
3rd-hour midEpochTimestamp case and one for the baseTimestamp+10min case), and
inside the test call setVeNFTStateSnapshot(entity, inputTimestamp, context) and
assert the same id/timestamp expectations using VeNFTStateSnapshotId and
SNAPSHOT_INTERVAL_IN_MS; keep references to setVeNFTStateSnapshot,
VeNFTStateSnapshotId, SNAPSHOT_INTERVAL_IN_MS and the mock context so the
assertions remain identical.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #454
closes #484
Implements:
Summary by CodeRabbit
Bug Fixes
Refactor
Tests