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

[ENG-3471] feat: use estimated fees for stx transactions #720

Merged
merged 8 commits into from
Jan 9, 2024

Conversation

teebszet
Copy link
Member

@teebszet teebszet commented Dec 18, 2023

🔘 PR Type

  • Bugfix
  • Enhancement

📜 Background

https://linear.app/xverseapp/issue/ENG-3458

depends on secretkeylabs/xverse-core#323 and
secretkeylabs/xverse-core#335

🔄 Changes

  • update to core version where we use the correct estimateTransaction function for STX transactions (sendStx, sendNft, sendFt)
  • update to core version where we have applyFeeMultiplier logic in one place
  • applyFeeMultiplier also caps the fee at the thresholdHighStacksFee
  • use applyFeeMultiplier in sendFt, sendNft, sendStx, and transactionRequest (popup)

Impact:

  • impacts sendFt, sendNft, sendStx
  • also impacts dapps which trigger a transaction request popup

🖼 Screenshot / 📹 Video

send stx (estimates are returning high fees. screenshot shows capped at threshold: 1STX):
image

send nft:
image

send ft:
image

✅ Review checklist

Please ensure the following are true before merging:

  • Code Style is consistent with the project guidelines.
  • Code is readable and well-commented.
  • No unnecessary or debugging code has been added.
  • Security considerations have been taken into account.
  • The change has been manually tested and works as expected.
  • Breaking changes and their impacts have been considered and documented.
  • Code does not introduce new technical debt or issues.

@teebszet teebszet changed the title Feat/use estimated fees for stx transactions [ENG-3458] feat: use estimated fees for stx transactions Jan 3, 2024
@teebszet teebszet removed the request for review from yknl January 4, 2024 03:48
@teebszet teebszet marked this pull request as draft January 4, 2024 03:49
@teebszet teebszet marked this pull request as ready for review January 4, 2024 08:04
@teebszet teebszet requested review from dhriaznov and removed request for m-aboelenein January 4, 2024 09:42
@teebszet teebszet changed the title [ENG-3458] feat: use estimated fees for stx transactions [ENG-3471] feat: use estimated fees for stx transactions Jan 4, 2024
Copy link
Contributor

@dhriaznov dhriaznov left a comment

Choose a reason for hiding this comment

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

Good job 👍 left a small ask below :)

@@ -55,11 +55,7 @@ function TransactionRequest() {
tokenTransferPayload.amount,
tokenTransferPayload.memo!,
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also add a check for the tokenTransferPayload.memo here to make the code safer?

Copy link
Member Author

Choose a reason for hiding this comment

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

good call. it should be an optional arg in the core function

Copy link
Member Author

Choose a reason for hiding this comment

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

but I don't want to refactor the core function to make it optional, because then I should also reorder the args to have memo?: string come after the required params.

maybe a compromise here is to apply the default earlier like tokenTransferPayload.memo ?? ''

Copy link

github-actions bot commented Jan 8, 2024

@DuskaT021 DuskaT021 added testing QA testing in progress ready for release and removed ready for test testing QA testing in progress labels Jan 9, 2024
@fedeerbes fedeerbes merged commit 2b1b387 into develop Jan 9, 2024
5 checks passed
This was referenced Jan 11, 2024
@teebszet teebszet deleted the feat/use-estimated-fees-for-stx-transactions branch January 18, 2024 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants