fix: cumulative payment dynamo db unit conversion #979
Conversation
hopeyen
force-pushed
the
hope/payments-db-fix
branch
from
5eab313 to
1a58cb6
Compare
core/meterer/meterer.go
Outdated
| func (m *Meterer) PaymentCharged(numSymbols uint) uint64 { | ||
| return uint64(m.SymbolsCharged(numSymbols)) * uint64(m.ChainPaymentState.GetPricePerSymbol()) | ||
| func (m *Meterer) PaymentCharged(numSymbols uint) *big.Int { | ||
| return big.NewInt(int64(m.SymbolsCharged(numSymbols) * m.ChainPaymentState.GetPricePerSymbol())) |
There was a problem hiding this comment.
can m.SymbolsCharged(numSymbols) * m.ChainPaymentState.GetPricePerSymbol() ever be greater than max int64?
There was a problem hiding this comment.
Highly unlikely 😬 If we induct backwards to find what's allowed by uint64 with our expected price per symbol 447,000,000 then the number of symbols uint64 can charge is 2^64/477,000,000=41,267,883,833.8. With 1 symbol being 32 bytes, we are going to allow 1,320,572,282,681.7bytes=1.3TB per blob
The reason I updated this is for consistency in the cumulativePayment calculations, but I'm happy to revert these changes and just convert the numbers to big.Int if you think that's better?
There was a problem hiding this comment.
Yeah converting each values to big int and using num0.Mul(num1) seems more safe?
no, I think the changes are only how we read from dynamodb. Writing to the db is still the same as before |
core/meterer/offchain_store.go
Outdated
| prevPayment, err = strconv.ParseUint(smallerResult[0]["CumulativePayments"].(*types.AttributeValueMemberN).Value, 10, 64) | ||
| if err != nil { | ||
| return 0, 0, 0, fmt.Errorf("failed to parse previous payment: %w", err) | ||
| setPrevPayment, success := prevPayment.SetString(smallerResult[0]["CumulativePayments"].(*types.AttributeValueMemberN).Value, 10) |
There was a problem hiding this comment.
I know these values are all expected in the happy path, but this chain seems dangerous
It's missing at least a couple validations: checking if "CumulativePayments" exists in the map and the result of the type assertion
Here and below as well
There was a problem hiding this comment.
added explicit checking for them!
core/meterer/meterer.go
Outdated
| func (m *Meterer) PaymentCharged(numSymbols uint) uint64 { | ||
| return uint64(m.SymbolsCharged(numSymbols)) * uint64(m.ChainPaymentState.GetPricePerSymbol()) | ||
| func (m *Meterer) PaymentCharged(numSymbols uint) *big.Int { | ||
| return big.NewInt(int64(m.SymbolsCharged(numSymbols) * m.ChainPaymentState.GetPricePerSymbol())) |
There was a problem hiding this comment.
Yeah converting each values to big int and using num0.Mul(num1) seems more safe?
hopeyen
force-pushed
the
hope/payments-db-fix
branch
from
1a58cb6 to
f53dae8
Compare
Why are these changes needed?
Quick PR to move away from using dynamoDB
cumulativePaymentvalue touint64usingstrconv.ParseUint; directly convert to big.IntChecks