fix: make disperser client backwards compatible #1686

samlaf · 2025-06-26T10:27:26Z

Why are these changes needed?

#1664 made the current clients incompatible with our current testnet and mainnet network. This PR is an attempt to make them backwards compatible, but the conversion function was generated by Claude, so @hopeyen please take a look. We can discuss whether this approach makes sense at all.

If it doesn't, then we probably should not have merged this client to master until the testnet and mainnet dispersers are updated with the new disperser apiserver API.

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 :(

…sion

TODO: review the Claude generated getPaymentStateFromLegacyAPI... no idea if its OK or not.

samlaf · 2025-06-26T10:28:06Z

api/clients/v2/disperser_client.go

+// TODO: this was generated by Claude. I haven't checked that it actually makes sense.... SO DO NOT MERGE BEFORE CHECKING THIS.
+func convertLegacyPaymentStateToNew(legacyReply *disperser_rpc.GetPaymentStateReply) *disperser_rpc.GetPaymentStateForAllQuorumsReply {


@hopeyen can you take a look at this function please?

hopeyen

I think the check for conversion makes sense for backward compatibility, and left a few small comments

api/clients/v2/disperser_client.go

hopeyen · 2025-06-26T16:10:32Z

api/clients/v2/disperser_client.go

+	if legacyReply.Reservation != nil {
+		// Apply the reservation to all quorums mentioned in the reservation
+		quorums := legacyReply.Reservation.QuorumNumbers
+		if len(quorums) == 0 {


this should be invalid and error out

disperser/apiserver/server_v2.go

api/clients/v2/disperser_client.go

…tible

api/clients/v2/disperser_client.go

also GetPaymentStateForAllQuorumsReply fields

github-actions · 2025-06-27T15:01:45Z

The latest Buf updates on your PR. Results from workflow Buf Proto / buf (pull_request).

Build	Format	Lint	Breaking	Updated (UTC)
`✅ passed`	`✅ passed`	`✅ passed`	`✅ passed`	Jun 30, 2025, 2:48 PM

…tible

samlaf · 2025-06-27T15:09:15Z

@hopeyen @litt3 added documentation to the proto files. PTAL to make sure it makes sense: e2a0493

api/clients/v2/disperser_client.go

hopeyen · 2025-06-27T19:11:13Z

api/proto/disperser/v2/disperser_v2.proto

  bytes cumulative_payment = 4;
  // on-chain on-demand payment deposited
+  // The bytes are parsed to a big.Int value.
+  // This value should always be <= cumulative_payment, as the disperser cumulative_payment kept offchain is only periodically updated onchain.


we currently don't have payment updates on-chain at all, still something to add in the future. It would look something like disperser sees a request containing payment header, account id, and signature. disperser submits the message onchain, contract verifies, update cumulative_payment_used which would always be onchain_cumulative_payment_used <= onchain_cumulative_payment. then the field above cumulative_payment must be within these onchain two ranges

from GetPaymentStateForAllQuorumsRequest

…tateToNew

return early when no reservation exists, which makes the logic much simpler

…tible

hopeyen · 2025-06-30T10:15:23Z

api/clients/v2/disperser_client.go

+				MinNumSymbols: legacyReply.PaymentGlobalParams.MinNumSymbols,
+				// ReservationAdvanceWindow is not used offchain at the moment so it's okay to set to any value.
+				// It was added for consistency with the onchain data structure but get removed in the future.
+				ReservationAdvanceWindow:   legacyReply.PaymentGlobalParams.ReservationWindow,


nit: perhaps we set this to 0 similar to all the other "Not available in legacy format" fields, for consistency.

hopeyen · 2025-06-30T10:18:21Z

api/clients/v2/disperser_client.go

+		}
+
+		// Apply the global params to all quorums mentioned in on-demand quorum numbers
+		quorums := legacyReply.PaymentGlobalParams.OnDemandQuorumNumbers


quorums won't be able to cover custom quorums, but reservations would rely on quorum configurations and the client should apply global params to all quorums it might use. I would suggest constructing quorums by taking an union of legacyReply.PaymentGlobalParams.OnDemandQuorumNumbers and legacyReply.Reservation.QuorumNumbers

good catch. fixed in 689f53d

hopeyen · 2025-06-30T10:24:23Z

api/clients/v2/disperser_client.go

+				ReservationAdvanceWindow:   legacyReply.PaymentGlobalParams.ReservationWindow,
+				ReservationRateLimitWindow: legacyReply.PaymentGlobalParams.ReservationWindow,
+				OnDemandRateLimitWindow:    0,    // Not available in legacy format
+				OnDemandEnabled:            true, // we are iterating over on-demand quorums, so this is always true


if we update the content of quorums, we might need a separate loop to set field to true for ondemand quorum numbers whereas the default is false

hopeyen · 2025-06-30T10:46:32Z

api/proto/disperser/v2/disperser_v2.proto

-  // dispersers unlimited to EigenLabs.
+  // off-chain on-demand payment usage.
+  // The bytes are parsed to a big.Int value.
+  // This value should always be <= cumulative_payment, as the disperser cumulative_payment kept offchain is only periodically updated onchain.


this description isn't accurate atm. updating the onchain balance is something we have been putting off for the future. There's some proposal on how it updates, verifies, and handling refunds... but nothing has been decided

in convertLegacyPaymentStateToNew

api/clients/v2/disperser_client.go

* chore(apiserver): refactor GetPaymentState to highlight legacy conversion * wip: attempt to make disperser_client backwards compatible TODO: review the Claude generated getPaymentStateFromLegacyAPI... no idea if its OK or not. * docs(apiserver): document convertAllQuorumsReplyToLegacy function * fix(disperser_client): convertLegacyPaymentStateToNew returns error * fix(disperser_client): set correct value for ReservationRateLimitWindow * style: targetting -> targeting * docs: remove outdated TODO comment * docs: add docstring for getPaymentStateFromLegacyAPI * fix: return error if PaymentGlobalParams==nil * docs(proto): document GetPaymentStateReply fields also GetPaymentStateForAllQuorumsReply fields * docs(proto): document why GetPaymentStateRequest is separate type from GetPaymentStateForAllQuorumsRequest * style: remove outdated comment * fix(disperser_client): onDemandEnabled value in convertLegacyPaymentStateToNew * style: better error message in convertLegacyPaymentStateToNew * refactor: convertLegacyPaymentStateToNew function return early when no reservation exists, which makes the logic much simpler * docs(proto): fix inaccurate cumulative_payment docstring * style(disperser_client): set reservationAdvanceWindow to 0 in convertLegacyPaymentStateToNew * fix(disp_client): convertLegacyPaymentStateToNew quorums setting logic * docs(proto): remove wrong cumulative_payment comment * style: remove unneeded comments * style: add comment explaining why append is ok * docs: move reservation_advance_window comment to proto

This reverts commit 26c612f.

* 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.

samlaf added 2 commits June 26, 2025 18:25

chore(apiserver): refactor GetPaymentState to highlight legacy conver…

8dc481a

…sion

wip: attempt to make disperser_client backwards compatible

f6c2619

TODO: review the Claude generated getPaymentStateFromLegacyAPI... no idea if its OK or not.

samlaf requested a review from a team as a code owner June 26, 2025 10:27

samlaf requested a review from hopeyen June 26, 2025 10:27

samlaf marked this pull request as draft June 26, 2025 10:27

samlaf commented Jun 26, 2025

View reviewed changes

hopeyen mentioned this pull request Jun 26, 2025

feat: GetPaymentStateForAllQuorums api impl #1664

Merged

5 tasks

hopeyen reviewed Jun 26, 2025

View reviewed changes

samlaf marked this pull request as ready for review June 26, 2025 17:00

samlaf added 4 commits June 27, 2025 01:10

docs(apiserver): document convertAllQuorumsReplyToLegacy function

5eceac6

fix(disperser_client): convertLegacyPaymentStateToNew returns error

066fcb4

fix(disperser_client): set correct value for ReservationRateLimitWindow

249d831

Merge branch 'master' into fix--make-disperser-client-backwards-compa…

adf5597

…tible

samlaf requested a review from hopeyen June 26, 2025 17:23

litt3 reviewed Jun 26, 2025

View reviewed changes

samlaf added 5 commits June 27, 2025 21:53

style: targetting -> targeting

3347eb0

docs: remove outdated TODO comment

c499ebe

docs: add docstring for getPaymentStateFromLegacyAPI

78f2f3c

fix: return error if PaymentGlobalParams==nil

f417dbd

docs(proto): document GetPaymentStateReply fields

e2a0493

also GetPaymentStateForAllQuorumsReply fields

Merge branch 'master' into fix--make-disperser-client-backwards-compa…

8f2f9dd

…tible

samlaf requested a review from litt3 June 27, 2025 15:08

litt3 previously approved these changes Jun 27, 2025

View reviewed changes

hopeyen reviewed Jun 27, 2025

View reviewed changes

samlaf added 4 commits June 30, 2025 17:05

docs(proto): document why GetPaymentStateRequest is separate type

44e13c0

from GetPaymentStateForAllQuorumsRequest

style: remove outdated comment

76faa3b

fix(disperser_client): onDemandEnabled value in convertLegacyPaymentS…

d24eacb

…tateToNew

style: better error message in convertLegacyPaymentStateToNew

7db678c

refactor: convertLegacyPaymentStateToNew function

d225f94

return early when no reservation exists, which makes the logic much simpler

samlaf dismissed litt3’s stale review via d225f94 June 30, 2025 09:55

samlaf added 2 commits June 30, 2025 18:08

docs(proto): fix inaccurate cumulative_payment docstring

5609dae

Merge branch 'master' into fix--make-disperser-client-backwards-compa…

7188864

…tible

samlaf requested review from hopeyen and litt3 June 30, 2025 10:10

hopeyen previously approved these changes Jun 30, 2025

View reviewed changes

samlaf added 3 commits June 30, 2025 22:12

style(disperser_client): set reservationAdvanceWindow to 0

e00a2ba

in convertLegacyPaymentStateToNew

fix(disp_client): convertLegacyPaymentStateToNew quorums setting logic

689f53d

docs(proto): remove wrong cumulative_payment comment

f10d69a

samlaf dismissed hopeyen’s stale review via f10d69a June 30, 2025 14:21

samlaf requested a review from hopeyen June 30, 2025 14:22

litt3 previously approved these changes Jun 30, 2025

View reviewed changes

api/clients/v2/disperser_client.go Outdated Show resolved Hide resolved

api/clients/v2/disperser_client.go Show resolved Hide resolved

api/clients/v2/disperser_client.go Outdated Show resolved Hide resolved

samlaf added 3 commits June 30, 2025 22:44

style: remove unneeded comments

0dafcd6

style: add comment explaining why append is ok

fbbb8ef

docs: move reservation_advance_window comment to proto

c1f2fbc

samlaf dismissed litt3’s stale review via c1f2fbc June 30, 2025 14:48

litt3 approved these changes Jun 30, 2025

View reviewed changes

hopeyen approved these changes Jun 30, 2025

View reviewed changes

samlaf merged commit 26c612f into master Jun 30, 2025
22 checks passed

samlaf deleted the fix--make-disperser-client-backwards-compatible branch June 30, 2025 15:05

samlaf added a commit that referenced this pull request Jul 11, 2025

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

6e14aed

This reverts commit 26c612f.

		// TODO: this was generated by Claude. I haven't checked that it actually makes sense.... SO DO NOT MERGE BEFORE CHECKING THIS.
		func convertLegacyPaymentStateToNew(legacyReply disperser_rpc.GetPaymentStateReply) disperser_rpc.GetPaymentStateForAllQuorumsReply {

fix: make disperser client backwards compatible #1686

fix: make disperser client backwards compatible #1686

Uh oh!

Conversation

samlaf commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Checks

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hopeyen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samlaf commented Jun 27, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samlaf commented Jun 26, 2025 •

edited

Loading

github-actions bot commented Jun 27, 2025 •

edited

Loading