Skip to content

Conversation

hopeyen
Copy link
Contributor

@hopeyen hopeyen commented Jul 7, 2025

Why are these changes needed?

Get the refresh behavior back

It was removed/lost in a refactoring focused PR ed89588#diff-12b74ec71c12c2e17777a7dee108651d3a1cd184a32a2bb522a43c0f0baa204dL161

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@hopeyen hopeyen requested a review from a team as a code owner July 7, 2025 19:27
@hopeyen hopeyen requested a review from dmanc July 7, 2025 20:00
Copy link
Collaborator

@samlaf samlaf left a comment

Choose a reason for hiding this comment

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

LGTM. Although I feel like this code could be refactored to not mix circular buffer logic with business logic. Claude seems to agree:
image

Comment on lines +237 to +238
record = accountant.periodRecords.GetRelativePeriodRecord(overflowPeriod, quorumNumber)
assert.Equal(t, uint64(0), record.Usage)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are you not checking the overflowPeriod for the previous update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here the overflowPeriod got reset, so there's no previous update anymore

@hopeyen
Copy link
Contributor Author

hopeyen commented Jul 8, 2025

LGTM. Although I feel like this code could be refactored to not mix circular buffer logic with business logic. Claude seems to agree: image

I agree we can do this, I made a backlog ticket so we can work on this later on, and I'll go ahead to merge this fix for now

@hopeyen hopeyen merged commit 1091f46 into master Jul 8, 2025
17 of 18 checks passed
@hopeyen hopeyen deleted the hope/circular-wrapping branch July 8, 2025 11:55
dmanc pushed a commit that referenced this pull request Jul 9, 2025
* fix: period record circular wrapping refresh

* chore: add unit tests for period records getRelativePeriodRecord
dmanc added a commit that referenced this pull request Jul 9, 2025
fix: period record circular wrapping refresh (#1731)

* fix: period record circular wrapping refresh

* chore: add unit tests for period records getRelativePeriodRecord

Co-authored-by: hopeyen <[email protected]>
samlaf added a commit that referenced this pull request Jul 11, 2025
samlaf added a commit that referenced this pull request Jul 16, 2025
* Revert "refactor: account ledger interface incorporates debit slip (#1732)"

This reverts commit b4cf3d9.

* Revert "fix: period record circular wrapping refresh (#1731)"

This reverts commit 1091f46.

* Revert "fix: disperser_client v2 nil ptr bug (#1710)"

This reverts commit a5acd93.

* Revert "fix: disperser_client v2 should use default onDemandQuorums (#1700)"

This reverts commit 005e0a6.

* Revert "refactor: unify accountant debit slip (#1695)"

This reverts commit a9c0868.

* Revert "feat: AccountantLedger interface for next step unifications (#1694)"

This reverts commit 354e42f.

* Revert "feat: extract generic payment logic (#1693)"

This reverts commit 1003b98.

* Revert "fix: make disperser client backwards compatible (#1686)"

This reverts commit 26c612f.

* Revert "fix: payment state api doesn't fail at zero valued reservations/ondemand (#1682)"

This reverts commit 2031c1d.

* Revert "refactor: quorum specific metrics for metered bytes (#1668)"

This reverts commit 7ac688d.

* Revert "feat: GetPaymentStateForAllQuorums api impl (#1664)"

This reverts commit ed89588.

* Revert "refactor: core meterer consolidation (#1663)"

This reverts commit 319a865.

* Revert "refactor: onchain state interface (#1662)"

This reverts commit d795dfa.

* Revert "refactor: core meterer period record module (#1661)"

This reverts commit afbef90.

* Revert "refactor: payment common functions (#1653)"

This reverts commit 36a3d33.

* Revert "fix: period index calculation (#1641)"

This reverts commit c229748.

* Revert "refactor: use generic column name (#1626)"

This reverts commit 4d1994e.

* Revert "feat: payment onchain state interface (#1625)"

This reverts commit 61e46ac.

* Revert "feat: offchain quorum period record getter (#1620)"

This reverts commit a309139.

* Revert "feat: protobuf payment state api quorum specific variant (#1613)"

This reverts commit 58acdfd.

* Revert "fix: nil assignment to new account (#1612)"

This reverts commit bcbf9f5.

* Revert "feat: offchain batch writes (#1580)"

This reverts commit ce4d2f5.

* Revert "feat: validator authenticate on-demand request by disperser key (#1539)"

This reverts commit f545b16.

* fix: issues after series of reverts (scary)

* chore: add back ErrZeroSymbols error

This error was likely removed by mistake during a revert conflict fix from one of the previous set of reverted PRs.

* style: fix lint

* test: fix accountant_test hardcoded error string

* Revert "Revert "fix: period index calculation (#1641)""

This reverts commit b8767ae.
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