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.

Sign up for GitHub

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

Jump to bottom

Feat/live 15551 aptos refactor cache for get estimated gas #8848

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open

Feat/live 15551 aptos refactor cache for get estimated gas #8848

wants to merge 15 commits into from
+99 −450

Conversation

may01
Copy link
Contributor

@may01 may01 commented Jan 8, 2025

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • ...

📝 Description

The cache for transaction fees was broken, and after the removal of the advanced transaction settings, there is unnecessary logic and structures remaining in the code.

Issues that were resolved:

  • The cached value was stored to the wrong key in some cases.
  • Removed redundant call to estimate the gas price. Now, the price is taken from the simulation.
  • Optional parameters corresponding to advanced user input have been removed.
  • Previously, the simulation used default parameters for the first call and estimated parameters for subsequent calls, which created potential side effects. Now, all simulations use default parameters (gasPrice, gasAmount).
  • If the simulation fails, we now set default values instead of using the latest estimated value.
  • The transaction status check has been updated to use only relevant parameters.

❓ Context

  • JIRA or GitHub link:

🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@may01 may01 requested a review from a team as a code owner January 8, 2025 23:41
@vercel Vercel
Copy link

vercel bot commented Jan 8, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
web-tools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 9, 2025 5:15pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Jan 9, 2025 5:15pm
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jan 9, 2025 5:15pm
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jan 9, 2025 5:15pm

@live-github-bot live-github-bot bot added the common Has changes in live-common label Jan 8, 2025
@may01 may01 mentioned this pull request Jan 8, 2025
Merged
3 tasks
semeano
semeano previously approved these changes Jan 9, 2025
View reviewed changes
@may01 may01 requested review from jccguimaraes and semeano January 9, 2025 14:11
@may01 may01 requested review from a team as code owners January 9, 2025 14:27
@live-github-bot live-github-bot bot added desktop Has changes in LLD mobile Has changes in LLM ui Has changes in the design system library ledgerjs Has changes in the ledgerjs open source libs automation CI/CD stuff translations Translation files have been touched screenshots Screenshots have been updated labels Jan 9, 2025
@ledger-wiz-cspm-secret-detection
Copy link

ledger-wiz-cspm-secret-detection bot commented Jan 9, 2025 •
edited
Loading

Wiz Scan Summary

Scanner Findings
Data Finding Sensitive Data
Secret Finding Secrets
IaC Misconfiguration IaC Misconfigurations
Total

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

@ledger-wiz-cspm-secret-detection
Copy link

Wiz Scan Summary

Scanner Findings
Data Finding Sensitive Data 2 Info
Secret Finding Secrets
IaC Misconfiguration IaC Misconfigurations
Total 2 Info

View scan details in Wiz

To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension.

@may01 may01 force-pushed the feat/LIVE-15551-aptos-refactor-cache-for-get-estimated-gas branch from 7234088 to d3e36a5 Compare January 9, 2025 14:32
@live-github-bot live-github-bot bot removed desktop Has changes in LLD mobile Has changes in LLM ui Has changes in the design system library ledgerjs Has changes in the ledgerjs open source libs automation CI/CD stuff translations Translation files have been touched screenshots Screenshots have been updated labels Jan 9, 2025
jccguimaraes
jccguimaraes requested changes Jan 9, 2025
View reviewed changes
Copy link
Member

@jccguimaraes jccguimaraes left a comment

Choose a reason for hiding this comment

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

Also check uninported file (cicd) error

libs/ledger-live-common/src/families/aptos/types.ts Outdated Show resolved Hide resolved
libs/ledger-live-common/src/families/aptos/logic.test.ts Outdated Show resolved Hide resolved
libs/ledger-live-common/src/families/aptos/getFeesForTransaction.ts Show resolved Hide resolved
@may01 may01 requested review from jccguimaraes and semeano January 9, 2025 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jccguimaraes jccguimaraes Awaiting requested review from jccguimaraes

@semeano semeano Awaiting requested review from semeano

Requested changes must be addressed to merge this pull request.

Assignees

@may01 may01

Labels
common Has changes in live-common
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@may01 @semeano @jccguimaraes