Add Max Payout Flag #2238
Add Max Payout Flag #2238
Conversation
tools/settlement/bitflyer/upload.go
Outdated
| dryRun *bitflyer.DryRunOption, | ||
| token string, | ||
| settlementRequests []settlement.AggregateTransaction, | ||
| ) (*bitflyer.WithdrawToDepositIDBulkPayload, error) { | ||
| set := []custodian.Transaction{} | ||
| sourceFrom := "" | ||
| maxAmountInterface := ctx.Value(appctx.PayoutTxnMaxAmountCTXKey) | ||
| maxAmount, ok := maxAmountInterface.(decimal.Decimal) |
There was a problem hiding this comment.
I have not seen us anywhere parse from a string value into a decimal.Decimal. Is it missing? From the looks of it, it is.
tools/settlement/cmd/uphold.go
Outdated
| maxAmountInterface := ctx.Value(appctx.PayoutTxnMaxAmountCTXKey) | ||
| maxAmount, ok := maxAmountInterface.(decimal.Decimal) |
There was a problem hiding this comment.
Similar comment as above – nowhere do we parse the string value into a decimal and put it in the context. It's a string here, hence the type assertion never succeeds.
Also, it's a waste to do this step in the loop – is the limit going to change between iterations? Seems unlikely, therefore it must be terminated outside of the loop.
There was a problem hiding this comment.
You are, of course, correct about the loop. Fixed.
tools/settlement/gemini/upload.go
Outdated
| maxAmountInterface := ctx.Value(appctx.PayoutTxnMaxAmountCTXKey) | ||
| maxAmount, ok := maxAmountInterface.(decimal.Decimal) |
There was a problem hiding this comment.
Similar to the case above, two issues – it's not a decimal yet, and we are wasting resources doing the same work in the loop.
| maxAmountAny := ctx.Value(appctx.PayoutTxnMaxAmountCTXKey) | ||
| maxAmount, ok := maxAmountAny.(decimal.Decimal) | ||
| if !ok { | ||
| return nil, errors.New("provided max amount is not an integer") |
There was a problem hiding this comment.
If there's any intent to make this unit-testable in the future, then replacing this with a pre-defined sentinel error (which can be tested against) would be good.
| maxAmountAny := ctx.Value(appctx.PayoutTxnMaxAmountCTXKey) | ||
| maxAmount, ok := maxAmountAny.(decimal.Decimal) | ||
| if !ok { | ||
| logger.Panic().Err(err).Msg("provided max amount is not a number") |
There was a problem hiding this comment.
Nit: Surrounding places to use Panic, but the function we are in does return an error. We could return and error and handle it in the caller if we wanted a panic. But also, what value does a panic over error bring here?
| for i := 0; i < total; i++ { | ||
| settlementTransaction := &settlementState.Transactions[i] | ||
|
|
||
| if settlementTransaction.IsComplete() || settlementTransaction.IsFailed() { | ||
| continue | ||
| } | ||
|
|
||
| if settlementTransaction.Amount.GreaterThan(maxAmount) { | ||
| logger.Panic().Err(err).Msgf("amount %s exceeds the max %s", settlementTransaction.Amount, maxAmount) |
There was a problem hiding this comment.
Re: panic – same comment as above.
Co-authored-by: Pavel Brm <[email protected]>
Co-authored-by: Pavel Brm <[email protected]>
Co-authored-by: Pavel Brm <[email protected]>
|
@Sneagan Can this be closed? |
Summary
Add a max payout flag for existing payout tooling.
Type of Change
Tested Environments
Before Requesting Review