Skip to content

feat: increase delegated stakes#205

Open
gclaramunt wants to merge 1 commit intodevelopfrom
gabe/delegated-stakes-increase
Open

feat: increase delegated stakes#205
gclaramunt wants to merge 1 commit intodevelopfrom
gabe/delegated-stakes-increase

Conversation

@gclaramunt
Copy link
Contributor

No description provided.

Copy link

@ryle-ai ryle-ai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This PR adds support for displaying effective delegated stake amounts when stakes have been increased (via token lock replacement).

Changes Analysis

Schema Changes:

  • Adds current_token_lock_hash and current_amount to delegate_stake_create_events
  • Adds index on current_token_lock_hash for efficient lookups

Handler Logic:
The implementation uses null-coalescing to show "effective" values:

amount: event.current_amount ?? event.amount,
tokenLockHash: event.current_token_lock_hash ?? event.lock_reference_hash,

Observations

1. Potential Conflict with PR #204

There's an open PR #204 with similar intent but a different approach:

  • PR #204: Exposes currentTokenLockHash and currentAmount as additional fields
  • PR #205 (this PR): Replaces the original amount and tokenLockHash with effective values

These approaches have different trade-offs:

  • PR #205 is simpler for API consumers (single source of truth)
  • PR #204 preserves original data visibility in the response

Recommendation: Coordinate with the author of PR #204 to decide on the preferred approach. If PR #205's approach is chosen, consider adding original values as separate fields for audit purposes.

2. Missing Migration File

PR #204 includes SQL migration files (prisma/migrations/20260109/*.sql) but this PR only has schema changes. Will Prisma generate the migration automatically, or is a manual migration file needed for consistency?

3. Documentation Update

The OpenAPI schema update (docs/schemas/delegatedStaking.yml) is well done with clear descriptions explaining the "effective" semantics.

Test Coverage

The test updates correctly handle the effective amount logic:

const effectiveAmount = match.current_amount ?? match.amount;
expect(tx.amount).toBeBigInt(effectiveAmount);

No Critical Issues Found

The logic is correct and the tests validate the expected behavior.

Verdict: Request clarification on the coordination with PR #204 and migration strategy before merging.


Reviewed against block_explorer codebase

fee: event.fee,
tokenLockHash: event.lock_reference_hash,
tokenLockHash: event.current_token_lock_hash ?? event.lock_reference_hash,
parentHash: event.parent_hash,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This applies the effective tokenLockHash for creates, but delegateStakeWithdrawResponse (line ~64-73) isn't updated to do the same. The snapshot-streaming side (PR #114, SnapshotDAO.scala) inserts current_lock_reference_hash and current_amount into delegate_stake_withdraw_events too (via PendingDelegatedStakeWithdrawal.currentTokenLockRef from tessellation delegatedStake.scala:147-148). So withdrawal responses should also show effective amounts if a withdrawal happens on an increased stake.

I think you need the same null-coalescing pattern in delegateStakeWithdrawResponse:

currentTokenLockHash: event.current_token_lock_hash ?? event.lock_reference_hash, // or however the field maps
currentAmount: event.current_amount ?? event.amount,

(Though note the column naming issue -- see my comment on snapshot-streaming PR #114 re: current_lock_reference_hash vs current_token_lock_hash.)

current_amount BigInt?

global_snapshot global_snapshots @relation(fields: [global_snapshot_hash], references: [hash], onDelete: Cascade, onUpdate: Restrict)
addresses addresses @relation(fields: [source_addr], references: [address], onDelete: Cascade, onUpdate: Restrict)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: the column name -- snapshot-streaming PR #114 creates this column as current_lock_reference_hash in the SQL schema (sql/delegated_staking.sql), not current_token_lock_hash. These need to be aligned or the Prisma client won't find the data that snapshot-streaming writes. Same issue for the delegate_stake_withdraw_events table below and the replacement_hash on dag_token_locks (which SS calls replace_token_lock_ref).

I'd suggest coordinating with the SS PR author on a single naming convention. The tessellation Scala types use currentTokenLockRef on DelegatedStakeRecord and replaceTokenLockRef on TokenLock, so current_token_lock_hash and replace_token_lock_ref might be reasonable compromises -- but the important thing is they match across repos.

lockAmount: change.current_amount ?? change.amount,
rewardsAccrued:
change.delegate_stake_total_rewards?.delegate_stake_total_rewards ?? 0,
withdrawnAmount: completedAmount(change),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lockAmount gets the effective amount here, but the position response also has an implicit tokenLockHash that still uses the original lock_reference_hash (not shown in the diff since it's unchanged). For consistency with the create response above, I think delegateStakePositionResponse should also resolve the effective token lock hash -- otherwise you'd get a position showing the increased amount but referencing the original (now-replaced) token lock, which is confusing for API consumers.

Copy link

@ryle-ai ryle-ai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overlaps a lot with #204 -- both PRs add current_token_lock_hash and current_amount to the schema, but differ in how they surface it to the API. This one replaces amount/tokenLockHash with the effective values via null-coalescing, while #204 exposes them as separate additional fields. I think the approaches should be reconciled before either merges, since they'll conflict otherwise.

The null-coalescing approach here is clean but I think the withdrawal response is missing the same treatment -- delegateStakeWithdrawResponse still returns the original values even though snapshot-streaming writes current_token_lock_hash / current_amount into delegate_stake_withdraw_events too. Same for delegateStakePositionResponse which gets the effective amount for lockAmount but still uses the original lock_reference_hash for the token lock hash. Also flagged in the inline comments -- the column naming needs to align with what snapshot-streaming PR #114 actually writes, or Prisma won't find the data.

No migration file included here (unlike #204 which has the SQL files). Might be fine if Prisma generates it automatically, but worth confirming.

Copy link

@ryle-ai ryle-ai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on the earlier review -- I want to flag the coordination issue with #204 more explicitly now that I've looked at both approaches in depth.

The core trade-off between the two PRs is about what amount and tokenLockHash mean in the API response. This PR uses null-coalescing so those fields transparently return the current effective values. That's nice for consumers since they always see what's actually in effect without having to know about replacement mechanics. But it has a cost: the original token lock reference hash is a distinct cryptographic reference that has meaning on its own -- it identifies the original lock that was created, which is different from the replacement lock even though the replacement supersedes it. Collapsing the two into a single field loses that provenance. And if anyone downstream currently relies on amount being the original staked amount (for audit trails, historical analysis, reconciliation against on-chain data), this silently changes the semantics of an existing field, which is a subtle kind of breakage.

PR #204 takes the additive approach -- currentTokenLockHash and currentAmount as separate fields alongside the originals. Existing fields keep their original meaning, and consumers who need effective values can read the new fields. It's more verbose in the response, but it's backwards compatible and preserves all the information. Nobody's code breaks and nobody loses data.

I'd lean towards #204's separate-fields approach being the more correct one here. The original lock reference hash genuinely is different information from the replacement lock reference hash, and preserving both seems right. But this is a team call -- if the consensus is that consumers should never need to see original values once a stake has been increased, then the null-coalescing approach is cleaner. Either way, these two PRs will conflict if they both merge independently, so it would be good to coordinate with @effects-ai on a single approach before either goes in.

Copy link

@ryle-ai ryle-ai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: feat: increase delegated stakes

This PR changes delegateStakeCreateResponse and delegateStakePositionResponse to return "effective" values (current amount/lock hash if stake was increased, otherwise originals) using null-coalescing. There are three issues that should be addressed before merging.

1. Silent semantic change to existing API fields (breaking change)

amount and tokenLockHash in the create response (src/handlers/delegatedStakingHandler.ts:50-52) currently return the original staked values. This PR changes them to return different data (the replacement lock's values) without any API versioning, deprecation notice, or new field names. Any downstream consumer that relies on amount being the originally staked amount -- for reconciliation, audit trails, or matching against on-chain create events -- will silently get different data.

The null-coalescing pattern (event.current_amount ?? event.amount) is clean, but it's replacing what an existing field means. PR #204 takes the additive approach of exposing currentTokenLockHash and currentAmount as separate fields alongside the originals, which is backwards-compatible. I'd recommend the additive approach from #204 over this one, or at minimum coordinating so only one approach merges.

2. Incomplete application to withdrawal and position responses

The null-coalescing is applied to delegateStakeCreateResponse and delegateStakePositionResponse.lockAmount, but:

  • delegateStakeWithdrawResponse (src/handlers/delegatedStakingHandler.ts:64-73) is not updated. Snapshot-streaming (PR #114) writes current_token_lock_hash and current_amount into delegate_stake_withdraw_events as well. If a withdrawal occurs on an increased stake, the withdrawal response will show stale values.

  • delegateStakePositionResponse (src/handlers/delegatedStakingHandler.ts:86-106) gets the effective amount for lockAmount but doesn't expose a token lock hash field at all. This is less critical since the position response uses stakeHash (the create event hash) rather than the lock reference hash, but it's worth confirming this is intentional -- a consumer seeing an increased lockAmount has no way to discover which token lock backs it.

3. Column naming must match snapshot-streaming

The Prisma schema adds current_token_lock_hash and current_amount to delegate_stake_create_events (prisma/schema.prisma:610). The trusted reviewer on this PR noted that snapshot-streaming PR #114 may create this column as current_lock_reference_hash in its SQL schema. If the column names don't match, Prisma will generate queries against columns that don't exist in the database. The null-coalescing fallback would mask this failure -- the fields would always be null, so the API would silently return original values, and the "increase" feature would appear to not work without any error.

Please confirm alignment with the snapshot-streaming schema before merging.

Copy link

@ryle-ai ryle-ai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two additional observations not covered in the prior review.

completedAmount uses original amount, not effective amount

completedAmount (line 80) computes withdrawnAmount using change.amount — the original staked amount. But after this PR, lockAmount in the same position response uses change.current_amount ?? change.amount — the effective amount. If a stake is increased from 3.3T to 5T and then withdrawn, the position response will show lockAmount: 5000000000000 but withdrawnAmount: 3333300000000 + rewards. That's internally inconsistent within the same response object — the withdrawn amount should reflect the effective lock amount, not the original.

completedAmount should use the same effective amount: (change.current_amount ?? change.amount) + change.total_rewards_view?.total_rewards.

Withdrawal response leaks raw DB columns via stake field

delegateStakeWithdrawResponse (line 67) passes the raw Prisma object event.delegate_stake_create_event directly into the response as the stake field. Unlike delegateStakeCreateResponse, which carefully maps DB columns to API field names, the withdrawal response includes every column from the Prisma model as-is. Adding current_token_lock_hash and current_amount to the model means these raw column names will now appear in the stake object of every withdrawal API response — uncoalesced, untransformed, and using snake_case DB naming instead of the camelCase API convention.

This is a pre-existing design issue (the stake field has always leaked raw columns like source_addr, lock_reference_hash, etc.), but this PR makes it worse by adding new columns that are semantically sensitive. A consumer seeing current_amount: 5000000000000 alongside amount: 3333300000000 in the raw stake object, while the create endpoint returns the coalesced value as amount: 5000000000000, will be confused about which is authoritative.

Neither of these is a merge blocker on its own — the completedAmount issue only manifests when an increased stake is withdrawn, and the raw leak is pre-existing. But they're worth addressing.

status: stakeStatus(change),
stakeHash: change.hash,
lockAmount: change.amount,
lockAmount: change.current_amount ?? change.amount,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lockAmount now shows the effective amount, but withdrawnAmount on line 98 is computed by completedAmount (line 80) which still uses change.amount — the original. For an increased stake that gets withdrawn, these will be inconsistent within the same response. completedAmount should use (change.current_amount ?? change.amount) to match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants