refactor: required quorum number contract read #1148
Conversation
| @@ -67,7 +67,11 @@ func NewOnchainPaymentState(ctx context.Context, tx *eth.Reader) (*OnchainPaymen | |||
| } | |||
|
|
|||
| func (pcs *OnchainPaymentState) GetPaymentVaultParams(ctx context.Context) (*PaymentVaultParams, error) { | |||
| quorumNumbers, err := pcs.GetOnDemandQuorumNumbers(ctx) | |||
There was a problem hiding this comment.
don't we need to keep this in order to make the cache work?
There was a problem hiding this comment.
yeah, we still have this on line#74 and it is calling tx directly instead of using the helper function, and the helper function is basically this but with cache
| @@ -192,7 +196,13 @@ func (pcs *OnchainPaymentState) GetOnDemandQuorumNumbers(ctx context.Context) ([ | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| return pcs.tx.GetRequiredQuorumNumbers(ctx, blockNumber) | |||
| quorumNumbers, err := pcs.tx.GetRequiredQuorumNumbers(ctx, blockNumber) | |||
| if err != nil { | |||
core/meterer/onchain_state.go
Outdated
| if err != nil { | ||
| // On demand required quorum is unlikely to change, so we are comfortable using the cached value | ||
| // in case the contract read fails | ||
| return pcs.PaymentVaultParams.Load().OnDemandQuorumNumbers, nil |
There was a problem hiding this comment.
what if pcs.PaymentVaultParams.Load() is nil or pcs.PaymentVaultParams.Load().OnDemandQuorumNumbers is empty?
There was a problem hiding this comment.
hmm I don't think we can get here if payment vault params is nil, because this only get called after Meterer has obtained the onchain state successfully, and can OnDemandQuorumNumbers ever be empty? If the protocol set it as empty on-chain, we should be okay to return an empty array as well and have the later checks be consistent with 'no required quroum numbers', would you agree?
I am going to add the checks anyway for additional security and not rely on the external logic 🤔
hopeyen
force-pushed
the
hope/contract-read-fallback
branch
from
38200f2 to
73d4cf0
Compare
| @@ -192,7 +198,21 @@ func (pcs *OnchainPaymentState) GetOnDemandQuorumNumbers(ctx context.Context) ([ | |||
| if err != nil { | |||
| return nil, err | |||
There was a problem hiding this comment.
do we want to read from cache if this fails too?
There was a problem hiding this comment.
Payment vault doesn't cache block number so we can't read from cache. I don't think it makes sense to cache block number neither, because block number updates should be consistent and constant, while required quorums might never get updated. If we failed to read a block number, I don't think we should continue reading the chain from a historical block
in short, no? if you have other concerns I'm open to change
| @@ -67,7 +67,11 @@ func NewOnchainPaymentState(ctx context.Context, tx *eth.Reader) (*OnchainPaymen | |||
| } | |||
|
|
|||
| func (pcs *OnchainPaymentState) GetPaymentVaultParams(ctx context.Context) (*PaymentVaultParams, error) { | |||
| quorumNumbers, err := pcs.GetOnDemandQuorumNumbers(ctx) | |||
Why are these changes needed?
Disperser client making On-demand request occasionally runs into this failure from the disperser
Since required quorum number is unlikely to change and is refreshed occasionally, I think we are comfortable using the cached value in case the contract read fails, instead of failing the dispersal request
Checks