Skip to content

feat: add support for increased delegated stakes#204

Open
effects-ai wants to merge 1 commit intodevelopfrom
increase_delegated_stakes
Open

feat: add support for increased delegated stakes#204
effects-ai wants to merge 1 commit intodevelopfrom
increase_delegated_stakes

Conversation

@effects-ai
Copy link

No description provided.

@effects-ai effects-ai requested a review from gclaramunt January 12, 2026 00:31
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 tracking token lock replacement data (increased delegated stakes) by exposing additional fields in the API responses.

Approach Comparison with PR #205

There are two PRs addressing the same feature with different approaches:

Aspect PR #204 (this) PR #205
Fields added currentTokenLockHash, currentAmount as new fields Replaces amount/tokenLockHash with effective values
Original data Preserved alongside current values Hidden (only effective shown)
API compatibility Additive (backwards compatible) Breaking (changes existing field semantics)
Migrations Includes SQL migration files Schema-only changes

PR #204 Analysis

Strengths:

  1. Backwards compatible - existing consumers won't break
  2. Full data visibility - both original and current values available
  3. Proper migration files included

Issues Found:

1. Bug in delegateStakeWithdrawResponse

currentTokenLockHash: event.lock_reference_hash, // Should this be current_token_lock_hash?

This appears to use lock_reference_hash instead of current_token_lock_hash. The schema adds current_token_lock_hash but the handler doesn't use it.

2. Bug in delegateStakeCreateResponse

Same issue:

currentTokenLockHash: event.lock_reference_hash, // Should be event.current_token_lock_hash?

3. Missing Index

PR #205 adds an index on current_token_lock_hash for efficient lookups. This PR doesn't include that optimization.

Recommendation

  1. Coordinate with PR #205 author to agree on the preferred approach
  2. Fix the field mapping bugs (use current_token_lock_hash not lock_reference_hash)
  3. Consider adding the index from PR #205 if this approach is chosen

If the team prefers exposing both original and current values (this PR's approach), this is the more flexible option, but the bugs need fixing first.


Reviewed against block_explorer codebase

timestamp: event.created_at,
globalSnapshotHash: event.global_snapshot_hash,
globalSnapshotOrdinal: event.global_snapshot?.ordinal,
currentTokenLockHash: event.lock_reference_hash,
Copy link

Choose a reason for hiding this comment

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

This uses event.lock_reference_hash instead of event.current_token_lock_hash. The lock_reference_hash is the original token lock ref from the Create event (UpdateDelegatedStake.Create.tokenLockRef in tessellation), not the current/replacement one. So this will always return the original hash even when a stake has been increased.

Should be:

currentTokenLockHash: event.current_token_lock_hash,

(The previous review flagged this too -- just confirming with the tessellation type reference: DelegatedStakeRecord.currentTokenLockRef at delegatedStake.scala:127 on the feature/increase-delegated-stake branch is an Option[Hash] that's None for original stakes and Some(newLockHash) for increased stakes.)

timestamp: event.created_at,
globalSnapshotHash: event.global_snapshot_hash,
globalSnapshotOrdinal: event.global_snapshot?.ordinal,
currentTokenLockHash: event.lock_reference_hash,
Copy link

Choose a reason for hiding this comment

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

Same bug here -- event.lock_reference_hash should be event.current_token_lock_hash. The withdraw events table also has the current_token_lock_hash column (added in your migration 03_add_current_fields_to_delegate_stake_withdraw_events.sql), so we should be reading from it.

-- Add replacement_hash field to dag_token_locks

ALTER TABLE dag_token_locks
ADD COLUMN replacement_hash varchar NULL;
Copy link

Choose a reason for hiding this comment

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

re: naming -- snapshot-streaming PR #114 creates this column as replace_token_lock_ref in its SQL schema (sql/snapshot.sql:358), not replacement_hash. Whichever name we pick, both repos need to agree on it or the block_explorer won't be able to read data that snapshot-streaming writes.

I'd suggest replace_token_lock_ref since it matches the tessellation Scala type field name (TokenLock.replaceTokenLockRef at tokenLock.scala:90 on feature/increase-delegated-stake).

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.

Same overlap concern as #205 -- both PRs are adding the same schema fields but with different API approaches. This one exposes currentTokenLockHash and currentAmount as additional fields alongside the originals, which is more backwards-compatible. I think coordinating with the #205 author to pick one approach makes sense before merging either.

The main bug I see is in the handler mappings -- both delegateStakeCreateResponse and delegateStakeWithdrawResponse set currentTokenLockHash to event.lock_reference_hash, which is the original token lock ref, not the current/replacement one. Should be event.current_token_lock_hash instead. The column exists in the schema and the migration files add it, so I believe this is just a copy-paste issue.

Also flagged in inline comments -- the column naming for replacement_hash on dag_token_locks doesn't match what snapshot-streaming PR #114 uses (replace_token_lock_ref). Whichever name wins is fine but they need to agree across repos or the data won't line up.

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.

Same overlap concern as #205 -- both PRs are adding the same schema fields but with different API approaches. This one exposes currentTokenLockHash and currentAmount as additional fields alongside the originals, which is more backwards-compatible. I think coordinating with the #205 author to pick one approach makes sense before merging either.

The main bug I see is in the handler mappings -- both delegateStakeCreateResponse and delegateStakeWithdrawResponse set currentTokenLockHash to event.lock_reference_hash, which is the original token lock ref, not the current/replacement one. Should be event.current_token_lock_hash instead. The column exists in the schema and the migration files add it, so I believe this is a copy-paste issue.

Also flagged in inline comments -- the column naming for replacement_hash on dag_token_locks does not match what snapshot-streaming PR #114 uses (replace_token_lock_ref). Whichever name wins is fine but they need to agree across repos or the data will not line up.

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 here to connect the dots with #205. I posted a longer comment there comparing the two approaches -- the short version is that I think the separate-fields approach in this PR is probably the right call. The original token lock reference hash is meaningful in its own right, and silently replacing it with the current effective value (as #205 does) loses that provenance. Adding currentTokenLockHash and currentAmount as additional fields keeps the API backwards compatible and preserves all the information.

That said, the two PRs will conflict if they merge independently, so it'd be worth coordinating with @gclaramunt on settling on one approach before either goes in. The bugs I flagged in the earlier review (the lock_reference_hash / current_token_lock_hash mix-up in the handler mappings, and the column naming alignment with snapshot-streaming) still apply and should be fixed regardless of which approach wins.

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 of PR #204: feat: add support for increased delegated stakes

This PR adds currentTokenLockHash and currentAmount as new separate fields alongside the originals in the API responses, and adds replacement_hash to dag_token_locks. The additive approach (preserving original fields while exposing new ones) is a good design choice for backwards compatibility. However, there are confirmed bugs that mean the new fields will never return the correct data.

Bug: currentTokenLockHash reads the wrong column in both response functions

In src/handlers/delegatedStakingHandler.ts, both delegateStakeCreateResponse (line ~57) and delegateStakeWithdrawResponse (line ~73) set:

currentTokenLockHash: event.lock_reference_hash,

This reads the original lock_reference_hash column, not the new current_token_lock_hash column that was added to the Prisma schema. The intent is clearly to return the current/replacement token lock hash, but the code returns the original lock reference hash instead -- making currentTokenLockHash always identical to the existing tokenLockHash field. It should be:

currentTokenLockHash: event.current_token_lock_hash,

This is the primary bug -- the new Prisma columns are defined but never actually read anywhere in the handler code.

Withdraw events columns will always be NULL

The PR adds current_token_lock_hash and current_amount to delegate_stake_withdraw_events in both the Prisma schema and a migration file (03_add_current_fields_to_delegate_stake_withdraw_events.sql). However, examining the snapshot-streaming SnapshotDAO.scala (the system that writes to this database), the insertDelegatedStakingCreateWithdrawCommand only inserts: hash, source_addr, stake_create_hash, global_snapshot_hash, created_at_epoch, unlock_epoch, is_completed. It does not write current_token_lock_hash or current_amount to the withdraw events table.

The snapshot-streaming SQL schema (sql/delegated_staking.sql) for delegate_stake_withdraw_events also does not include these columns.

So even after fixing the column-read bug above, currentTokenLockHash and currentAmount in the withdraw response will always be null because nothing writes those values. The migration file adds columns that no writer populates.

If the intent is to show effective values on withdrawals, the data would need to come from the related delegate_stake_create_event (which is already included via Prisma relation), not from the withdraw event itself.

Position response not updated

delegateStakePositionResponse is not updated in this PR -- lockAmount still uses change.amount (the original amount). This is inconsistent: create responses expose currentAmount, but the position endpoint (which is likely the most-consumed view of a stake) does not. If the goal is to surface effective stake amounts, the position response should expose them too.

Column naming alignment with snapshot-streaming

The column names current_token_lock_hash, current_amount, and replacement_hash all match what snapshot-streaming uses in its SQL schema and SnapshotDAO. This is correctly aligned (the previous review concern about naming mismatch appears to have been based on an older version of snapshot-streaming, or was incorrect).

Summary

The currentTokenLockHash field reads lock_reference_hash instead of current_token_lock_hash in both response functions -- this is a clear copy-paste bug that makes the new field useless. The withdraw events columns are added but never written by snapshot-streaming. These need to be addressed before merge.

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: add support for increased delegated stakes

Bug: currentTokenLockHash reads the wrong column (both response functions)

In src/handlers/delegatedStakingHandler.ts, both delegateStakeCreateResponse (line 59) and delegateStakeWithdrawResponse (line 75) set:

currentTokenLockHash: event.lock_reference_hash,

This reads the original token lock reference hash -- the same value already exposed as tokenLockHash. The new current_token_lock_hash column added in the Prisma schema and migration files is never actually read. This makes currentTokenLockHash always identical to tokenLockHash, defeating the purpose of the feature.

Fix: change both lines to event.current_token_lock_hash.

Withdraw events columns will always be NULL

Migration 03_add_current_fields_to_delegate_stake_withdraw_events.sql adds current_token_lock_hash and current_amount to delegate_stake_withdraw_events. However, snapshot-streaming (the upstream writer) does not populate these columns on withdraw events -- its insertDelegatedStakingCreateWithdrawCommand only writes hash, source_addr, stake_create_hash, global_snapshot_hash, created_at_epoch, unlock_epoch, and is_completed.

So even after fixing the column-read bug above, the withdraw response's currentTokenLockHash and currentAmount will always be null. If the intent is to show effective values on withdrawals, the data should come from the related delegate_stake_create_event (already available via Prisma relation), not from the withdraw event row itself.

Summary

The additive API design (new fields alongside originals) is the right approach for backwards compatibility, but the two bugs above mean the new fields will never return correct data. The column-read bug is a straightforward fix; the withdraw events issue needs a decision on where to source the data from.

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