Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add swap fee to FeeDetailsBottomSheet #6025

Merged
merged 26 commits into from
Sep 19, 2024
Merged

feat: add swap fee to FeeDetailsBottomSheet #6025

merged 26 commits into from
Sep 19, 2024

Conversation

MuckT
Copy link
Collaborator

@MuckT MuckT commented Sep 17, 2024

Description

Adds the swap fee if available to the FeedDetailsBottomSheet used in EarnEnterAmount.tsx.

iOS w Swap Android w Swap iOS w/o Swap Android w/o Swap

Test plan

  • Unit tests updated
  • Tested locally on iOS
  • Tested locally on Android

Related issues

Backwards compatibility

Yes

Network scalability

Yes

Copy link

codecov bot commented Sep 17, 2024

Codecov Report

Attention: Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.69%. Comparing base (9817785) to head (8dc832c).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/earn/EarnEnterAmount.tsx 94.73% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6025      +/-   ##
==========================================
+ Coverage   88.68%   88.69%   +0.01%     
==========================================
  Files         727      727              
  Lines       30723    30746      +23     
  Branches     5596     5301     -295     
==========================================
+ Hits        27246    27271      +25     
- Misses       3277     3432     +155     
+ Partials      200       43     -157     
Files with missing lines Coverage Δ
src/components/RowDivider.tsx 100.00% <100.00%> (ø)
src/earn/EarnEnterAmount.tsx 87.05% <94.73%> (+0.61%) ⬆️

... and 70 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9817785...8dc832c. Read the comment docs.

locales/base/translation.json Outdated Show resolved Hide resolved
@@ -659,14 +687,40 @@ function FeeDetailsBottomSheet({
)}
</View>
</View>
<RowDivider style={{ marginBottom: Spacing.Large32 }} />
<View style={styles.gap8}>
<RowDivider />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we also remove the style prop from RowDivider now that it's not used? Would prefer to keep overrides at a minimum where possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in d6d56d3!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this commit on a different branch / PR?

Comment on lines 635 to 637
return new BigNumber(swapTransaction.buyAmount)
.multipliedBy(new BigNumber(swapTransaction.appFeePercentageIncludedInPrice).shiftedBy(-2)) // To convert from percentage to decimal
.shiftedBy(-depositToken.decimals)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the app fee is a % of the from amount, not the to amount. So this should be using the amount the user inputs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fff206b!

<TokenDisplay tokenId={token.tokenId} amount={swapFeeAmount.toString()} />
{' ('}
<TokenDisplay
tokenId={depositToken.tokenId}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

related to above comment, this should be the swap from token

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fff206b!

Base automatically changed from tomm/act-1357 to main September 19, 2024 03:31
MuckT and others added 2 commits September 18, 2024 22:18
# Conflicts:
#	locales/base/translation.json
#	src/earn/EarnEnterAmount.test.tsx
#	src/earn/EarnEnterAmount.tsx

const swapFeeAmount = useMemo(() => {
if (swapTransaction && swapTransaction.appFeePercentageIncludedInPrice) {
return new BigNumber(swapTransaction.buyAmount)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buyAmount is the swap to amount. Can we use the user inputted amount (tokenAmount from the parent component)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 4431acd!

@@ -659,14 +687,40 @@ function FeeDetailsBottomSheet({
)}
</View>
</View>
<RowDivider style={{ marginBottom: Spacing.Large32 }} />
<View style={styles.gap8}>
<RowDivider />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this commit on a different branch / PR?

@MuckT MuckT added this pull request to the merge queue Sep 19, 2024
Merged via the queue into main with commit f27172b Sep 19, 2024
15 checks passed
@MuckT MuckT deleted the tomm/act-1357-1 branch September 19, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants