fix: properly set fields without onchain correspondance #1014
Conversation
hopeyen
force-pushed
the
hope/payment-state-nil-handling
branch
from
1bb83ad to
0767072
Compare
| Index: record.Index, | ||
| Usage: record.Usage, | ||
| } | ||
|
|
There was a problem hiding this comment.
can we document this behavior (which fields are optional and what the consequences are)?
api/clients/v2/accountant.go
Outdated
| binRecords := make([]BinRecord, len(paymentState.GetBinRecords())) | ||
| for i, record := range paymentState.GetBinRecords() { | ||
| if record == nil { | ||
| binRecords[i] = *DummyBinRecord() |
There was a problem hiding this comment.
nit:
binRecords[i] = BinRecord{
Index: 0,
Usage: 0,
}
?
disperser/apiserver/server_v2.go
Outdated
| } | ||
| // on-Chain account state | ||
| reservation, err := s.meterer.ChainPaymentState.GetReservedPaymentByAccount(ctx, accountID) | ||
| if err != nil { | ||
| return nil, api.NewErrorNotFound("failed to get active reservation") | ||
| s.logger.Debug("failed to get onchain reservation, use zero values", err) | ||
| reservation = core.DummyReservedPayment() |
There was a problem hiding this comment.
why don't we leave them as nil?
There was a problem hiding this comment.
The way we build reply struct assumes that there is a valid nonnil object. Would it make more sense to set the inner fields here and use those when we construct the reply?
There was a problem hiding this comment.
Does the reply need to return zero valued object? Could we return nil instead?
Accountant handles nil reservation/ondemand payment here
There was a problem hiding this comment.
We could return nil, it is just that we will add a separate step to convert cached struct to protobuf structs outside of the reply construction.
This seems like a cleaner behavior, so I will update to do that
disperser/apiserver/server_v2.go
Outdated
| } | ||
| largestCumulativePayment, err := s.meterer.OffchainStore.GetLargestCumulativePayment(ctx, req.AccountId) | ||
| if err != nil { | ||
| return nil, api.NewErrorNotFound("failed to get largest cumulative payment") | ||
| s.logger.Debug("failed to get largest cumulative payment, use zero value", err) |
There was a problem hiding this comment.
nit: I think it would be helpful to log accountIDs
There was a problem hiding this comment.
updated to include accountIDs and use labels
disperser/apiserver/server_v2.go
Outdated
| @@ -254,20 +255,44 @@ func (s *DispersalServerV2) GetPaymentState(ctx context.Context, req *pb.GetPaym | |||
| currentReservationPeriod := meterer.GetReservationPeriod(now, reservationWindow) | |||
| binRecords, err := s.meterer.OffchainStore.GetBinRecords(ctx, req.AccountId, currentReservationPeriod) | |||
| if err != nil { | |||
| return nil, api.NewErrorNotFound("failed to get active reservation") | |||
| s.logger.Debug("failed to get reservation records, use placeholders", err, accountID) | |||
There was a problem hiding this comment.
use labels:
s.logger.Debug("failed to get reservation records, use placeholders", "err ", err, "accountID", accountID)
disperser/apiserver/server_v2.go
Outdated
| } | ||
| // on-Chain account state | ||
| pbReservation := &pb.Reservation{} |
There was a problem hiding this comment.
if we declare it like var pbReservation *pb.Reservation, we don't have to set pbReservation = nil in L269
core/data.go
Outdated
| type OnDemandPayment struct { | ||
| // Total amount deposited by the user | ||
| CumulativePayment *big.Int | ||
| } | ||
|
|
||
| func DummyOnDemandPayment() *OnDemandPayment { |
api/clients/v2/accountant.go
Outdated
| } | ||
|
|
||
| if paymentState.GetReservation() == nil { | ||
| a.reservation = core.DummyReservedPayment() |
There was a problem hiding this comment.
nit: could we explicitly create empty object
a.reservation = &ReservedPayment{
SymbolsPerSecond: 0,
StartTimestamp: 0,
EndTimestamp: 0,
QuorumNumbers: []uint8{},
QuorumSplits: []byte{},
}
and remove DummyReservedPayment?
Why are these changes needed?
Checks