Skip to content

No zeros#313

Closed
n8mgr wants to merge 1 commit into
masterfrom
nate/no-zeros
Closed

No zeros#313
n8mgr wants to merge 1 commit into
masterfrom
nate/no-zeros

Conversation

@n8mgr

@n8mgr n8mgr commented Jun 13, 2025

Copy link
Copy Markdown
Member

Events that do not change a wallet's balance should not be indexed by walletd.

@github-project-automation github-project-automation Bot moved this to In Progress in Sia Jun 13, 2025

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR prevents walletd from indexing events that have zero-value effects on the wallet’s balance.

  • Added non-zero checks before emitting events for Siacoin, Siafund, file contract outputs, miner payouts, and foundation subsidies
  • Refactored file contract output loops to filter out zero-value outputs early
  • Documented the fix in the changeset

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
wallet/wallet.go Added !Value.IsZero() (or != 0) guards on all event additions to skip zero-value outputs
.changeset/fixed_an_issue_with_zero_value_events_being_added_to_the_wallet.md Added a changelog entry for the zero-value event fix
Comments suppressed due to low confidence (2)

.changeset/fixed_an_issue_with_zero_value_events_being_added_to_the_wallet.md:5

  • [nitpick] Use the imperative mood in changelog titles (e.g., "Fix an issue with zero-value events being added to the wallet") to align with conventional commit and changeset conventions.
# Fixed an issue with zero value events being added to the wallet.

wallet/wallet.go:394

  • This closing brace may be extraneous or misaligned, which could alter the intended control flow—please verify block delimiters and remove any unnecessary braces.
}

Comment thread wallet/wallet.go
Comment thread wallet/wallet.go

@peterjan peterjan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's slightly cleaner to change relevant to take an output and then check both the address and value there. It's also missing testing 😬 but I guess we just want to get this out.

@ChrisSchinnerl ChrisSchinnerl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks alright but has no testing

@ChrisSchinnerl

Copy link
Copy Markdown
Member

@n8maninger do you still intend to merge this?

@n8mgr n8mgr closed this Aug 12, 2025
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Sia Aug 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants