Skip to content

Conversation

@wolf31o2
Copy link
Member

@wolf31o2 wolf31o2 commented Dec 4, 2025


Summary by cubic

Add includeInactive support to account, pool, and drep lookups so callers can fetch inactive records when needed. Default behavior remains active-only; callers updated to pass false.

  • New Features

    • Added includeInactive to GetAccount, GetPool, and GetDrep in Database and MetadataStore.
    • SQLite queries filter active=true when includeInactive is false; pools also check latest registration vs retirement.
    • Preloaded Pool Retirement for accurate active/retired checks.
    • Added models.ErrDrepNotFound.
  • Migration

    • Update all call sites to pass the new includeInactive flag.
    • Internal callers (transactions, ledger view, benchmarks) updated to use false.
    • No schema changes.

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

Summary by CodeRabbit

  • New Features

    • Added an option to include or exclude inactive entities (accounts, pools, DREPs) when fetching data.
  • Bug Fixes

    • Clearer not-found behavior for missing DREPs and pools; improved handling of absent results.
  • Tests

    • Benchmarks and tests updated to pass the new include/exclude inactive flag.

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

@wolf31o2 wolf31o2 requested a review from a team as a code owner December 4, 2025 00:52
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 4, 2025

📝 Walkthrough

Walkthrough

The PR adds an includeInactive bool parameter to metadata getters (GetAccount, GetPool, GetDrep) across Database and MetadataStore layers and updates all call sites to pass that flag. SQLite metadata implementations use the flag to conditionally filter out inactive records (accounts, pools with retirement/AddedSlot logic, drep lookup). GetDrep’s signature and return semantics change to return models.Drep (value) and the new package-level error ErrDrepNotFound is introduced. Benchmarks and internal callers are updated to pass false to preserve prior behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

  • Repetitive signature additions reduce surface area, but each ABI change requires updating and verifying call sites.
  • Files needing focused review:
    • database/plugin/metadata/sqlite/pool.go — retirement/AddedSlot filtering logic and preloads.
    • database/plugin/metadata/sqlite/drep.go and database/models/drep.go — change from pointer to value return and introduction of ErrDrepNotFound.
    • transaction paths in database/plugin/metadata/sqlite/transaction.go — many updated call sites; ensure includeInactive propagation is correct.
    • Public Database methods (database/account.go, database/pool.go, database/drep.go) — verify transaction handling and error propagation remain correct.

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 PR title directly and accurately summarizes the main change: adding support for retrieving inactive items across account, pool, and drep lookups.
✨ 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 feat/get-inactive

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
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
database/plugin/metadata/sqlite/drep.go (1)

24-28: Consider adding documentation for the includeInactive parameter.

The function comment is minimal. Consider expanding it to explain the purpose of includeInactive:

// GetDrep retrieves a DRep by credential.
// If includeInactive is false, only active DReps are returned.
// If includeInactive is true, both active and inactive DReps are returned.
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0a2b68c and c2ef475.

📒 Files selected for processing (11)
  • database/account.go (1 hunks)
  • database/drep.go (1 hunks)
  • database/models/drep.go (1 hunks)
  • database/plugin/metadata/sqlite/account.go (1 hunks)
  • database/plugin/metadata/sqlite/drep.go (1 hunks)
  • database/plugin/metadata/sqlite/pool.go (3 hunks)
  • database/plugin/metadata/sqlite/transaction.go (7 hunks)
  • database/plugin/metadata/store.go (3 hunks)
  • database/pool.go (1 hunks)
  • ledger/benchmark_test.go (6 hunks)
  • ledger/view.go (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-24T21:56:15.978Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 975
File: database/models/transaction.go:21-26
Timestamp: 2025-10-24T21:56:15.978Z
Learning: In database/models/transaction.go, the Transaction struct uses a dual foreign-key pattern for UTXO associations: Outputs []Utxo uses TransactionID→ID (where UTXOs were created), and Inputs []Utxo uses SpentAtTxId→Hash (where UTXOs were consumed). Explicit GORM tags `gorm:"foreignKey:SpentAtTxId;references:Hash"` for Inputs and `gorm:"foreignKey:TransactionID;references:ID"` for Outputs prevent association conflicts and preserve UTXO provenance.

Applied to files:

  • database/plugin/metadata/sqlite/transaction.go
📚 Learning: 2025-10-22T21:29:26.924Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: ledger/state_test.go:248-248
Timestamp: 2025-10-22T21:29:26.924Z
Learning: For tests under ledger/, prefer constructing a minimal ledger Block (MockBlock mirroring chain/chain_test.go) and pass it to Chain.AddBlock, rather than using database/models.Block or relying on models.Block.Decode().

Applied to files:

  • ledger/benchmark_test.go
📚 Learning: 2025-10-22T20:19:26.360Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:91-91
Timestamp: 2025-10-22T20:19:26.360Z
Learning: In utxorpc/sync_test.go, tests should not construct database/models.Block directly for sync server use. Instead, build a models.Block only as a container for CBOR and call models.Block.Decode() to obtain a github.com/blinklabs-io/gouroboros/ledger/common.Block (or the repository’s ledger.Block interface), and use that decoded value in the test.

Applied to files:

  • ledger/benchmark_test.go
📚 Learning: 2025-10-22T21:28:48.396Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 971
File: utxorpc/sync_test.go:339-339
Timestamp: 2025-10-22T21:28:48.396Z
Learning: In utxorpc/sync_test.go, tests must not pass database/models.Block into chain.Chain.AddBlock. They should call models.Block.Decode() and pass the resulting github.com/blinklabs-io/gouroboros/ledger/common.Block to AddBlock.

Applied to files:

  • ledger/benchmark_test.go
🧬 Code graph analysis (6)
database/plugin/metadata/sqlite/account.go (1)
database/models/account.go (1)
  • Account (27-35)
database/plugin/metadata/sqlite/drep.go (1)
database/models/drep.go (1)
  • Drep (25-33)
database/drep.go (2)
database/txn.go (1)
  • Txn (27-34)
database/models/drep.go (1)
  • Drep (25-33)
database/pool.go (1)
database/models/pool.go (1)
  • Pool (26-38)
database/plugin/metadata/sqlite/pool.go (1)
database/models/account.go (2)
  • Registration (72-78)
  • Registration (80-82)
database/account.go (3)
database/txn.go (1)
  • Txn (27-34)
database/models/account.go (1)
  • Account (27-35)
database/models/transaction.go (2)
  • Transaction (20-39)
  • Transaction (41-43)
⏰ 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). (9)
  • GitHub Check: cubic · AI code reviewer
  • GitHub Check: go-test (1.25.x, windows-latest)
  • GitHub Check: go-test (1.24.x, ubuntu-latest)
  • GitHub Check: go-test (1.24.x, windows-latest)
  • GitHub Check: go-test (1.24.x, macos-latest)
  • GitHub Check: go-test (1.25.x, macos-latest)
  • GitHub Check: lint
  • GitHub Check: nilaway
  • GitHub Check: Analyze (go)
🔇 Additional comments (13)
database/models/drep.go (1)

17-23: LGTM!

The addition of ErrDrepNotFound follows Go conventions for sentinel errors and is consistent with the existing error patterns in the codebase (e.g., ErrAccountNotFound, ErrPoolNotFound).

database/plugin/metadata/sqlite/account.go (1)

26-47: LGTM!

The conditional filtering logic is correct: when includeInactive is false, the query filters for active = true, restricting results to active accounts only. The implementation is clean and consistent with similar patterns in the codebase.

database/plugin/metadata/sqlite/pool.go (2)

40-43: Good addition of Retirement preload.

The new Retirement preload is necessary for the post-query filtering logic that determines pool activity status.


55-62: Pool activity filtering logic is correct.

The filtering logic properly implements Cardano pool activity semantics. A pool is correctly treated as inactive when:

  1. No registration exists, or
  2. The most recent retirement's AddedSlot exceeds the most recent registration's AddedSlot

Comparing AddedSlot values (block slot numbers) accurately determines certificate recency. All production callers use includeInactive=false, confirming this behavior aligns with expected usage.

database/plugin/metadata/sqlite/transaction.go (3)

106-106: LGTM!

The includeInactive parameter is correctly set to false, ensuring certificate processing only operates on active accounts. This maintains existing behavior.


526-526: LGTM!

Both GetPool calls correctly pass false for includeInactive, ensuring only active pools are considered during certificate processing. This maintains existing behavior.

Also applies to: 626-626


654-654: LGTM!

Both GetAccount calls correctly pass false for includeInactive, maintaining the existing behavior of only operating on active accounts during deregistration certificate processing.

Also applies to: 689-689

ledger/view.go (1)

84-84: LGTM!

Passing false for includeInactive is correct for PoolCurrentState, which specifically retrieves the current active pool registration certificate, as documented in the function comment on line 79-80.

database/pool.go (1)

23-40: LGTM!

The implementation correctly:

  1. Adds the includeInactive parameter to control filtering behavior
  2. Forwards it to the metadata layer
  3. Returns models.ErrPoolNotFound when the pool is not found

This provides a consistent error handling pattern for pool lookups.

database/account.go (1)

22-43: LGTM!

The implementation correctly adds the includeInactive parameter and forwards it to the metadata layer. The nil check properly returns models.ErrAccountNotFound when the account is not found, providing consistent error handling.

database/plugin/metadata/store.go (1)

46-74: LGTM! Interface changes are consistent.

The three getter methods (GetPool, GetAccount, GetDrep) have been consistently updated with the includeInactive boolean parameter. The inline comments provide helpful clarity about the parameter's purpose.

ledger/benchmark_test.go (1)

520-520: LGTM! Benchmark call sites properly updated.

All six benchmark functions have been correctly updated to pass false for the new includeInactive parameter, maintaining the existing behavior of excluding inactive items. The changes are consistent across:

  • BenchmarkAccountLookupByStakeKeyNoData and BenchmarkAccountLookupByStakeKeyRealData
  • BenchmarkPoolLookupByKeyHashNoData and BenchmarkPoolLookupByKeyHashRealData
  • BenchmarkDRepLookupByKeyHashNoData and BenchmarkDRepLookupByKeyHashRealData

Also applies to: 594-594, 624-624, 698-698, 728-728, 803-803

database/plugin/metadata/sqlite/drep.go (1)

31-38: Query building logic is correct, and all call sites have been updated.

The implementation properly handles the includeInactive parameter and transaction context. The active filter is correctly applied when includeInactive is false. All GetDrep() call sites (benchmark tests and the database wrapper) have been updated with the new parameter.

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 11 files

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
ledger/benchmark_test.go (1)

518-523: Benchmarks correctly updated to pass includeInactive=false

All benchmark call sites now supply the new includeInactive flag as false for GetAccount, GetPool, and GetDrep, which preserves the prior “active-only” behavior while matching the updated APIs. If you ever want to measure worst-case query performance over inactive history, you could add separate benchmarks with includeInactive = true, but it’s not required for this PR.

Also applies to: 592-597, 622-627, 696-701, 727-731, 801-805

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c2ef475 and c6b00d0.

📒 Files selected for processing (11)
  • database/account.go (1 hunks)
  • database/drep.go (1 hunks)
  • database/models/drep.go (1 hunks)
  • database/plugin/metadata/sqlite/account.go (1 hunks)
  • database/plugin/metadata/sqlite/drep.go (2 hunks)
  • database/plugin/metadata/sqlite/pool.go (3 hunks)
  • database/plugin/metadata/sqlite/transaction.go (7 hunks)
  • database/plugin/metadata/store.go (3 hunks)
  • database/pool.go (1 hunks)
  • ledger/benchmark_test.go (6 hunks)
  • ledger/view.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • database/pool.go
  • database/account.go
  • database/plugin/metadata/sqlite/pool.go
  • ledger/view.go
🧰 Additional context used
🧬 Code graph analysis (4)
database/plugin/metadata/sqlite/account.go (1)
database/models/account.go (1)
  • Account (27-35)
database/drep.go (1)
database/models/drep.go (2)
  • Drep (25-33)
  • ErrDrepNotFound (23-23)
database/plugin/metadata/store.go (1)
database/models/drep.go (1)
  • Drep (25-33)
database/plugin/metadata/sqlite/drep.go (1)
database/models/drep.go (1)
  • Drep (25-33)
⏰ 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). (6)
  • GitHub Check: nilaway
  • GitHub Check: go-test (1.25.x, macos-latest)
  • GitHub Check: go-test (1.25.x, windows-latest)
  • GitHub Check: go-test (1.24.x, macos-latest)
  • GitHub Check: go-test (1.24.x, windows-latest)
  • GitHub Check: Analyze (go)
🔇 Additional comments (7)
database/models/drep.go (1)

17-23: ErrDrepNotFound definition looks good

Explicit ErrDrepNotFound plus the new errors import is straightforward and consistent with typical model-level not-found errors. No issues here.

database/drep.go (1)

17-39: GetDrep wiring and not-found handling are consistent

The new GetDrep method correctly mirrors the existing Database patterns: it owns transaction lifecycle when txn is nil, delegates to metadata.GetDrep, and normalizes a nil result to models.ErrDrepNotFound. That gives a clean, consistent API surface.

database/plugin/metadata/sqlite/account.go (1)

28-39: includeInactive flag handling in GetAccount is sound

Conditionally scoping the query with WHERE active = true when includeInactive is false cleanly preserves the existing “active-only” behavior while allowing future callers to opt into inactive records. The not-found behavior (returning nil, nil) remains consistent.

database/plugin/metadata/sqlite/drep.go (1)

18-45: SQLite GetDrep correctly implements includeInactive and not-found semantics

Using a shared query rooted at txn (or d.DB() when nil) plus the conditional active = true filter mirrors the account getter and cleanly supports active-only vs. all-DRep lookups. Returning (nil, nil) on gorm.ErrRecordNotFound is appropriate given that the Database layer normalizes this to models.ErrDrepNotFound.

database/plugin/metadata/sqlite/transaction.go (2)

101-118: Confirm intended semantics of using includeInactive=false for internal account/pool lookups

All internal call sites now pass includeInactive = false when fetching accounts and pools (get-or-create path and deregistration/retirement paths). This means:

  • Certificate handling only “sees” currently-active rows; previously, inactive rows would also have been returned by these getters.
  • For already-deregistered/retired entities, subsequent certs will behave as if there is no current record and will rely on upsert behavior to reconcile state.

Given ledger rules, you shouldn’t get repeated deregistrations/retirements for the same key without reactivation in between, so this is probably safe, but it is a behavioral change. Please sanity-check this against your expected Cardano invariants and any historical data quirks.

Also applies to: 524-527, 626-627, 653-656, 688-691


210-213: Formatting-only changes around upsert error and Redeemer ExUnits

The rewrapped fmt.Errorf("failed to fetch transaction ID after upsert: %w", err) block and the multi-line ExUnitsMemory/ExUnitsCPU initializers keep the same logic and just improve readability. No behavioral impact.

Also applies to: 488-493

database/plugin/metadata/store.go (1)

47-51: All MetadataStore implementations have been updated correctly. The SQLite plugin now matches the interface signatures: GetPool(lcommon.PoolKeyHash, bool, *gorm.DB), GetAccount([]byte, bool, *gorm.DB), and GetDrep([]byte, bool, *gorm.DB) all with proper return types and includeInactive flag usage.

agaffney
agaffney previously approved these changes Dec 4, 2025
agaffney
agaffney previously approved these changes Dec 4, 2025
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
database/plugin/metadata/sqlite/account.go (1)

51-61: active parameter in SetAccount is unused and may desync the Active flag

SetAccount accepts an active bool but never assigns it to models.Account.Active, so callers cannot actually control the stored Active state via this API, even though GetAccount now filters on active. This is likely to cause confusing or incorrect behavior around active/inactive accounts.

Consider wiring the parameter through:

 tmpItem := models.Account{
-    StakingKey: stakeKey,
-    AddedSlot:  slot,
-    Pool:       pkh,
-    Drep:       drep,
+    StakingKey: stakeKey,
+    AddedSlot:  slot,
+    Pool:       pkh,
+    Drep:       drep,
+    Active:     active,
 }

and verifying that any callers pass the intended value.

♻️ Duplicate comments (1)
database/drep.go (1)

21-35: GetDrep semantics are inconsistent with GetPool/GetAccount and don’t use ErrDrepNotFound

Right now GetDrep:

  • Returns a value (models.Drep) instead of *models.Drep, unlike GetPool/GetAccount.
  • Simply propagates the metadata error and never maps a not‑found condition to models.ErrDrepNotFound.

This makes the DRep path behave differently from the others and leaves the new sentinel error effectively unused. I’d recommend either:

  1. Aligning with the existing patterns:
-func (d *Database) GetDrep(
-    cred []byte,
-    includeInactive bool,
-    txn *Txn,
-) (models.Drep, error) {
+func (d *Database) GetDrep(
+    cred []byte,
+    includeInactive bool,
+    txn *Txn,
+) (*models.Drep, error) {
     if txn == nil {
         txn = d.Transaction(false)
         defer txn.Commit() //nolint:errcheck
     }
-    ret, err := d.metadata.GetDrep(cred, includeInactive, txn.Metadata())
-    if err != nil {
-        return models.Drep{}, err
-    }
-    return ret, nil
+    ret, err := d.metadata.GetDrep(cred, includeInactive, txn.Metadata())
+    if err != nil {
+        return nil, err
+    }
+    if ret == nil {
+        return nil, models.ErrDrepNotFound
+    }
+    return ret, nil
 }

…with the corresponding pointer return in the MetadataStore and sqlite implementation; or

  1. If you prefer value semantics, at least normalize not‑found errors (e.g., wrap gorm.ErrRecordNotFound as models.ErrDrepNotFound) so callers don’t need to reach into storage‑specific errors.
🧹 Nitpick comments (3)
database/models/drep.go (1)

17-23: Sentinel error looks good; consider wiring it into lookup paths

Defining ErrDrepNotFound as a package‑level sentinel is idiomatic and fine. As a follow‑up, ensure your DRep retrieval code (sqlite + Database.GetDrep) actually maps “not found” conditions to this error so callers can rely on a stable domain error instead of gorm.ErrRecordNotFound.

database/plugin/metadata/sqlite/drep.go (1)

29-42: IncludeInactive logic is correct; consider simplifying and mapping not‑found to ErrDrepNotFound

The includeInactive filtering and transaction handling look good. Two small improvements you might consider:

  • You don’t need both ret and tmpDrep; a single variable is enough:
-func (d *MetadataStoreSqlite) GetDrep(
-    cred []byte,
-    includeInactive bool,
-    txn *gorm.DB,
-) (models.Drep, error) {
-    ret := models.Drep{}
-    tmpDrep := models.Drep{}
+func (d *MetadataStoreSqlite) GetDrep(
+    cred []byte,
+    includeInactive bool,
+    txn *gorm.DB,
+) (models.Drep, error) {
+    var drep models.Drep
@@
-    if result := query.First(&tmpDrep, "credential = ?", cred); result.Error != nil {
-        return ret, result.Error
-    }
-    ret = tmpDrep
-    return ret, nil
+    if result := query.First(&drep, "credential = ?", cred); result.Error != nil {
+        return models.Drep{}, result.Error
+    }
+    return drep, nil
 }
  • Given models.ErrDrepNotFound now exists, you may also want to translate gorm.ErrRecordNotFound into that sentinel here so higher layers don’t depend on gorm’s error type.
database/plugin/metadata/sqlite/transaction.go (1)

102-118: Verify that always passing includeInactive=false matches desired certificate semantics

All internal metadata lookups here now explicitly use includeInactive=false:

  • getOrCreateAccountGetAccount(stakeKey, false, txn)
  • Pool registration and retirement certificates → GetPool(..., false, txn)
  • Stake deregistration and generic deregistration certificates → GetAccount(stakeKey, false, txn)

This keeps behavior “active-only”, but it also means:

  • Previously inactive accounts/pools are treated as “not found” by these helpers, even if a row exists.
  • In the pool retirement and (re-)registration paths, any state stored on an inactive row won’t be loaded and will instead be rebuilt from scratch or treated as missing.

Please double-check that for re-registration, multiple deregistrations, and retiring already-retired pools, this “ignore inactive rows” behavior is what you want, especially now that Active is part of your model and Get* APIs.

Also applies to: 629-630, 735-736, 767-768, 805-806

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c6b00d0 and c61387b.

📒 Files selected for processing (11)
  • database/account.go (1 hunks)
  • database/drep.go (1 hunks)
  • database/models/drep.go (1 hunks)
  • database/plugin/metadata/sqlite/account.go (1 hunks)
  • database/plugin/metadata/sqlite/drep.go (1 hunks)
  • database/plugin/metadata/sqlite/pool.go (3 hunks)
  • database/plugin/metadata/sqlite/transaction.go (6 hunks)
  • database/plugin/metadata/store.go (3 hunks)
  • database/pool.go (1 hunks)
  • ledger/benchmark_test.go (6 hunks)
  • ledger/view.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
database/plugin/metadata/sqlite/pool.go (1)
database/models/account.go (2)
  • Registration (72-78)
  • Registration (80-82)
database/plugin/metadata/sqlite/account.go (1)
database/models/account.go (1)
  • Account (27-35)
database/drep.go (3)
database/database.go (1)
  • Database (44-49)
database/txn.go (1)
  • Txn (27-34)
database/models/drep.go (1)
  • Drep (25-33)
database/account.go (1)
database/models/account.go (1)
  • Account (27-35)
database/plugin/metadata/sqlite/drep.go (1)
database/models/drep.go (1)
  • Drep (25-33)
⏰ 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). (8)
  • GitHub Check: Analyze (go)
  • GitHub Check: lint
  • GitHub Check: go-test (1.25.x, windows-latest)
  • GitHub Check: nilaway
  • GitHub Check: go-test (1.24.x, macos-latest)
  • GitHub Check: go-test (1.25.x, macos-latest)
  • GitHub Check: go-test (1.24.x, ubuntu-latest)
  • GitHub Check: go-test (1.24.x, windows-latest)
🔇 Additional comments (9)
database/pool.go (1)

22-39: GetPool includeInactive plumbing is consistent and preserves existing semantics

The new includeInactive parameter is correctly threaded through to metadata.GetPool, and the nil check/ErrPoolNotFound behavior remains intact. This should preserve existing callers’ behavior when they pass false while enabling the new inactive‑inclusive queries.

database/plugin/metadata/sqlite/pool.go (1)

25-63: Pool active/inactive filtering and preloading look correct

The sqlite GetPool implementation correctly:

  • Preloads the latest Registration and Retirement.
  • Returns (nil, nil) for missing pools.
  • Filters out logically inactive pools when includeInactive is false by comparing the latest Retirement.AddedSlot to the latest Registration.AddedSlot.

This aligns with Database.GetPool’s ErrPoolNotFound behavior and with how PoolCurrentState interprets the result.

database/account.go (1)

21-42: GetAccount includeInactive wiring matches GetPool pattern

The new includeInactive parameter is cleanly threaded through to the metadata store, and the existing not‑found handling via ErrAccountNotFound remains unchanged. This is consistent with the pool path and should preserve behavior for callers that pass false.

ledger/view.go (1)

81-88: PoolCurrentState correctly opts out of inactive pools

Updating the call to GetPool(pkh, false, lv.txn) keeps PoolCurrentState focused on active pools while preserving the existing ErrPoolNotFound handling. This matches the new includeInactive contract without changing the method’s observable behavior.

database/plugin/metadata/store.go (1)

46-74: MetadataStore interface extensions are coherent with implementation changes

Adding the includeInactive boolean to GetPool, GetAccount, and GetDrep here lines up with the updated database and sqlite implementations. Just ensure any other MetadataStore plugins are updated to this new signature so they continue to satisfy the interface.

database/plugin/metadata/sqlite/account.go (1)

26-40: includeInactive handling in GetAccount is clear and preserves prior behavior

Using a local query and conditionally adding active = true when includeInactive is false cleanly preserves the old behavior while enabling the new flag. Returning (nil, nil) on ErrRecordNotFound keeps existing call-site semantics.

database/plugin/metadata/sqlite/transaction.go (1)

480-495: Clamping ExUnits before uint64 cast is a good safety improvement

Using uint64(max(0, value.ExUnits.Memory)) / uint64(max(0, value.ExUnits.Steps)) avoids negative-to-unsigned underflow and makes the cast safe by construction. Just ensure your Go toolchain version supports the built-in max you’re relying on.

ledger/benchmark_test.go (2)

520-520: Benchmarks correctly pass includeInactive=false to preserve prior behavior

Updating the account and pool lookup benchmarks to call GetAccount(..., false, nil) and GetPool(..., false, nil) keeps behavior equivalent to the old signatures while exercising the new API shape. This matches the “internal callers default to active-only” goal.

Also applies to: 595-595, 624-624, 698-699


728-731: Review comment is incorrect—benchmarks are already aligned with GetDrep's actual error contract

The DRep benchmarks correctly check for gorm.ErrRecordNotFound because that is what GetDrep actually returns. The SQLite implementation passes through GORM's error directly (line 39 of database/plugin/metadata/sqlite/drep.go), and GORM returns gorm.ErrRecordNotFound when First() finds no matching record.

While models.ErrDrepNotFound exists as a sentinel in the codebase, it is not currently used by GetDrep and is not part of its error contract. The benchmarks require no changes.

Likely an incorrect or invalid review comment.

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