chore: remove unused Account#freshAddresses field #6523

gre · 2024-03-22T13:58:13Z

✅ Checklist

npx changeset was attached. (no need)
Covered by automatic tests. if it builds, it's good. the field is not used.
Impact of the changes: no changes

📝 Description

freshAddresses field on Account is not used today nor planned by product anytime soon, so we can completely drop the feature for now. this work is part of Account model simplification.

❓ Context

JIRA or GitHub link: https://ledgerhq.atlassian.net/browse/LIVE-11772

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

vercel · 2024-03-22T13:58:23Z

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	Apr 25, 2024 2:18pm

4 Ignored Deployments

Name	Status	Preview	Updated (UTC)
ledger-live-docs	⬜️ Ignored (Inspect)	Visit Preview	Apr 25, 2024 2:18pm
ledger-live-github-bot	⬜️ Ignored (Inspect)	Visit Preview	Apr 25, 2024 2:18pm
native-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	Apr 25, 2024 2:18pm
react-ui-storybook	⬜️ Ignored (Inspect)	Visit Preview	Apr 25, 2024 2:18pm

KVNLS

@gre do you know if integration tests are failing following your changes?

sprohaszka-ledger

There are some strange modification in the snapshots file.

I wonder if it can be interesting to keep the Account type to put together the freshAddress and freshAddressPath together, as some functions return this type.

libs/ledger-live-common/src/families/near/__snapshots__/bridge.integration.test.ts.snap

...edger-live-common/src/families/cosmos/datasets/__snapshots__/coreum.integration.test.ts.snap

gre · 2024-03-26T16:13:51Z

in this PR, i have huge changes due to re-running the tests to have up to date snapshots.
the problem is that, many of these snapshots are already outdated on develop.
so maybe, i will change a bit the strategy on this and first send a PR that just update the snapshot against current's develop?

as reminder, live-common's "integration tests" are defective on develop as of today, they just don't always run. So maybe, i could try to send a PR that restore them / disable the broken ones?

that might highlights the PR feebacks from you both @sprohaszka-ledger @KVNLS because some of these changes actually are not related to the field removal.

gre · 2024-03-29T14:07:21Z

^FYI this PR is in standby until we solve this. i think we should fix the tests on develop?

github-actions · 2024-04-14T04:30:43Z

There as been no activity on this PR for the last 14 days. Please consider closing this PR.

ledger-wiz-cspm-secret-detection · 2024-04-25T07:00:20Z

Wiz Scan Summary


IaC Misconfigurations
Sensitive Data
Total
Secrets

gre · 2024-04-25T14:17:22Z

This PR is ready again, develop is up to date with integration tests 🤞

KVNLS

LGTM 🚀 Nice cleanup!

sprohaszka-ledger

One quick last comment

sprohaszka-ledger · 2024-04-26T07:37:13Z

libs/ledger-live-common/src/families/stacks/bridge/utils/misc.ts

@@ -1,6 +1,6 @@
 import { BigNumber } from "bignumber.js";
 import flatMap from "lodash/flatMap";
-import { Account, Address, Operation, OperationType } from "@ledgerhq/types-live";


Why remove it and change the signature of the getAddress method? It is more synthetic

this type { freshAddress, freshAddressPath } and getAddress function is now strictly internal to the coin, i didn't want to change it too much, but it's possible that coin could simplify even further this: there is no need to return Address anymore there, maybe getAddress(account) usages could just be replaced by account.freshAddress , like we don't even need this indirection anymore since there is no ambiguity on where to look for the address/path, since we don't have to push them into a freshAddresses array.
Anyway as it is not used externally, on types-live perspective there are no need to have this concept of 'Address' pair, so indeed took a direction to remove what's not useful from the type side.
we could always reintroduce the concept if we want in future, like if we want to reorganize the field together, but today in the Account type, they are just nested as part of Account.
also to be clear: there is no type regression on this, we still have the same type safety, it allows coins to do the type they want.

chore: remove unused Account#freshAddresses

gre requested review from a team as code owners March 22, 2024 13:58

github-actions bot assigned gre Mar 22, 2024

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 cli labels Mar 22, 2024

vercel bot deployed to Preview – web-tools March 22, 2024 14:01 View deployment

gre force-pushed the live-11772 branch from 090b145 to 5aaeef1 Compare March 22, 2024 14:31

vercel bot deployed to Preview – web-tools March 22, 2024 14:35 View deployment

gre force-pushed the live-11772 branch from 5aaeef1 to 44ee940 Compare March 22, 2024 16:08

vercel bot deployed to Preview – web-tools March 22, 2024 16:13 View deployment

gre force-pushed the live-11772 branch from 44ee940 to 9b1c68f Compare March 22, 2024 16:23

vercel bot deployed to Preview – web-tools March 22, 2024 16:27 View deployment

KVNLS reviewed Mar 26, 2024

View reviewed changes

sprohaszka-ledger reviewed Mar 26, 2024

View reviewed changes

libs/ledger-live-common/src/families/near/__snapshots__/bridge.integration.test.ts.snap Outdated Show resolved Hide resolved

...edger-live-common/src/families/cosmos/datasets/__snapshots__/coreum.integration.test.ts.snap Outdated Show resolved Hide resolved

gre marked this pull request as draft March 26, 2024 17:06

gre mentioned this pull request Apr 5, 2024

introduce wallet store, decoupling of account user's data (name, starred,..) from the account's blockchain data #6534

Closed

3 tasks

github-actions bot added the Stale label Apr 14, 2024

github-actions bot closed this Apr 22, 2024

gre reopened this Apr 22, 2024

gre force-pushed the live-11772 branch from 9b1c68f to 3c3345a Compare April 25, 2024 06:59

vercel bot deployed to Preview – web-tools April 25, 2024 07:05 View deployment

gre force-pushed the live-11772 branch from 3c3345a to 1150633 Compare April 25, 2024 08:35

vercel bot deployed to Preview – web-tools April 25, 2024 08:36 View deployment

gre force-pushed the live-11772 branch from 1150633 to 67d69cf Compare April 25, 2024 12:07

vercel bot deployed to Preview – web-tools April 25, 2024 12:18 View deployment

chore: remove unused Account#freshAddresses

bac6ce6

gre force-pushed the live-11772 branch from 67d69cf to bac6ce6 Compare April 25, 2024 14:14

gre marked this pull request as ready for review April 25, 2024 14:14

gre requested a review from KVNLS April 25, 2024 14:17

vercel bot deployed to Preview – web-tools April 25, 2024 14:18 View deployment

KVNLS reviewed Apr 25, 2024

View reviewed changes

KVNLS approved these changes Apr 25, 2024

View reviewed changes

github-actions bot removed the Stale label Apr 26, 2024

sprohaszka-ledger reviewed Apr 26, 2024

View reviewed changes

hedi-edelbloute approved these changes Apr 26, 2024

View reviewed changes

valpinkman approved these changes Apr 26, 2024

View reviewed changes

gre merged commit 8f22da4 into develop Apr 26, 2024
57 of 60 checks passed

gre deleted the live-11772 branch April 26, 2024 10:30

VicAlbr pushed a commit that referenced this pull request May 3, 2024

chore: remove unused Account#freshAddresses field (#6523)

f5313e5

chore: remove unused Account#freshAddresses

gre mentioned this pull request May 3, 2024

chore: decoupled {unit, starred, name} fields away from Account model #6796

Merged

3 tasks

VicAlbr pushed a commit that referenced this pull request May 6, 2024

chore: remove unused Account#freshAddresses field (#6523)

2f41f49

chore: remove unused Account#freshAddresses

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: remove unused Account#freshAddresses field #6523

chore: remove unused Account#freshAddresses field #6523

gre commented Mar 22, 2024

vercel bot commented Mar 22, 2024 •

edited

Loading

KVNLS left a comment

sprohaszka-ledger left a comment •

edited

Loading

gre commented Mar 26, 2024 •

edited

Loading

gre commented Mar 29, 2024 •

edited

Loading

github-actions bot commented Apr 14, 2024

ledger-wiz-cspm-secret-detection bot commented Apr 25, 2024 •

edited

Loading

gre commented Apr 25, 2024

KVNLS left a comment

sprohaszka-ledger left a comment

sprohaszka-ledger Apr 26, 2024

gre Apr 26, 2024 •

edited

Loading

chore: remove unused Account#freshAddresses field #6523

chore: remove unused Account#freshAddresses field #6523

Conversation

gre commented Mar 22, 2024

✅ Checklist

📝 Description

❓ Context

🧐 Checklist for the PR Reviewers

vercel bot commented Mar 22, 2024 • edited Loading

KVNLS left a comment

Choose a reason for hiding this comment

sprohaszka-ledger left a comment • edited Loading

Choose a reason for hiding this comment

gre commented Mar 26, 2024 • edited Loading

gre commented Mar 29, 2024 • edited Loading

github-actions bot commented Apr 14, 2024

ledger-wiz-cspm-secret-detection bot commented Apr 25, 2024 • edited Loading

Wiz Scan Summary

gre commented Apr 25, 2024

KVNLS left a comment

Choose a reason for hiding this comment

sprohaszka-ledger left a comment

Choose a reason for hiding this comment

sprohaszka-ledger Apr 26, 2024

Choose a reason for hiding this comment

gre Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

vercel bot commented Mar 22, 2024 •

edited

Loading

sprohaszka-ledger left a comment •

edited

Loading

gre commented Mar 26, 2024 •

edited

Loading

gre commented Mar 29, 2024 •

edited

Loading

ledger-wiz-cspm-secret-detection bot commented Apr 25, 2024 •

edited

Loading

gre Apr 26, 2024 •

edited

Loading