fix: tx calc unhandled errors, closes #4941 #5011
Conversation
src/app/pages/send/ordinal-inscription/coinselect/select-inscription-coins.ts
Outdated
Show resolved
Hide resolved
src/app/pages/send/ordinal-inscription/coinselect/select-inscription-coins.ts
Outdated
Show resolved
Hide resolved
alter-eggo
force-pushed
the
fix/tx-size-error
branch
from
b354027 to
7bdabba
Compare
alter-eggo
force-pushed
the
fix/tx-size-error
branch
from
7bdabba to
0fdaafd
Compare
alter-eggo
requested review from
kyranjamie,
pete-watters,
fbwoolf and
314159265359879
| @@ -75,36 +79,46 @@ export function useBitcoinFeesList({ | |||
| feeRate: feeRates.hourFee.toNumber(), | |||
| }; | |||
|
|
|||
| const feesArr = []; | |||
There was a problem hiding this comment.
Maybe you can spread values into this array instead?
...(highFeeValue ? [{
label: BtcFeeType.High,
value: highFeeValue,
btcValue: formatMoneyPadded(createMoney(highFeeValue, 'BTC')),
time: btcTxTimeMap.fastestFee,
fiatValue: getFiatFeeValue(highFeeValue),
feeRate: feeRates.fastestFee.toNumber(),
}] : [])
]
There was a problem hiding this comment.
I made it so to improve readability. don't really like such constructions even if they save several lines of code
| @@ -37,11 +37,11 @@ describe(selectInscriptionTransferCoins.name, () => { | |||
| Number(inscriptionInputAmount) | |||
| ); | |||
|
|
|||
| expect(result.txFee).toEqual(6765); | |||
| expect(result.txFee).toEqual(5048); | |||
There was a problem hiding this comment.
How is this value of 5048 decided? Maybe we can give it a const name for readability?
There was a problem hiding this comment.
since I will refactor fees gen, will return to this test in future pr. thanks Pete!
|
|
||
| const feesArr = []; | ||
|
|
||
| if (highFeeValue) { |
There was a problem hiding this comment.
This seems almost the same as - https://github.com/leather-wallet/extension/pull/5011/files#r1514527890
Maybe we can extract and share logic?
There was a problem hiding this comment.
yeah, it makes sense in general, but I will change the fees gen logic in future prs, so prob better to leave it for now
There was a problem hiding this comment.
Thanks for fixing @alter-eggo
This is good for now but this feature clearly needs a refactor. Look forward to seeing what you come up with next
| function processFeeValue(feeRate: string) { | ||
| try { | ||
| const feeValues = getCustomFeeValues(Number(feeRate)); | ||
| setFeeValue(feeValues); | ||
|
|
||
| setUnknownError(false); | ||
| setCustomInsufficientBalanceError(false); | ||
| } catch (err) { | ||
| if (err instanceof InsufficientFundsError) { | ||
| return setCustomInsufficientBalanceError(true); | ||
| } | ||
|
|
||
| setUnknownError(true); | ||
| } | ||
| } | ||
|
|
||
| function onChange(e: React.ChangeEvent<HTMLInputElement>) { | ||
| const value = e.target.value; | ||
| setCustomFeeInitialValue?.(e.target.value); | ||
| processFeeValue(value); | ||
| } | ||
|
|
||
| useOnMount(() => { | ||
| processFeeValue(customFeeInitialValue); | ||
| }); |
There was a problem hiding this comment.
Processing the fee on mount, then setting the values, like this seems kinda side-effecty. Okay for now as we're going to do fee work and refactor entirely, but I'm sure there's a nicer way to do this next time.
There was a problem hiding this comment.
yes, I realise it could have been done way better
This pr handles unprocessed fees calc error in bitcoin flows:
Also: