[payment] transactor ping payment vault contract #827

hopeyen · 2024-10-22T19:22:43Z

Why are these changes needed?

filling in the transactor functions to read the payment vault contract on-chain

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/chainio.go

ian-shim · 2024-12-03T23:01:52Z

core/data.go

@@ -9,6 +9,7 @@ import (

 	commonpb "github.com/Layr-Labs/eigenda/api/grpc/common"
 	"github.com/Layr-Labs/eigenda/common"
+	paymentvault "github.com/Layr-Labs/eigenda/contracts/bindings/PaymentVault"


can we not have contract dependency on core data structs?

can we do something here?

what do you think about moving type ActiveReservation = paymentvault.IPaymentVaultReservation to core/eth?

How does that remove the contract dependency?

core/eth already has contract dependency, so we are won't introducing it in data.go, we just need to avoid circular dependencies in this case

ian-shim · 2024-12-03T23:02:59Z

core/eth/reader.go

@@ -37,6 +38,7 @@ type ContractBindings struct {
 	EigenDAServiceManager *eigendasrvmg.ContractEigenDAServiceManager
 	EjectionManager       *ejectionmg.ContractEjectionManager
 	AVSDirectory          *avsdir.ContractAVSDirectory
+	PaymentVault          *paymentvault.ContractPaymentVault


should this be initialized in updateContractBindings?

for all the contract comments, I pinged @0x0aa0 to link payment vault to the eigenda service manager contract so that the payment vault contract addr can be queried. Will make the appropriate updates once that's in:)

Cool, are you planning to leave this PR open till the contract code is ready and deployed?
Maybe we should just add nil checks and proceed for now? We can update/remove the nil checks when contracts are ready

I added nil checks before the contract calls so we can go ahead to merge! I think the payment DASM link is almost ready, can work on that as soon as that one gets merged

ian-shim · 2024-12-03T23:03:22Z

core/eth/reader.go

+	}
+
+	reservations_map := make(map[string]core.ActiveReservation)
+	reservations, err := t.bindings.PaymentVault.GetReservations(&bind.CallOpts{


would this crash if PaymentVault is not initialized

core/eth/reader.go

core/chainio.go

ian-shim · 2024-12-03T23:13:17Z

core/eth/reader.go

-	// contract is not implemented yet
-	return core.ActiveReservation{}, nil
+func (t *Reader) GetActiveReservationByAccount(ctx context.Context, accountID string) (core.ActiveReservation, error) {
+	reservation, err := t.bindings.PaymentVault.GetReservation(&bind.CallOpts{


If account is not part of the mapping, contract will return zero value for the reservation instead of an error. We should probably validate that and return err here instead.

Same with other methods in this file that accesses mapping onchain

core/meterer/onchain_state.go

core/eth/reader.go

core/chainio.go

core/meterer/onchain_state.go

core/eth/reader.go

core/meterer/onchain_state.go

ian-shim · 2024-12-05T22:04:12Z

core/eth/reader.go

@@ -37,6 +38,7 @@ type ContractBindings struct {
 	EigenDAServiceManager *eigendasrvmg.ContractEigenDAServiceManager
 	EjectionManager       *ejectionmg.ContractEjectionManager
 	AVSDirectory          *avsdir.ContractAVSDirectory
+	PaymentVault          *paymentvault.ContractPaymentVault


Cool, are you planning to leave this PR open till the contract code is ready and deployed?
Maybe we should just add nil checks and proceed for now? We can update/remove the nil checks when contracts are ready

ian-shim

few last comments

ian-shim · 2024-12-06T19:30:43Z

core/eth/reader.go

+	// filter out all zero-valued reservations
+	for accountID, reservation := range reservationsMap {
+		if isZeroValuedReservation(*reservation) {
+			delete(reservationsMap, accountID)
+		}
+	}


We can move this check inside the for loop above (L675) and only set the value if reservation isn't zero valued

ian-shim · 2024-12-06T19:31:10Z

core/eth/reader.go

+	}
+
+	// filter out all zero-valued payments
+	for accountID, payment := range paymentsMap {


ian-shim · 2024-12-06T19:31:57Z

core/eth/utils.go

+func isZeroValuedReservation(reservation paymentvault.IPaymentVaultReservation) bool {
+	zeroReservation := paymentvault.IPaymentVaultReservation{}
+
+	return reflect.DeepEqual(reservation, zeroReservation)


maps.Equal for performance

I had trouble with this because IPaymentVaultReservation is not a map. is it more performant to put the struct into a map and use maps.Equal?

Ah it's a struct. We could compare each field individually (or i'm fine with using reflect as well).

Do you know if eth rpc will return nil vs. empty byte slice for []byte type fields?
If they return empty byte slice, this comparison will evaluate to false

ian-shim · 2024-12-06T19:34:11Z

core/eth/reader.go

+	if err != nil {
+		return nil, err
+	}
+	if onDemandPayment == big.NewInt(0) {


Has this been tested? This will result in false even if onDemandPayment has value of 0 because you're comparing bigint pointers.
Use something like onDemandPayment.Cmp(big.NewInt(0)) == 0 instead

I don't have test coverage over this. Do we have anywhere that puts tests on contract calls?
(updated to use your suggestion)

ian-shim · 2024-12-06T19:38:21Z

core/meterer/onchain_state.go

-func (pcs *OnchainPaymentState) GetActiveReservationByAccount(ctx context.Context, accountID string) (core.ActiveReservation, error) {
-	if reservation, ok := pcs.ActiveReservations[accountID]; ok {
+func (pcs *OnchainPaymentState) GetActiveReservationByAccount(ctx context.Context, accountID gethcommon.Address) (*core.ActiveReservation, error) {
+	if reservation, ok := (pcs.ActiveReservations)[accountID]; ok {


Don't we need rlock here? I think it can crash if a map is read at the same time it's being written

oof, I added RLock and defer RUnlock

If you defer pcs.ReservationsLock.RUnlock() it will hold the rlock until the end of the execution. Will it cause conflict with write lock in L160?

ian-shim · 2024-12-06T19:39:11Z

core/data.go

@@ -9,6 +9,7 @@ import (

 	commonpb "github.com/Layr-Labs/eigenda/api/grpc/common"
 	"github.com/Layr-Labs/eigenda/common"
+	paymentvault "github.com/Layr-Labs/eigenda/contracts/bindings/PaymentVault"


can we do something here?

ian-shim · 2024-12-09T19:17:53Z

core/eth/reader.go

+	// since reservations are returned in the same order as the accountIDs, we can directly map them
+	for i, reservation := range reservations {
+		if isZeroValuedReservation(reservation) {
+			delete(reservationsMap, accountIDs[i])


If a reservation is zero, this delete would be a no-op and it would be added in L677, no?
Don't we want if !isZeroValuedReservation(reservation) { reservationsMap[accountIDs[i]] = &reservation } ?

ian-shim · 2024-12-09T19:22:49Z

core/eth/utils.go

+func isZeroValuedReservation(reservation paymentvault.IPaymentVaultReservation) bool {
+	zeroReservation := paymentvault.IPaymentVaultReservation{}
+
+	return reflect.DeepEqual(reservation, zeroReservation)


Ah it's a struct. We could compare each field individually (or i'm fine with using reflect as well).

Do you know if eth rpc will return nil vs. empty byte slice for []byte type fields?
If they return empty byte slice, this comparison will evaluate to false

ian-shim · 2024-12-09T19:23:57Z

core/data.go

@@ -9,6 +9,7 @@ import (

 	commonpb "github.com/Layr-Labs/eigenda/api/grpc/common"
 	"github.com/Layr-Labs/eigenda/common"
+	paymentvault "github.com/Layr-Labs/eigenda/contracts/bindings/PaymentVault"


How does that remove the contract dependency?

ian-shim

couple of nits, but lgtm!

ian-shim · 2024-12-10T20:22:39Z

core/eth/utils.go

+// Returns an error if the input reservation is zero-valued.
+func ConvertToActiveReservation(reservation paymentvault.IPaymentVaultReservation) (*core.ActiveReservation, error) {
+	if isZeroValuedReservation(reservation) {
+		return nil, fmt.Errorf("reservation does not exist for given account")


This error message is not accurate given this is a generic util method

updated to say that reservation is not a valid active reservation

ian-shim · 2024-12-10T20:23:58Z

core/eth/reader.go

+	// since payments are returned in the same order as the accountIDs, we can directly map them
+	for i, payment := range payments {
+		if payment.Cmp(big.NewInt(0)) == 0 {
+			t.logger.Warn("failed to get on demand payment for account", "account", accountIDs[i], "err", err)


I don't think err is relevant here?

you are right! removed

hopeyen force-pushed the hope/payment-contract branch from 25199d5 to acec203 Compare November 22, 2024 19:31

hopeyen requested a review from ian-shim November 22, 2024 21:05

ian-shim reviewed Dec 3, 2024

View reviewed changes

ian-shim requested a review from mooselumph December 3, 2024 23:17

ian-shim reviewed Dec 5, 2024

View reviewed changes

core/eth/reader.go Outdated Show resolved Hide resolved

core/chainio.go Outdated Show resolved Hide resolved

core/meterer/onchain_state.go Outdated Show resolved Hide resolved

core/meterer/onchain_state.go Outdated Show resolved Hide resolved

hopeyen force-pushed the hope/payment-contract branch 3 times, most recently from e973ddf to 3e3b64d Compare December 5, 2024 19:00

hopeyen requested a review from ian-shim December 5, 2024 19:17

ian-shim reviewed Dec 5, 2024

View reviewed changes

hopeyen requested a review from ian-shim December 6, 2024 17:22

ian-shim reviewed Dec 6, 2024

View reviewed changes

hopeyen added 12 commits December 6, 2024 12:33

feat: fill in transactor payment contract call

afd077e

fix: use on-chain field names

a1f9d6a

fix: fill in global param contract calls

75320b1

feat: global bin interval calls

eee8f4e

refactor: ptrs, addrs, zero-value mapping check

56c85e1

fix: atomic and map pointers

7fd1c61

fix: lint

28bed77

fix: tests

572ef67

refactor: atomicPointer type

b0d8d51

fix: temporary nil checks for paymentVault contract

49b6559

fix: onchain state type

9b68330

refactor: adding locks and moving checks

573c031

hopeyen force-pushed the hope/payment-contract branch from 8427c85 to 673d81c Compare December 6, 2024 20:33

fix: QuorumSplits typo

34ffa52

hopeyen force-pushed the hope/payment-contract branch from 673d81c to 34ffa52 Compare December 6, 2024 20:52

fix: proto gen

7e9d37e

ian-shim reviewed Dec 9, 2024

View reviewed changes

refactor: define and check ActiveReservation against onchain struct

2c2f893

hopeyen added 2 commits December 10, 2024 12:11

fix: properly check zero valued reservation

91856ce

fix: adding on-chain payments to cached maps

b585fca

hopeyen force-pushed the hope/payment-contract branch from fc716e1 to b585fca Compare December 10, 2024 20:19

ian-shim approved these changes Dec 10, 2024

View reviewed changes

fix: logs

971edc2

hopeyen merged commit 2f5ced8 into master Dec 10, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[payment] transactor ping payment vault contract #827

[payment] transactor ping payment vault contract #827

hopeyen commented Oct 22, 2024

ian-shim Dec 3, 2024

ian-shim Dec 6, 2024

hopeyen Dec 6, 2024

ian-shim Dec 9, 2024

hopeyen Dec 9, 2024

ian-shim Dec 3, 2024

hopeyen Dec 4, 2024

ian-shim Dec 5, 2024

hopeyen Dec 6, 2024

ian-shim Dec 3, 2024

ian-shim Dec 3, 2024

ian-shim Dec 5, 2024

ian-shim left a comment

ian-shim Dec 6, 2024

ian-shim Dec 6, 2024

ian-shim Dec 6, 2024

hopeyen Dec 6, 2024

ian-shim Dec 9, 2024

ian-shim Dec 6, 2024

hopeyen Dec 6, 2024

ian-shim Dec 6, 2024

hopeyen Dec 6, 2024

ian-shim Dec 10, 2024

ian-shim Dec 6, 2024

ian-shim Dec 9, 2024

ian-shim Dec 9, 2024

ian-shim Dec 9, 2024

ian-shim left a comment

ian-shim Dec 10, 2024

hopeyen Dec 10, 2024

ian-shim Dec 10, 2024

hopeyen Dec 10, 2024

[payment] transactor ping payment vault contract #827

[payment] transactor ping payment vault contract #827

Conversation

hopeyen commented Oct 22, 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

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

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