-
Notifications
You must be signed in to change notification settings - Fork 3
feat: withdrawal queue schema + allocation/deallocation rounding logic #474
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: master
Are you sure you want to change the base?
Conversation
3e7ab11 to
87d7809
Compare
pkg/postgres/migrations/202511141700_withdrawalQueueAndAllocationRounding/up.go
Outdated
Show resolved
Hide resolved
pkg/postgres/migrations/202511141700_withdrawalQueueAndAllocationRounding/up.go
Outdated
Show resolved
Hide resolved
pkg/postgres/migrations/202511141700_withdrawalQueueAndAllocationRounding/up.go
Outdated
Show resolved
Hide resolved
fix test fix time to utc fix time time utc fix fix
267d580 to
f51270b
Compare
bfc21ac to
4d8299b
Compare
| strategy, | ||
| operator_set_id, | ||
| magnitude, | ||
| block_time, |
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.
So the AllocationUpdated event emits an effectBlock. I'm not sure if we're using that anywhere here. The effectBlock is the block at which the allocation/dealloction becomes active
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.
Changed join from oa.block_number to oa.effective_block
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.
We're joining on the effective_block, but now we also need to make sure that we're making sure that the allocation is counted at the effective block and its associated day, not the block at which the event was emitted.
Example is when the event is emitted at block 1 but the allocation is activated at block 10
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.
correct,
INNER JOIN blocks b ON oa.effective_block = b.number
WHERE b.block_time < TIMESTAMP '{{.cutoffDate}}'
The join is on effective_block, so b.block_time comes from the effective block, not the emission block. The new test case added for the future allocation scenario verifies this
4a659a7 to
41d0383
Compare
ypatil12
left a comment
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.
One minor comment on the effective_block and a test to add. Other than looking solid!
| strategy, | ||
| operator_set_id, | ||
| magnitude, | ||
| block_time, |
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.
We're joining on the effective_block, but now we also need to make sure that we're making sure that the allocation is counted at the effective block and its associated day, not the block at which the event was emitted.
Example is when the event is emitted at block 1 but the allocation is activated at block 10
| "go.uber.org/zap" | ||
| ) | ||
|
|
||
| const operatorAllocationSnapshotsQuery = ` |
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.
Taking a look at operatorAVSRegistrationSnapshots.go, I think we might be missing a few cases in general:
From that file:
// Operator AVS Registration Windows: Ranges at which an operator has registered for an AVS
// 0. Ranked: Rank the operator state changes by block_time and log_index since sqlite lacks LEAD/LAG functions
// 1. Marked_statuses: Denote which registration status comes after one another
// 2. Removed_same_day_deregistrations: Remove a pairs of (registration, deregistration) that happen on the same day
// 3. Registration_periods: Combine registration together, only select registrations with:
// a. (Registered, Unregistered)
// b. (Registered, Null). If null, the end time is the current timestamp
// 4. Registration_snapshots: Round up each start_time to 0 UTC on NEXT DAY and round down each end_time to 0 UTC on CURRENT DAY
// 5. Operator_avs_registration_windows: Ranges that start and end on same day are invalid
// Note: We cannot assume that the operator is registered for avs at end_time because it is
// Payments calculations should only be done on snapshots from the PREVIOUS DAY. For example say we have the following:
// <-----0-------1-------2------3------>
// ^ ^
// Entry Exit
// Since exits (deregistrations) are rounded down, we must only look at the day 2 snapshot on a pipeline run on day 3.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.
- If an allocation and a non-zero deallocation happens on the same day (this is just on testnet), we should just use the deallocation and not round down. Rounding down would just give the user an extra day of stake.
- If an allocation and zero deallocation happens on the same day, then discard the entire allocation/deallocation pair
Again, these cases only exist on testnet since allocation/deallocation can happen in the same day. In practice mainnet would never have this problem.
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.
Actually it looks like we properly handle these cases based on the tests. I just want to leave this comment up to make sure you agree that we're not missing anything based on the what we did in the above logic too :)
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.
yes, tests properly handle allocation/deallocation on same day by taking latest record!
41d0383 to
691afe7
Compare
Description
1. Database Migration (
202511141700_withdrawalQueueAndAllocationRounding)queued_slashing_withdrawalstable with timing columns for 14-day withdrawal tracking:completed,completion_*- Completion trackingoperator_allocationstable with rounding support:effective_date- Rounded date when allocation/deallocation takes effect2. Allocation/Deallocation Rounding Logic (Sabine Fork)
Implements day-boundary rounding to prevent gaming and mimic existing registration behavior:
3. Dual Behavior Implementation
Maintains backward compatibility with fork-based behavior switching:
Type of change
How Has This Been Tested?
Unit tests
Checklist: