Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v2] disperser server payments api #902

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

[v2] disperser server payments api #902

wants to merge 16 commits into from

Conversation

hopeyen
Copy link
Contributor

@hopeyen hopeyen commented Nov 15, 2024

Why are these changes needed?

Adding payments to disperser v2 api and server; disperser v2 client is not available yet

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

core/meterer/offchain_store.go Fixed Show resolved Hide resolved
@hopeyen hopeyen force-pushed the hope/v2-payments branch 2 times, most recently from c0652fa to 4cf7d2c Compare November 15, 2024 10:07
@hopeyen hopeyen marked this pull request as draft November 15, 2024 10:07
@hopeyen hopeyen force-pushed the hope/v2-payments branch 7 times, most recently from 89af572 to e306bc7 Compare November 22, 2024 17:14
@hopeyen hopeyen marked this pull request as ready for review November 22, 2024 18:47
@hopeyen hopeyen changed the title [v2] payments api [v2] disperser server payments api Nov 22, 2024
api/proto/disperser/v2/disperser_v2.proto Outdated Show resolved Hide resolved
api/proto/disperser/v2/disperser_v2.proto Show resolved Hide resolved
api/proto/disperser/v2/disperser_v2.proto Outdated Show resolved Hide resolved
@@ -55,3 +56,36 @@ func (*authenticator) AuthenticateBlobRequest(header *core.BlobHeader) error {

return nil
}

func (*authenticator) AuthenticatePaymentStateRequest(request *pb.GetPaymentStateRequest) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably belongs in apiserver/server_v2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

isn't this function doing similar authentications?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you mean. Then let's take the signature bytes as the input vs. the request protobuf in this method, and use this as a library from the api server?

core/auth/v2/signer.go Outdated Show resolved Hide resolved
api/clients/accountant.go Show resolved Hide resolved
disperser/apiserver/disperse_blob_v2.go Outdated Show resolved Hide resolved
disperser/apiserver/disperse_blob_v2.go Outdated Show resolved Hide resolved
}

type BinRecord struct {
Index uint32
Usage uint64
}

func NewAccountant(reservation *core.ActiveReservation, onDemand *core.OnDemandPayment, reservationWindow uint32, pricePerSymbol uint32, minNumSymbols uint32, paymentSigner core.PaymentSigner, numBins uint32) *accountant {
func NewAccountant(accountID string, reservation *core.ActiveReservation, onDemand *core.OnDemandPayment, reservationWindow uint32, pricePerSymbol uint32, minNumSymbols uint32, numBins uint32) *accountant {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are reservationWindow, pricePerSymbol, minNumSymbols, numBins different for each account?
Aren't these globally available somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reservationWindow, pricePerSymbol, minNumSymbols should be set through the contract. I linked a PR in the previous comment about this. numBins can be set by the accountants preferences, doesn't really matter but restricted by a minimum of 3

Copy link
Contributor

Choose a reason for hiding this comment

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

in which case does it make sense to account for more than 3 bins?
I'm wondering if we should fix/hardcode this number everywhere vs. making it a variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a nice functionality considered for disperser clients who want to track more history in-memory. The data isn't really used and not critical

disperser/apiserver/server_v2.go Show resolved Hide resolved
Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

looks good, last few comments

@@ -55,3 +56,36 @@ func (*authenticator) AuthenticateBlobRequest(header *core.BlobHeader) error {

return nil
}

func (*authenticator) AuthenticatePaymentStateRequest(request *pb.GetPaymentStateRequest) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see what you mean. Then let's take the signature bytes as the input vs. the request protobuf in this method, and use this as a library from the api server?

@@ -237,3 +237,79 @@ func (s *OffchainStore) GetRelevantOnDemandRecords(ctx context.Context, accountI

return prevPayment, nextPayment, nextDataLength, nil
}

func (s *OffchainStore) GetBinUsages(ctx context.Context, accountID string, binIndex uint32) (uint64, uint64, uint64, error) {
// Fetch the 3 bins start from the current bin
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it depend on MinNumBins? Looks like it will always return 3 bin records no matter what MinNumBins is.

}

type BinRecord struct {
Index uint32
Usage uint64
}

func NewAccountant(reservation *core.ActiveReservation, onDemand *core.OnDemandPayment, reservationWindow uint32, pricePerSymbol uint32, minNumSymbols uint32, paymentSigner core.PaymentSigner, numBins uint32) *accountant {
func NewAccountant(accountID string, reservation *core.ActiveReservation, onDemand *core.OnDemandPayment, reservationWindow uint32, pricePerSymbol uint32, minNumSymbols uint32, numBins uint32) *accountant {
Copy link
Contributor

Choose a reason for hiding this comment

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

in which case does it make sense to account for more than 3 bins?
I'm wondering if we should fix/hardcode this number everywhere vs. making it a variable

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.

2 participants