[payments] meterer structs and helpers #789

hopeyen · 2024-10-07T23:57:51Z

Why are these changes needed?

adding structures used by payment meterer, with utility functions and unit tests

Checks

I've made sure the lint is passing in this PR.
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/meterer.go

mooselumph · 2024-10-08T16:31:21Z

core/meterer/offchain_store.go

+	}
+
+	update := map[string]types.AttributeValue{
+		"BinUsage": &types.AttributeValueMemberN{Value: strconv.FormatUint(uint64(size), 10)},


nit: size is already a uint64

thanks! updated UpdateItemIncrement function to IncrementBy that only takes in a key name and a value uint64

mooselumph · 2024-10-08T16:32:10Z

core/meterer/offchain_store.go

+		"BinUsage": &types.AttributeValueMemberN{Value: strconv.FormatUint(uint64(size), 10)},
+	}
+
+	fmt.Println("increment the item in a table", s.reservationTableName)


Remove or convert to log?

mooselumph · 2024-10-08T16:33:08Z

core/meterer/offchain_store.go

+}
+
+// Find all reservation bins for a given account
+func (s *OffchainStore) FindReservationBins(ctx context.Context, accountID string) ([]ReservationBin, error) {


Is this used?

nope, this was for later exposing API to the user, but removed for now

mooselumph · 2024-10-08T16:45:28Z

core/meterer/types.go

+type BlobHeader struct {
+	// Existing fields
+	Commitment    core.G1Point
+	DataLength    uint32


Note: This should be the length in number of symbols.

I added a comment for this, though should I update the name to be something like NumSymbols?

ian-shim · 2024-10-08T20:38:34Z

common/aws/dynamodb/client.go

@@ -38,7 +38,7 @@ var (

 type Item = map[string]types.AttributeValue
 type Key = map[string]types.AttributeValue
-type ExpresseionValues = map[string]types.AttributeValue
+type ExpresseionValues = map[string]types.AttributeValue // is this a typo? ExpressionValues?


lol yeah.. want to update this in a separate PR?

ian-shim · 2024-10-08T20:50:22Z

core/meterer/meterer.go

+	TxnBroadcastTimeout time.Duration
+}
+
+// network parameters that should be published on-chain. We currently configure these params through disperser env vars.


nit: for any comments on a method/struct/field, start by the name of the method/struct/field. It will be recognized by godoc

updated, thanks for the tip!

core/meterer/meterer.go

ian-shim · 2024-10-08T20:51:34Z

core/meterer/meterer.go

+//
+// If the payment is valid, the meterer will add the blob header to its state and return a success response to the disperser API server.
+// if any of the checks fail, the meterer will return a failure response to the disperser API server.
+var OnDemandQuorumNumbers = []uint8{0, 1}


These probably shouldn't be hardcoded

Do you think they should be published on-chain?

We already have them onchain depending on what you need.
QuorumCount here and required quorums here

I realized that this is a read function from transactor, so will do corresponding updates :)

ian-shim · 2024-10-08T20:55:07Z

core/meterer/types.go

+}
+
+// EIP712Signer handles EIP-712 signing operations
+type EIP712Signer struct {


This signature scheme will likely be shared for validating a request. Should this live outside meterer package? Maybe somewhere like core/auth?

This makes sense, moved PaymentMetadata to core/data.go, and all the signing to core/auth! If we enforce meterer to handle everything PaymentMetadata, I can move it back to meterer package 🤔

ian-shim · 2024-10-08T21:14:28Z

core/meterer/offchain_store.go

+		return nil, fmt.Errorf("table names cannot be empty")
+	}
+
+	err = CreateReservationTable(cfg, reservationTableName)


Tables should be created outside the application code (devops). We can check if the tables with corresponding names exist in the constructor and error out if they don't exist

makes sense, updated to use check and error instead of making db changes

ian-shim · 2024-10-08T21:19:23Z

core/meterer/util.go

+		},
+		GlobalSecondaryIndexes: []types.GlobalSecondaryIndex{
+			{
+				IndexName: aws.String("AccountIDIndex"),


Do we need this index? Looks like we already key by AccountID

During testing, I found that this IndexName was needed to enable QueryIndex. I referenced the pattern like [here] with BlobStatus key and StatusIndex index (

eigenda/disperser/common/blobstore/blob_metadata_store.go

Line 462 in 35e0b6b

IndexName: aws.String(statusIndexName),

).

Yeah if you need to query by something other than the table's default keys, then building indexes on those attributes may help. However, looks like we can query by the table's default keys, in which case we can query from the table directly

ian-shim · 2024-10-08T21:20:19Z

core/meterer/util.go

+		},
+		GlobalSecondaryIndexes: []types.GlobalSecondaryIndex{
+			{
+				IndexName: aws.String("AccountIDIndex"),


ian-shim · 2024-10-08T21:26:02Z

core/meterer/util.go

+	"github.com/aws/aws-sdk-go-v2/service/dynamodb/types"
+)
+
+func DummyCommitment() core.G1Point {


This doesn't seem used anywhere

this is used in later PRs but I've removed it since we are removing commitment from the Payment header

ian-shim · 2024-10-08T21:26:25Z

core/meterer/util.go

+	return *commitment
+}
+
+func CreateReservationTable(clientConfig commonaws.ClientConfig, tableName string) error {


I feel like we can collapse these tables into a single table design. Wdyt?

🤔 something like this?
My main consideration is the read/write performance based on the evenness of key distributions, and think it is clearer if different payment methods live in different tables, but I'm interested in learning the advantages of combining these into a single table

Added a suggestion in the design doc. We can discuss it there

ian-shim · 2024-10-10T01:14:20Z

common/aws/dynamodb/client.go

@@ -2,8 +2,10 @@ package dynamodb



rebasing to master should hide these changes that have already been merged

ahh just realized that this branch was configured to merge into the dynamo-update branch. updated the merge base so the changes should be hidden now

ian-shim · 2024-10-10T23:21:19Z

core/auth/payment_metadata.go

+				{Name: "chainId", Type: "uint256"},
+				{Name: "verifyingContract", Type: "address"},
+			},
+			"PaymentMetadata": []apitypes.Type{


Does this depend on the structure of BlobHeader in v2? (i.e. would signature over BlobHeader be sufficient without one over PaymentMetadata?)
Do we need this struct signed in both v1 & v2?

I think payment metadata will depend on BlobHeader v2 in which PaymentMetadata includes BlobHeader's hash, a field like BlobKey.
This struct would always be signed, but I guess payments is currently disjointed from blob information. We could discuss whether it is okay to let payment be "blob-agnostic" in v1, and only check blob's dataLength against the paid amount?

In order to support both v1 and v2, what if we were to add a BlobKey field here which is not populated in v1?

Do we need signature over PaymentMetadata at all?
Disperser should validate the request independently i.e. by validating signature over the challenge parameter in v1, and by validating signature over blob key in v2.
Could downstream metering library assume the passed in payment metadata is correct?

ian-shim · 2024-10-10T23:22:42Z

core/data.go

@@ -470,3 +470,36 @@ func (cb Bundles) FromEncodedBundles(eb EncodedBundles) (Bundles, error) {
 	}
 	return c, nil
 }
+
+// PaymentMetadata represents the header information for a blob
+type PaymentMetadata struct {


Should this be independent of what BlobHeader looks like?

hmm we could add a field like blob key or blob pointer? this would mean we need to do blob lookup. With these current fields, we don't need the blob or blob header to do metering

ian-shim · 2024-10-10T23:24:30Z

core/meterer/meterer.go

+type Config struct {
+	// for rate limiting 2^64 ~= 18 exabytes per second; 2^32 ~= 4GB/s
+	// for payments      2^64 ~= 18M Eth;                2^32 ~= 4ETH
+	GlobalBytesPerSecond uint64 // Global rate limit in bytes per second for on-demand payments


nit: it's nicer to put comment for field description above the definition, starting with the name of the field

ian-shim · 2024-10-10T23:26:06Z

core/meterer/meterer.go

+	ChainReadTimeout time.Duration // Timeout for reading payment state from chain
+}
+
+// disperser API server will receive requests from clients. these requests will be with a blobHeader with payments information (CumulativePayments, BinIndex, and Signature)


nit: Meterer handles payment accounting across different accounts. Disperser API server receives requests from clients and each request contains a blob header....

ian-shim · 2024-10-10T23:27:21Z

core/meterer/meterer.go

+	ChainState    OnchainPayment
+	OffchainStore OffchainStore


nit: brief description of what gets stored on which store and how they're used differently?

ian-shim · 2024-10-10T23:32:41Z

core/meterer/util.go

+		},
+		GlobalSecondaryIndexes: []types.GlobalSecondaryIndex{
+			{
+				IndexName: aws.String("AccountIDIndex"),


Yeah if you need to query by something other than the table's default keys, then building indexes on those attributes may help. However, looks like we can query by the table's default keys, in which case we can query from the table directly

mooselumph · 2024-10-15T16:04:36Z

core/auth/payment_metadata_test.go

+	t.Run("SignPaymentMetadata", func(t *testing.T) {
+		signature, err := signer.SignPaymentMetadata(header, privateKey)
+		require.NoError(t, err)
+		assert.NotEmpty(t, signature)


One thing you can do is use a contract script like this to generate the same signature on chain and compare.

eigenda/contracts/script/GenerateUnitTestHashes.s.sol

Line 13 in fab84e3

contract GenerateHashes is Script {

mooselumph · 2024-10-15T16:06:34Z

core/auth/payment_metadata.go

+}
+
+// NewEIP712Signer creates a new EIP712Signer instance
+func NewEIP712Signer(chainID *big.Int, verifyingContract common.Address) *EIP712Signer {


Does the verifyingContract ever have a function?

Nope, verifyingContract would only affect the output signature and the recovery

mooselumph · 2024-10-15T16:08:20Z

core/auth/payment_metadata.go

+				{Name: "chainId", Type: "uint256"},
+				{Name: "verifyingContract", Type: "address"},
+			},
+			"PaymentMetadata": []apitypes.Type{


In order to support both v1 and v2, what if we were to add a BlobKey field here which is not populated in v1?

mooselumph · 2024-10-15T16:09:18Z

core/auth/payment_metadata.go

+				{Name: "accountID", Type: "string"},
+				{Name: "binIndex", Type: "uint32"},
+				{Name: "cumulativePayment", Type: "uint64"},
+				{Name: "dataLength", Type: "uint32"},


I'm not sure the dataLength would be needed in v2 if we also have the blob key.

Could I assume meterer will receive validated blob header which contains payment metadata and the quorum numbers and length? If so I could remove validateSignature from the meterer process and make a new blobHeader struct?

mooselumph · 2024-10-15T16:15:04Z

core/auth/payment_metadata.go

+	return signature, nil
+}
+
+func convertUint8SliceToMap(params []uint8) []string {


convertUint8SliceToStringSlice?

ian-shim · 2024-10-16T05:03:02Z

common/aws/dynamodb/client.go

@@ -408,3 +409,17 @@ func (c *Client) readItems(ctx context.Context, tableName string, keys []Key) ([

 	return items, nil
 }
+
+// TableCheck checks if a table exists and can be described
+func (c *Client) TableCheck(ctx context.Context, name string) error {


nit: TableExists?

ian-shim · 2024-10-16T05:03:46Z

core/data.go

+	return crypto.Keccak256(data)
+}
+
+type TokenAmount uint64 // TODO: change to uint128


should it be big.Int?

~~updated~~ I ended up deleting this type alias and just using big.Int directly

ian-shim · 2024-10-16T05:05:18Z

core/meterer/meterer.go

+	Config
+
+	// ChainState reads on-chain payment state periodically and cache it in memory
+	ChainState OnchainPayment


OnchainPayment? ChainState sounds like the ChainState struct in chainio 😭

Can we perhaps move it later?🫠 similar to replacing transactor with reader, I would prefer creating a separate PR after both of these are merged or at least chainio gets merged??

ian-shim · 2024-10-16T05:13:56Z

core/meterer/onchain_state.go

+// OnchainPaymentState is an interface for getting information about the current chain state for payments.
+type OnchainPayment interface {
+	GetCurrentBlockNumber(ctx context.Context) (uint32, error)
+	CurrentOnchainPaymentState(ctx context.Context, tx *eth.Transactor) (OnchainPaymentState, error)


should we use reader/writer from chainio to replace transactor?

ian-shim · 2024-10-16T05:31:19Z

core/meterer/util.go

+	return *commitment
+}
+
+func CreateReservationTable(clientConfig commonaws.ClientConfig, tableName string) error {


Added a suggestion in the design doc. We can discuss it there

mooselumph · 2024-10-16T20:45:40Z

core/data.go

+}
+
+type OnDemandPayment struct {
+	CumulativePayment big.Int // Total amount deposited by the user


I think it's common practice to use a pointer for big.Int, i.e. CumulativePayment *big.Int, as most methods consume the pointer.

sg, updated

hopeyen changed the title ~~payment meterer helpers~~ [payments] meterer structs and helpers Oct 8, 2024

mooselumph reviewed Oct 8, 2024

View reviewed changes

hopeyen force-pushed the hope/meterer-helpers branch from ae6e034 to c55e7c0 Compare October 8, 2024 17:57

hopeyen changed the base branch from master to hope/dynamodb-updates October 8, 2024 17:57

hopeyen force-pushed the hope/meterer-helpers branch 4 times, most recently from f553c37 to 6ae70ef Compare October 8, 2024 18:02

ian-shim reviewed Oct 8, 2024

View reviewed changes

hopeyen force-pushed the hope/dynamodb-updates branch from feb5543 to a8a6ca9 Compare October 8, 2024 23:55

hopeyen force-pushed the hope/meterer-helpers branch 2 times, most recently from 49e938b to 5b50c0e Compare October 9, 2024 17:00

hopeyen force-pushed the hope/dynamodb-updates branch 2 times, most recently from af1d9fe to a6810c2 Compare October 9, 2024 19:40

hopeyen force-pushed the hope/meterer-helpers branch 2 times, most recently from cc85c19 to e7a8e3a Compare October 9, 2024 21:58

ian-shim reviewed Oct 10, 2024

View reviewed changes

hopeyen force-pushed the hope/meterer-helpers branch from e7a8e3a to 7c0c037 Compare October 10, 2024 23:38

hopeyen added 10 commits October 10, 2024 16:43

feat: meterer helper types and structs

547257b

refactor: minor updates and cleanup

f066d70

refactor: PaymentMetadata instead of BlobHeader, remove commitment field

1e3a10b

refactor: reduce TimeoutConfig to read timeout in Config

1b2c991

refactor: PaymentMetadata to core/data, signing to core/auth

e6e4ea2

chore: use Mock for on-chain payments in tests

7ce96d5

refactor: CreateTable's -> TableCheck for existance

c487395

refactor: remove hardcoded quorumNumbers for on-demand

be254b8

fix: ExpresseionValues -> ExpressionValues typo

99a7c98

fix: db creation in tests, mock filename

7854687

hopeyen force-pushed the hope/meterer-helpers branch from 7c0c037 to 7854687 Compare October 10, 2024 23:44

hopeyen changed the base branch from hope/dynamodb-updates to master October 10, 2024 23:46

refactor: comments, db query fn and tests

4f1e931

hopeyen force-pushed the hope/meterer-helpers branch 3 times, most recently from c8c1f3e to ac0c882 Compare October 11, 2024 00:48

fix: lints and db/signer tests

fab84e3

hopeyen force-pushed the hope/meterer-helpers branch from ac0c882 to fab84e3 Compare October 11, 2024 00:50

mooselumph reviewed Oct 15, 2024

View reviewed changes

hopeyen force-pushed the hope/meterer-helpers branch from c255424 to aae5395 Compare October 15, 2024 22:21

refactor: payment metadata update

4a50bf6

hopeyen force-pushed the hope/meterer-helpers branch from aae5395 to 4a50bf6 Compare October 15, 2024 22:21

ian-shim reviewed Oct 16, 2024

View reviewed changes

hopeyen force-pushed the hope/meterer-helpers branch 2 times, most recently from 3545713 to 0d0dabf Compare October 16, 2024 17:24

refactor: update TableExists name and TokenAmount type

9d5663a

hopeyen force-pushed the hope/meterer-helpers branch from 0d0dabf to 9d5663a Compare October 16, 2024 17:31

mooselumph reviewed Oct 16, 2024

View reviewed changes

refactor: use pointer for big int

ef739df

mooselumph approved these changes Oct 16, 2024

View reviewed changes

ian-shim approved these changes Oct 16, 2024

View reviewed changes

hopeyen merged commit aa7b798 into master Oct 16, 2024
10 checks passed

hopeyen deleted the hope/meterer-helpers branch October 16, 2024 22:31

[payments] meterer structs and helpers #789

[payments] meterer structs and helpers #789

Conversation

hopeyen commented Oct 7, 2024

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

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

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

hopeyen Oct 10, 2024 • edited Loading

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

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

hopeyen Oct 16, 2024 • edited Loading

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

hopeyen Oct 10, 2024 •

edited

Loading

hopeyen Oct 16, 2024 •

edited

Loading