fix: preserve swap amounts when scanning QR codes #3612
Conversation
Summary of ChangesHello @shubhamkmr04, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a bug related to scanning QR codes for swap operations. Previously, the application failed to correctly process and display swap amounts embedded in QR codes, or would reset the swap form upon returning from the scanner. The changes ensure that scanned amounts are accurately captured, passed, and displayed on the Swaps screen, and that the swap state is preserved during the QR scanning process. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the bug where swap amounts were not preserved when scanning QR codes. The changes correctly handle cases where initialAmountSats might be undefined, ensuring that the invoice and reverse swap settings are still applied. Additionally, the logic for displaying fiat and BTC amounts has been improved to correctly format values based on the selected unit. The introduction of isNavigatingToQRScanner for state management also enhances the user experience by preventing unintended field resets when returning from the QR scanner.
| if (initialInvoice !== undefined && initialReverse !== undefined) { | ||
| if (initialAmountSats === undefined) { | ||
| this.setState( | ||
| { | ||
| paramsProcessed: true, | ||
| invoice: initialInvoice, | ||
| reverse: initialReverse, | ||
| outputSats: newOutputSats, | ||
| outputFiat: | ||
| units === 'fiat' | ||
| ? newOutputFiat | ||
| : this.state.outputFiat, | ||
| inputSats: newInputSats, | ||
| inputFiat: | ||
| units === 'fiat' | ||
| ? newInputFiat | ||
| : this.state.inputFiat, | ||
| serviceFeeSats: newServiceFeeSats | ||
| reverse: initialReverse | ||
| }, | ||
| () => this.checkIsValid() | ||
| ); | ||
| }); | ||
| } else { | ||
| this.setState({ paramsProcessed: true }, async () => { | ||
| // Use callback to ensure paramsProcessed is set before further calcs | ||
| const { units } = UnitsStore; | ||
| const { fiatRates } = FiatStore; | ||
| const { fiat } = settings; | ||
| const fiatEntry = | ||
| fiat && fiatRates | ||
| ? fiatRates.filter( | ||
| (entry: any) => entry.code === fiat | ||
| )[0] | ||
| : null; | ||
| const rate = | ||
| fiat && fiatRates && fiatEntry ? fiatEntry.rate : 0; | ||
|
|
||
| const info: any = initialReverse | ||
| ? SwapStore.reverseInfo | ||
| : SwapStore.subInfo; | ||
| const serviceFeePct = info?.fees?.percentage || 0; | ||
| const networkFeeBigNum = initialReverse | ||
| ? new BigNumber( | ||
| info?.fees?.minerFees?.claim || 0 | ||
| ).plus(info?.fees?.minerFees?.lockup || 0) | ||
| : new BigNumber(info?.fees?.minerFees || 0); | ||
| const networkFee = networkFeeBigNum.toNumber(); | ||
|
|
||
| const newOutputSats = new BigNumber( | ||
| initialAmountSats || 0 | ||
| ); | ||
| let newOutputFiat = ''; | ||
| if (newOutputSats.isGreaterThan(0)) { | ||
| if (units === 'fiat' && rate > 0) { | ||
| newOutputFiat = newOutputSats | ||
| .div(SATS_PER_BTC) | ||
| .times(rate) | ||
| .toFixed(2); | ||
| } else if (units === 'BTC') { | ||
| newOutputFiat = newOutputSats | ||
| .div(SATS_PER_BTC) | ||
| .toFixed(8); | ||
| } else if (units === 'sats') { | ||
| newOutputFiat = newOutputSats.toFixed(0); | ||
| } | ||
| } | ||
|
|
||
| const newInputSats = calculateSendAmount( | ||
| newOutputSats, | ||
| serviceFeePct, | ||
| networkFee, | ||
| this.state.reverse | ||
| ); | ||
| let newInputFiat = ''; | ||
| if (newInputSats.isGreaterThan(0)) { | ||
| if (units === 'fiat' && rate > 0) { | ||
| newInputFiat = newInputSats | ||
| .div(SATS_PER_BTC) | ||
| .times(rate) | ||
| .toFixed(2); | ||
| } else if (units === 'BTC') { | ||
| newInputFiat = newInputSats | ||
| .div(SATS_PER_BTC) | ||
| .toFixed(8); | ||
| } else if (units === 'sats') { | ||
| newInputFiat = newInputSats.toFixed(0); | ||
| } | ||
| } | ||
|
|
||
| const newServiceFeeSats = calculateServiceFeeOnSend( | ||
| newInputSats, | ||
| serviceFeePct, | ||
| networkFee, | ||
| this.state.reverse | ||
| ); | ||
|
|
||
| this.setState( | ||
| { | ||
| invoice: initialInvoice, | ||
| reverse: initialReverse, | ||
| outputSats: newOutputSats, | ||
| outputFiat: newOutputFiat, | ||
| inputSats: newInputSats, | ||
| inputFiat: newInputFiat, | ||
| serviceFeeSats: newServiceFeeSats | ||
| }, | ||
| () => this.checkIsValid() | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This refactoring correctly handles the scenario where initialAmountSats might be undefined when scanning a QR code. By separating the logic, it ensures that the invoice and reverse status are set even without an amount, and only proceeds with amount calculations when initialAmountSats is present. The enhanced newOutputFiat and newInputFiat calculations also correctly handle different unit displays (fiat, BTC, sats), improving the accuracy of displayed amounts.
if (initialInvoice !== undefined && initialReverse !== undefined) {
if (initialAmountSats === undefined) {
this.setState(
{
paramsProcessed: true,
invoice: initialInvoice,
reverse: initialReverse
},
() => this.checkIsValid()
);
} else {
this.setState({ paramsProcessed: true }, async () => {
// Use callback to ensure paramsProcessed is set before further calcs
const { units } = UnitsStore;
const { fiatRates } = FiatStore;
const { fiat } = settings;
const fiatEntry =
fiat && fiatRates
? fiatRates.filter(
(entry: any) => entry.code === fiat
)[0]
: null;
const rate =
fiat && fiatRates && fiatEntry ? fiatEntry.rate : 0;
const info: any = initialReverse
? SwapStore.reverseInfo
: SwapStore.subInfo;
const serviceFeePct = info?.fees?.percentage || 0;
const networkFeeBigNum = initialReverse
? new BigNumber(
info?.fees?.minerFees?.claim || 0
).plus(info?.fees?.minerFees?.lockup || 0)
: new BigNumber(info?.fees?.minerFees || 0);
const networkFee = networkFeeBigNum.toNumber();
const newOutputSats = new BigNumber(
initialAmountSats || 0
);
let newOutputFiat = '';
if (newOutputSats.isGreaterThan(0)) {
if (units === 'fiat' && rate > 0) {
newOutputFiat = newOutputSats
.div(SATS_PER_BTC)
.times(rate)
.toFixed(2);
} else if (units === 'BTC') {
newOutputFiat = newOutputSats
.div(SATS_PER_BTC)
.toFixed(8);
} else if (units === 'sats') {
newOutputFiat = newOutputSats.toFixed(0);
}
}
const newInputSats = calculateSendAmount(
newOutputSats,
serviceFeePct,
networkFee,
this.state.reverse
);
let newInputFiat = '';
if (newInputSats.isGreaterThan(0)) {
if (units === 'fiat' && rate > 0) {
newInputFiat = newInputSats
.div(SATS_PER_BTC)
.times(rate)
.toFixed(2);
} else if (units === 'BTC') {
newInputFiat = newInputSats
.div(SATS_PER_BTC)
.toFixed(8);
}
else if (units === 'sats') {
newInputFiat = newInputSats.toFixed(0);
}
}
const newServiceFeeSats = calculateServiceFeeOnSend(
newInputSats,
serviceFeePct,
networkFee,
this.state.reverse
);
this.setState(
{
invoice: initialInvoice,
reverse: initialReverse,
outputSats: newOutputSats,
outputFiat: newOutputFiat,
inputSats: newInputSats,
inputFiat: newInputFiat,
serviceFeeSats: newServiceFeeSats
},
() => this.checkIsValid()
);
});
}
}
|
Previously outputFiat/inputFiat were only updated when units === 'fiat'. Now they're always set regardless of unit type. Is that intentional? If so, might be better renaming the variables. |
2 participants
Description
isNavigatingToQRScannerflag to skipresetFields()when navigating to scannerinitialAmountSatswhen present, handle address-only params without overwriting existing amountsChecklist
yarn run tscand made sure my code compiles correctlyyarn run lintand made sure my code didn’t contain any problematic patternsyarn run prettierand made sure my code is formatted correctlyyarn run testand made sure all of the tests passTesting
If you modified or added a utility file, did you add new unit tests?
I have tested this PR on the following platforms (please specify OS version and phone model/VM):
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
Locales
Third Party Dependencies and Packages
yarnafter this PR is merged inpackage.jsonandyarn.lockhave been properly updatedOther: