Skip to content

Conversation

francois-guerin-ledger
Copy link
Contributor

@francois-guerin-ledger francois-guerin-ledger commented Jun 11, 2025

βœ… Checklist

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

πŸ“ Description

Allow developers to target a specific version of the CAL for test purposes.

1000005298 Screenshot 2025-06-12 at 08 59 51
1000005299 Screenshot 2025-06-12 at 09 00 03

Expose SPECULOS_API_PORT as global env, passing by.

❓ 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)

Copy link

vercel bot commented Jun 11, 2025

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 Jun 17, 2025 7:27am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Jun 17, 2025 7:27am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jun 17, 2025 7:27am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Jun 17, 2025 7:27am

@live-github-bot live-github-bot bot added desktop Has changes in LLD mobile Has changes in LLM common Has changes in live-common ledgerjs Has changes in the ledgerjs open source libs translations Translation files have been touched labels Jun 11, 2025
@francois-guerin-ledger francois-guerin-ledger force-pushed the feat/LIVE-14974-cal-select-cal-branch branch from 859dce9 to 3a4eb17 Compare June 11, 2025 22:58
@francois-guerin-ledger francois-guerin-ledger force-pushed the feat/LIVE-14974-cal-select-cal-branch branch from 3a4eb17 to cd18afd Compare June 12, 2025 06:54
themooneer
themooneer previously approved these changes Jun 12, 2025
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
49.5% Coverage on New Code (required β‰₯ 80%)
116 New Code Smells (required ≀ 1)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Comment on lines +91 to +92
signatureKind = "prod",
ref = getEnv("CAL_REF") || undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we use the test signatureKind when not on the main branch ?
Because usually we won't have a prod signature if the branch is not main

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't use test because these signatures are not working at the moment, making this feature impossible to use...
A prod signature has been added on purpose here in order to make it work.
Maybe we can add a TODO explaining the situation ?

Comment on lines +17 to +18
const ref = useEnv("CAL_REF");
const [inputValue, setInputValue] = useState(ref || "branch:next");
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you could add a default value in env. file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Default is empty because in the nominal case, it must be empty as we want the CAL to select its own default (asserting LL always know the default is quite big).
I want this env filled only with an explicit action from the user.

prepareCurrency: (currency: CryptoCurrency) => Promise<unknown | null | undefined>;
prepareCurrency: (
currency: CryptoCurrency,
{ forceUpdate }?: { forceUpdate: boolean },
Copy link
Contributor

Choose a reason for hiding this comment

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

[ASK] why do you need an object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it is easier to understand what happens with prepareCurrency(coin, { forceUpdate: true }) as opposed to prepareCurrency(coin, true) that does not give much context.

@francois-guerin-ledger francois-guerin-ledger merged commit b8d1ab0 into develop Jun 23, 2025
64 of 67 checks passed
@francois-guerin-ledger francois-guerin-ledger deleted the feat/LIVE-14974-cal-select-cal-branch branch June 23, 2025 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Has changes in live-common desktop Has changes in LLD ledgerjs Has changes in the ledgerjs open source libs mobile Has changes in LLM translations Translation files have been touched
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants