reservation timestamp check #990
Conversation
hopeyen
force-pushed
the
hope/reservations-time-check
branch
2 times, most recently
from
1c3a952 to
23f862b
Compare
core/data.go
Outdated
| @@ -606,7 +607,7 @@ func ConvertToPaymentMetadata(ph *commonpb.PaymentHeader) *PaymentMetadata { | |||
|
|
|||
| // OperatorInfo contains information about an operator which is stored on the blockchain state, | |||
| // corresponding to a particular quorum | |||
| type ActiveReservation struct { | |||
| type ReservedPayment struct { | |||
| SymbolsPerSecond uint64 // reserve number of symbols per second | |||
| //TODO: we are not using start and end timestamp, add check or remove | |||
There was a problem hiding this comment.
Is this TODO still relevant? Maybe we can update the documentation here for two timestamps.
Also nit: comments should be line above the fields starting with the name of the field
core/data.go
Outdated
| @@ -606,7 +607,7 @@ func ConvertToPaymentMetadata(ph *commonpb.PaymentHeader) *PaymentMetadata { | |||
|
|
|||
| // OperatorInfo contains information about an operator which is stored on the blockchain state, | |||
core/data.go
Outdated
| @@ -625,3 +626,8 @@ type BlobVersionParameters struct { | |||
| MaxNumOperators uint32 | |||
| NumChunks uint32 | |||
| } | |||
|
|
|||
| func (ar *ReservedPayment) IsActive() bool { | |||
| now := uint64(time.Now().Unix()) | |||
There was a problem hiding this comment.
do we have unit tests for active/inactive reservations?
would be useful to take this timestamp as input for testing
core/data.go
Outdated
|
|
||
| func (ar *ReservedPayment) IsActive() bool { | ||
| now := uint64(time.Now().Unix()) | ||
| return ar.StartTimestamp <= now && ar.EndTimestamp >= now |
There was a problem hiding this comment.
are all ReservedPayments guaranteed to have these timestamps populated?
There was a problem hiding this comment.
I think yes, because before we make ReservedPayments, they go through the convert function here
There was a problem hiding this comment.
If reservation has zero timestamps and non-zero values for other fields, looks like it would still get converted successfully
There was a problem hiding this comment.
yes, that is only for valid reservations, but this check should handle checking if the reservation is active or not
There was a problem hiding this comment.
I don't think I understand what are your concerns here 🤔 ?
There was a problem hiding this comment.
My concern is whether it's possible to have a valid reservation that's missing EndTimestamp for example.
There was a problem hiding this comment.
I guess the current logic would ignore those reservations if they're missing any timestamps
hopeyen
force-pushed
the
hope/reservations-time-check
branch
from
fdc6bed to
42b42fa
Compare
Why are these changes needed?
Utilize the start and end timestamp to check if the reservation is active. Updated the name from
ActiveReservationtoReservedPaymentto make it less confusing and consistent withOnDemandPaymentChecks