[v2] disperser client payments api #928

hopeyen · 2024-11-22T19:03:06Z

Why are these changes needed?

properly construct payment header in the dispersal request

For easy initialization of DisperserClient, pass in an empty accountant &accountant{} and then call client.PopulateAccountant(ctx) to populate the accountant by getting payment information both on-chain and off-chain for the local signer.

For more customized initialization, the user can explicitly construct their accountant and pass in as the argument

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

ian-shim · 2024-12-03T01:50:46Z

api/clients/disperser_client_v2.go

@@ -60,7 +60,7 @@ var _ DisperserClientV2 = &disperserClientV2{}
 //
 //	// Subsequent calls will use the existing connection
 //	status2, blobKey2, err := client.DisperseBlob(ctx, data, blobHeader)
-func NewDisperserClientV2(config *DisperserClientV2Config, signer corev2.BlobRequestSigner, prover encoding.Prover) (*disperserClientV2, error) {
+func NewDisperserClientV2(config *DisperserClientV2Config, signer corev2.BlobRequestSigner, prover encoding.Prover, accountant Accountant) (*disperserClientV2, error) {


It's quite difficult to construct a new Accountant. Does it make sense to provide a helper method that creates an accountant from the GetPaymentState result?

Let's also add a way to call GetPaymentState in this client

added a helper function PopulateAccountant that should run after creating NewDisperserClientV2 by constructing accountant from GetPaymentState

What do you think about allowing the user of DisperserClientV2 to provide their own accountant, or simply passing in &accountant{} to NewDisperserClientV2 and then explicitly calling PopulateAccountant to fill in the accountant state? (5004cf9)

Instead of directly mutating the client, why don't we make PopulateAccountant a helper method that returns Accountant by calling GetPaymentState so that the caller can pass that in to NewDisperserClientV2?

The reason is GetPaymentState is a function of disperserClientV2. If we write a standalone PopulateAccountant, then we would need it to be able to initOnceGrpcConnection, pass in a signer for creating the request, and a disperser_rpc.DisperserClient that makes the actual request. I figured it is cleaner to just use disperserClientV2

Are there any use cases where a use of disperser_client_v2 would want to pass in its own accountant? If not, then how about making populateAccountant() private and calling it from within the constructor? Feels like disperser_client and accountant are too intertwined. Are there any use cases for using an accountant without a disperser? If not, let's just hide the populateAccountant and make the disperser_client more RAII-like.

ian-shim · 2024-12-04T02:30:25Z

api/clients/disperser_client_v2.go

@@ -109,15 +121,11 @@ func (c *disperserClientV2) DisperseBlob(
 		return nil, [32]byte{}, api.NewErrorInternal("uninitialized signer for authenticated dispersal")
 	}



check if c.accountant is nil?
we can instruct how to initialize accountant in the error message

ian-shim · 2024-12-04T02:31:48Z

api/clients/disperser_client_v2.go

 		// conn and client are initialized lazily
 	}, nil
 }

+// PopulateAccountant populates the accountant with the payment state from the disperser.
+// This function is required to be called before using the accountant. Perhaps rename to Start()?


This function isn't required if a valid accountant is passed in the constructor, right?
If you want to require this method, we can remove accountant as an argument from the constructor.

Oh yeah, will update the comment

ian-shim · 2024-12-04T02:32:30Z

api/clients/accountant.go

@@ -160,6 +159,37 @@ func (a *accountant) GetRelativeBinRecord(index uint32) *BinRecord {
 	return &a.binRecords[relativeIndex]
 }

+func (a *accountant) SetPaymentState(paymentState *disperser_rpc.GetPaymentStateReply) {
+	a.minNumSymbols = uint32(paymentState.PaymentGlobalParams.MinNumSymbols)


these will panic if any of the attributes are nil?

yes.... added a check for all of them!
It would've been great if protobuf let me set fields to be required, but proto3 removed "required" keyword:(

If you use getters (i.e. GetXXX), you could chain them!

ian-shim

lgtm, lets address the lint issue

api/clients/accountant.go

samlaf · 2024-12-05T21:59:17Z

api/clients/disperser_client_v2.go

@@ -60,7 +60,7 @@ var _ DisperserClientV2 = &disperserClientV2{}
 //
 //	// Subsequent calls will use the existing connection
 //	status2, blobKey2, err := client.DisperseBlob(ctx, data, blobHeader)
-func NewDisperserClientV2(config *DisperserClientV2Config, signer corev2.BlobRequestSigner, prover encoding.Prover) (*disperserClientV2, error) {
+func NewDisperserClientV2(config *DisperserClientV2Config, signer corev2.BlobRequestSigner, prover encoding.Prover, accountant Accountant) (*disperserClientV2, error) {


Are there any use cases where a use of disperser_client_v2 would want to pass in its own accountant? If not, then how about making populateAccountant() private and calling it from within the constructor? Feels like disperser_client and accountant are too intertwined. Are there any use cases for using an accountant without a disperser? If not, let's just hide the populateAccountant and make the disperser_client more RAII-like.

samlaf · 2024-12-05T22:01:55Z

api/clients/accountant.go

@@ -160,6 +159,59 @@ func (a *accountant) GetRelativeBinRecord(index uint32) *BinRecord {
 	return &a.binRecords[relativeIndex]
 }

+func (a *accountant) SetPaymentState(paymentState *disperser_rpc.GetPaymentStateReply) error {


This feels too coupled to me. A Public setter that is not part of the interface and it set by some response from another client. As discussed in other threads, could we not combine accountant and disperser_client more smoothly? Does accountant really need to be an independent struct?

We wanted to keep accounting flow separate from the client, but arguably we can combine them into a bigger struct. What do you think @ian-shim ?

I think this would improve the UX quite a bit. You cannot disperse a blob without payment metadata and you probably don't need accountant functionality other than for the purpose of dispersing a blob, so coupling them more explicitly makes sense imo

Makes sense! I would also not try to future-proof this design too much (to a case where there are multiple dispersers and you need some super complex accountant that manages multiple dispersers). We should try to make the simple case simple, true to golang's philosophy, and people who need complex integrations can take on the complexity hit around our simple API.

Another possibility is to have 2 constructors: one where we inject the accountant, and the other that initializes the accountant by calling the disperser?

Hmm all one needs to do is to call the current constructor and then call PopulateAccountant, so adding a separate constructor doesn't seem that beneficial to me. I won't mind adding it, probably in the follow-up PR that focus on integrations adaptions (maybe something like NewDisperserClientV2AutoPay?

api/clients/accountant.go

samlaf · 2024-12-05T22:08:19Z

api/clients/disperser_client_v2.go

@@ -108,16 +123,15 @@ func (c *disperserClientV2) DisperseBlob(
 	if c.signer == nil {
 		return nil, [32]byte{}, api.NewErrorInternal("uninitialized signer for authenticated dispersal")
 	}
+	if c.accountant == nil {
+		return nil, [32]byte{}, api.NewErrorInternal("uninitialized accountant for paid dispersal; make sure to call PopulateAccountant after creating the client")


how about we make the call to PopulateAccountant as part of the initOnce function? This way its hidden from the user.
Is there ever a need to recall PopulateAccountant? For eg if disperser went down, or whatever other weird case? To reset the accountant basically? If so, should it be called ResetAccountantFromServer or something like that?

In this case, maybe we also move signer check to the initOnce?

There's some cases a user might recall populate accountant, I don't think it hurts to be able to reset. Can update the function name for sure

I think I'll leave this as a future refactoring

samlaf

I'll give a stamp on this in case its blocking progress on other work, but I would have liked to get @ian-shim's take on #928 (comment). Would really like to find a better interface/decoupling between these. But we can do so later (hopefully...)

hopeyen · 2024-12-06T18:18:16Z

Imma merge:) linking PR for any follow-ups

hopeyen changed the base branch from master to hope/v2-payments November 22, 2024 19:03

hopeyen force-pushed the hope/v2-payments branch from 1ae3e3c to 8ea4f03 Compare November 26, 2024 01:09

hopeyen requested review from ian-shim and samlaf December 2, 2024 18:46

Base automatically changed from hope/v2-payments to master December 2, 2024 22:43

hopeyen requested a review from cody-littley December 2, 2024 22:44

feat: client construct payment header

9982bf1

hopeyen force-pushed the hope/v2-payments-client branch from 0b944cf to 9982bf1 Compare December 2, 2024 22:46

ian-shim approved these changes Dec 3, 2024

View reviewed changes

feat: getPaymentState and init accountant

5004cf9

hopeyen force-pushed the hope/v2-payments-client branch from 0f1ddcc to 5004cf9 Compare December 3, 2024 16:51

ian-shim reviewed Dec 4, 2024

View reviewed changes

fix: nil checks and remove bad comment

dd00e32

ian-shim approved these changes Dec 5, 2024

View reviewed changes

hopeyen added 2 commits December 4, 2024 16:39

fix: lint

0d59ee9

refactor: use protobuf getters and chain

cd908e1

hopeyen mentioned this pull request Dec 5, 2024

payment struct and names ReservationPeriod and Salt #965

Merged

5 tasks

samlaf reviewed Dec 5, 2024

View reviewed changes

hopeyen requested a review from samlaf December 6, 2024 17:52

refactor: rm Accountant interface and rename struct to Accountant

6643ca9

samlaf approved these changes Dec 6, 2024

View reviewed changes

hopeyen merged commit 100eb3e into master Dec 6, 2024
9 checks passed

hopeyen deleted the hope/v2-payments-client branch December 6, 2024 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2] disperser client payments api #928

[v2] disperser client payments api #928

hopeyen commented Nov 22, 2024 •

edited

Loading

ian-shim Dec 3, 2024

hopeyen Dec 3, 2024

ian-shim Dec 3, 2024

hopeyen Dec 3, 2024

samlaf Dec 5, 2024

ian-shim Dec 4, 2024

ian-shim Dec 4, 2024

hopeyen Dec 4, 2024

ian-shim Dec 4, 2024

hopeyen Dec 4, 2024

ian-shim Dec 5, 2024

ian-shim left a comment

samlaf Dec 5, 2024

samlaf Dec 5, 2024

hopeyen Dec 5, 2024

ian-shim Dec 6, 2024

samlaf Dec 6, 2024

hopeyen Dec 6, 2024

samlaf Dec 5, 2024

hopeyen Dec 5, 2024

hopeyen Dec 6, 2024

samlaf left a comment

hopeyen commented Dec 6, 2024

		@@ -109,15 +121,11 @@ func (c *disperserClientV2) DisperseBlob(
		return nil, [32]byte{}, api.NewErrorInternal("uninitialized signer for authenticated dispersal")
		}

[v2] disperser client payments api #928

[v2] disperser client payments api #928

Conversation

hopeyen commented Nov 22, 2024 • edited Loading

Why are these changes needed?

Checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ian-shim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samlaf left a comment

Choose a reason for hiding this comment

hopeyen commented Dec 6, 2024

hopeyen commented Nov 22, 2024 •

edited

Loading