feat: protobuf payment state api quorum specific variant #1613

hopeyen · 2025-05-29T02:21:07Z

Why are these changes needed?

Added a new request type for payment state. This is to avoid the breaking change complain from buf formats, and later we will migrate the current call to the new payment state API with quorum specifications.

The request types between the two APIs are the same, and authentication is the same.

The current endpoint GetPaymentState should be deprecated as users migrate to use GetPaymentStateQuorumSpecific. During the time where both are served while the backend meterer moves to be quorum specific, GetPaymentState will serve the same on-demand and quorum 0 global parameter responses, but its singular reservation will be filled with the most restrictive quorum reservation record to ensure that reservations would still be functional even though might be wasteful for the client.

The reason we pick quorum 0 for the global parameter is because ondemand on-chain depositing is only enabled on quorum 0 until we support decentralized ondemand (functionality stays the same, users deposited once can still use quorum 0 or/and 1.)

// Reservation parameters of an account, used to determine the rate limit for the account.
message Reservation {
  // rate limit for the account; provide the lowest rate of all quorum reservations
  uint64 symbols_per_second = 1;
  // start timestamp of the reservation; latest start timestamp
  uint32 start_timestamp = 2;              
  // end timestamp of the reservation; earliest end timestamp
  uint32 end_timestamp = 3;
  // quorums allowed to make reserved dispersals; all the quorum numbers user account had a reservation on
  repeated uint32 quorum_numbers = 4;
  // quorum splits describes how the payment is split among the quorums; this was never used and will be empty
  repeated uint32 quorum_splits = 5;
}

// Global constant parameters defined by the payment vault.
// This message type will soon be deprecated in replacement of PaymentVaultParams. During endpoint migration, this will be filled
// with the parameters on quorum 0, quorum configurations will be the same across quorums for the foreseeable future.
message PaymentGlobalParams {
  // Global ratelimit for on-demand dispersals
  uint64 global_symbols_per_second = 1;
  // Minimum number of symbols accounted for all dispersals
  uint64 min_num_symbols = 2;
  // Price charged per symbol for on-demand dispersals
  uint64 price_per_symbol = 3;
  // Reservation window for all reservations
  uint64 reservation_window = 4;
  // quorums allowed to make on-demand dispersals
  repeated uint32 on_demand_quorum_numbers = 5;
}

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

github-actions · 2025-05-29T02:21:27Z

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 3, 2025, 4:25 PM

api/proto/disperser/v2/disperser_v2.proto

disperser/apiserver/server_v2.go

ian-shim · 2025-05-30T03:31:02Z

api/proto/disperser/v2/disperser_v2.proto

+  //
+  // For an example usage, see how our disperser_client makes a call to this endpoint to populate its local accountant struct:
+  // https://github.com/Layr-Labs/eigenda/blob/6059c6a068298d11c41e50f5bcd208d0da44906a/api/clients/v2/disperser_client.go#L298
+  rpc GetPaymentStateQuorumSpecific(GetPaymentStateQuorumSpecificRequest) returns (GetPaymentStateQuorumSpecificReply) {}


nit: What do you think about GetPaymentStateByQuorum or GetQuorumPaymentState?

I asked to rename to this here: #1613 (comment)
I don't like GetQuorumPaymentState for the reasons listed in that comment, but I'm fine with GetPaymentStateByQuorum

GetPaymentStateByQuorum is not accurate (comment). What about..... GetPaymentStates with s at the end?

What about GetPaymentStateFromQuorums?

Since we are not allowing users to specify quorums in this request, maybe we should remove that from the name altogether. what about GetPaymentStateVerbose?

How about GetPaymentStateForAllQuorums then?

updated to GetPaymentStateForAllQuorums (commit)

ian-shim · 2025-05-30T03:32:50Z

api/proto/disperser/v2/disperser_v2.proto

+  // payment vault parameters with per-quorum configurations
+  PaymentVaultParams payment_vault_params = 1;
+  // period_records maps quorum IDs to the off-chain account reservation usage records for the current and next two periods
+  map<uint32, PeriodRecords> period_records = 2;


Isn't the request filtering the payment state by quorum? What does it return results for all quorums?

The intended behavior is to return all the quorum usages they have reservations for, not filter by quorum. The QuorumSpecific really means that the results returned are quorum specific, not for a specific quorum. I can see how this leads to misunderstanding, so I'm okay to use a different name
There's arguments you can make for either behavior, but I think this is the easiest on the client side, so they don't need to know quorum numbers before they request for the state or make separate requests; and this behavior is closer to the behavior of the current GetPaymentState

disperser/apiserver/server_v2.go

core/auth/v2/authenticator.go

api/proto/disperser/v2/disperser_v2.proto

dmanc · 2025-05-30T19:51:00Z

api/proto/disperser/v2/disperser_v2.proto

@@ -27,13 +27,30 @@ service Disperser {

  // GetPaymentState is a utility method to get the payment state of a given account, at a given disperser.
  // EigenDA's payment system for v2 is currently centralized, meaning that each disperser does its own accounting.
+  // As reservation moves to be quorum specific and served by permissionless dispersers, GetPaymentState will soon be deprecate


Do we have a procedure for deprecating grpc methods?

no written procedure afaik, so the current plan is for disperser to serve both old and new endpoints. I think we should deprecate this before releasing permissionless dispersers. I can bring this up in the standup for making a standard procedure

api/proto/disperser/v2/disperser_v2.proto

dmanc · 2025-05-30T19:54:35Z

api/proto/disperser/v2/disperser_v2.proto

+  //
+  // For an example usage, see how our disperser_client makes a call to this endpoint to populate its local accountant struct:
+  // https://github.com/Layr-Labs/eigenda/blob/6059c6a068298d11c41e50f5bcd208d0da44906a/api/clients/v2/disperser_client.go#L298
+  rpc GetPaymentStateQuorumSpecific(GetPaymentStateQuorumSpecificRequest) returns (GetPaymentStateQuorumSpecificReply) {}


What about GetPaymentStateFromQuorums?

core/auth/v2/auth_test.go

anupsv · 2025-06-02T17:35:52Z

core/auth/v2/authenticator.go

+// AuthenticatePaymentStateQuorumSpecificRequest verifies the signature of the quorum specific payment state request
+// The signature is signed over the byte representation of the account ID and requestHash
+// See implementation of BlobRequestSigner.SignPaymentStateRequest for more details
+func (a *authenticator) AuthenticatePaymentStateQuorumSpecificRequest(accountAddr common.Address, request *pb.GetPaymentStateQuorumSpecificRequest) error {


This fn name will also need to change based on above comments.

yeah, I don't think there's an agreement on the name yet:(

anupsv

looks good over all. Need the naming conflict to resolve.

core/auth/v2/authenticator.go

samlaf

LGTM. Left some nits

samlaf · 2025-06-03T13:31:13Z

api/proto/disperser/v2/disperser_v2.proto

@@ -27,13 +27,30 @@ service Disperser {

  // GetPaymentState is a utility method to get the payment state of a given account, at a given disperser.
  // EigenDA's payment system for v2 is currently centralized, meaning that each disperser does its own accounting.
+  // As reservation moves to be quorum specific and served by permissionless dispersers, GetPaymentState will soon be deprecated


why soon? Why not now? When are we planning to deprecate?

Planning to deprecate this after the traffic of this method goes to zero. Alternatively can deprecate this in a later PR after the client has been updated
Relevant comment

It is a separate PR because I was told to split PR into smaller pieces for easier review process. This PR is meant to contain the new Protobuf API

Makes sense. I think it should be deprecated as soon as we have the new client available. Deprecating is a signal for people to switch over. If they don't then likely the traffic would never go to zero.
We shouldn't deprecate after traffic goes to zero. We should deprecate for traffic to go to zero, and then just remove the rpc method entirely.

api/proto/disperser/v2/disperser_v2.proto

api/clients/v2/accountant_test.go

api/proto/disperser/v2/disperser_v2.proto

api/clients/v2/accountant_test.go

…)" This reverts commit 58acdfd.

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

feat: protobuf payment state api quorum specific variant

bfdfe8d

hopeyen force-pushed the hope/quorum-pmt-state-api branch from 2943177 to ed2e9be Compare May 29, 2025 02:30

chore: remove unrelevant changes

f9d3e7c

hopeyen force-pushed the hope/quorum-pmt-state-api branch from ed2e9be to f9d3e7c Compare May 29, 2025 02:31

hopeyen marked this pull request as ready for review May 29, 2025 03:08

hopeyen requested a review from a team as a code owner May 29, 2025 03:08

hopeyen requested review from anupsv, dmanc and ian-shim May 29, 2025 03:09

samlaf reviewed May 29, 2025

View reviewed changes

api/proto/disperser/v2/disperser_v2.proto Outdated Show resolved Hide resolved

api/proto/disperser/v2/disperser_v2.proto Outdated Show resolved Hide resolved

disperser/apiserver/server_v2.go Outdated Show resolved Hide resolved

hopeyen added 6 commits May 29, 2025 08:03

refactor: rename api

e88f384

docs: payment state quorum specifications

9b09338

docs: migration description

0b8c183

Merge branch 'master' into hope/quorum-pmt-state-api

c99a86e

fix: quorum period records still needs an array

7678e94

refactor: rename leftover functions

92f2f89

hopeyen mentioned this pull request May 29, 2025

feat: quorum pmt state api impl #1619

Closed

9 tasks

ian-shim reviewed May 30, 2025

View reviewed changes

hopeyen added 3 commits May 30, 2025 12:11

feat: cleanup protobuf field

3d2fffa

chore: proto gen

8c5f315

Merge branch 'master' into hope/quorum-pmt-state-api

4c70005

dmanc reviewed May 30, 2025

View reviewed changes

hopeyen added 2 commits May 30, 2025 14:30

refactor: rm empty lines and fix grammar

7193742

chore: proto gen

a1c6649

hopeyen requested review from samlaf and dmanc May 30, 2025 21:59

anupsv reviewed Jun 2, 2025

View reviewed changes

anupsv previously approved these changes Jun 2, 2025

View reviewed changes

ian-shim previously approved these changes Jun 2, 2025

View reviewed changes

chore: rename api

0cefc62

hopeyen dismissed stale reviews from ian-shim and anupsv via 0cefc62 June 2, 2025 20:00

Merge branch 'master' into hope/quorum-pmt-state-api

b01c1de

dmanc previously approved these changes Jun 2, 2025

View reviewed changes

core/auth/v2/authenticator.go Show resolved Hide resolved

refactor: deprecation feedback

6fbbfe8

hopeyen dismissed dmanc’s stale review via 6fbbfe8 June 3, 2025 00:08

refactor: deprecation feedback

936c279

dmanc previously approved these changes Jun 3, 2025

View reviewed changes

samlaf previously approved these changes Jun 3, 2025

View reviewed changes

refactor: proto doc feedback

6278524

hopeyen dismissed stale reviews from samlaf and dmanc via 6278524 June 3, 2025 15:06

Merge branch 'master' into hope/quorum-pmt-state-api

d133da1

hopeyen requested a review from samlaf June 3, 2025 15:46

samlaf previously approved these changes Jun 3, 2025

View reviewed changes

api/proto/disperser/v2/disperser_v2.proto Outdated Show resolved Hide resolved

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

refactor: proto doc feedback

19d6f26

hopeyen dismissed samlaf’s stale review via 19d6f26 June 3, 2025 16:25

samlaf approved these changes Jun 3, 2025

View reviewed changes

hopeyen merged commit 58acdfd into master Jun 3, 2025
17 checks passed

hopeyen deleted the hope/quorum-pmt-state-api branch June 3, 2025 16:43

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

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

e988560

…)" This reverts commit 58acdfd.

feat: protobuf payment state api quorum specific variant #1613

feat: protobuf payment state api quorum specific variant #1613

Uh oh!

Conversation

hopeyen commented May 29, 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!

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

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!

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!

Uh oh!

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!

anupsv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

samlaf left a comment

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!

Uh oh!

Uh oh!

Uh oh!

hopeyen commented May 29, 2025 •

edited

Loading

github-actions bot commented May 29, 2025 •

edited

Loading