Skip to content

Conversation

@wolf31o2
Copy link
Member

@wolf31o2 wolf31o2 commented Nov 24, 2025

Summary by cubic

Fix account upsert behavior to correctly update delegation fields on conflict. When inserting an account with an existing staking_key, only pool and drep are updated, avoiding unintended overwrites of other columns.

Written for commit dd11149. Summary will update automatically on new commits.

Summary by CodeRabbit

  • Bug Fixes
    • Improved database conflict handling to prevent unintended data overwrites during staking key operations.

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
@wolf31o2 wolf31o2 requested a review from a team as a code owner November 24, 2025 21:52
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 24, 2025

📝 Walkthrough

Walkthrough

The change modifies the saveAccountIfNew function in the SQLite transaction plugin to adjust the OnConflict clause for the staking_key upsert operation. Instead of updating all columns when a conflict occurs, the modified behavior now updates only the "pool" and "drep" columns, while preserving the values of other fields from being overwritten.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Verify that "pool" and "drep" are the only columns intended to be updated upon staking_key conflict
  • Confirm that preserving other field values does not introduce data consistency issues
  • Ensure the narrowed update scope aligns with the account reconciliation logic

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: modifying how account updates are handled during database conflicts when inserting staking keys.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/db-account-conflict

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 040e5f2 and dd11149.

📒 Files selected for processing (1)
  • database/plugin/metadata/sqlite/transaction.go (1 hunks)
⏰ 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). (3)
  • GitHub Check: cubic · AI code reviewer
  • GitHub Check: nilaway
  • GitHub Check: Analyze (go)
🔇 Additional comments (1)
database/plugin/metadata/sqlite/transaction.go (1)

89-94: Verify intended behavior for fields excluded from conflict update.

The Account model contains fields (CertificateID, Active, AddedSlot) that are not included in the selective update. The original analysis incorrectly referenced "timestamps, balances" which don't exist in the model.

Confirm whether preserving CertificateID and Active on conflict is intentional:

  • CertificateID: May need to track the latest certificate that modified the account
  • Active: May need to reflect current delegation status from the certificate

Check if similar OnConflict patterns elsewhere in the file (lines 484-487, 514-517) handle these fields consistently.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 1 file

@wolf31o2 wolf31o2 merged commit 1819090 into main Nov 24, 2025
13 checks passed
@wolf31o2 wolf31o2 deleted the fix/db-account-conflict branch November 24, 2025 22:04
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.

3 participants