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

Coins deprecation #16131

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

Coins deprecation #16131

wants to merge 5 commits into from

Conversation

MiroslavProchazka
Copy link
Contributor

@MiroslavProchazka MiroslavProchazka commented Jan 2, 2025

As of February 2025, Trezor Suite will discontinue support for Dash, Bitcoin Gold, DigiByte, Namecoin, and Vertcoin. Users are advised to migrate their funds to alternative wallets to ensure continued access and security. The coins will still be supported on the FW level, so users can still access them using third party wallets.

Proposed Timeline:

  • February Suite Release: This version of Suite will no longer support the affected coins. However, users can still use older versions of Suite to access their coins.
  • July 2025: The BEs will be turned off. From this point forward, third-party apps will be the only way to access the coins.

I will divide the commits by environment (desktop/native) to make code review easier.

Please read the following before conducting the code review:

  • Since the coins will still be supported by the firmware and third-party apps, we should maintain support at the Connect level. Similarly, we will continue support in blockchain-link and utxo-lib.
  • For now, we have decided to keep these coins in Trading (users will need to use external addresses for this), so we need to retain the Address Validator as well.
  • We need to create a migration for users with remembered wallets.

Given the points above, I have prepared only basic deprecation that mainly affects NetworksConfig. Everything else (tests, icons, etc.) will remain unchanged until we decide to disable trading options and/or turn off the backends.

Todo:

  • In case that the coins will be still available for trading (to external wallets), can we delete their icons, names in messages.ts and other mentions in general files?
  • Should I also delete all tests mentioning those coins?
  • As some of the coins files needs to stay in the codespace (address validator, blockchain-link, connect etc.), it would be wise to determinate minimal scope for just Suite deprecation - will this be just NetworksConfig?
  • Adjust coins.ts
  • Adjust /trezor-suite/suite-common/suite-constants/src/protocol.ts
  • Create migration for users with remembered wallet - Suite
  • Create migration for users with remembered wallet - Suite lite
  • Create migration for migrateEnabledDiscoveryNetworkSymbols - discovery

This pull request includes significant changes related to the removal of support for several cryptocurrencies across multiple files. The most important changes include updating the LegacyNetworkSymbol type, modifying protocol definitions, and removing network configurations and symbols.

Removal of support for specific cryptocurrencies:

@MiroslavProchazka MiroslavProchazka changed the title chore(suite): basic deprecation on suite level Coins deprecation (WIP) Jan 2, 2025
@MiroslavProchazka MiroslavProchazka mentioned this pull request Jan 2, 2025
6 tasks
Copy link

github-actions bot commented Jan 2, 2025

🚀 Expo preview is ready!

  • Project → trezor-suite-preview
  • Platforms → android, ios
  • Scheme → trezorsuitelite
  • Runtime Version → 21
  • More info

Learn more about 𝝠 Expo Github Action

@@ -78,9 +58,7 @@ export const filterBlacklistedNetworks = (
);

export const portfolioTrackerMainnets = sortNetworks(
Copy link
Contributor

Choose a reason for hiding this comment

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

If I see correctly, if we remove the portfolio blacklist, we can remove portfolioTrackerMainnets and getPortfolioTrackerTestnets completely and use networkSymbolsWhitelistMap.mainnets and networkSymbolsWhitelistMap.testnets instead, wdyt?

@@ -18,12 +18,7 @@ type NetworkSymbol =
| 'etc'
| 'xrp'
| 'bch'
| 'btg'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep those in this migration

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. maybe there should be migration removing target deprecated coins accounts from state as well. but its not that big deal. it can stay there forever and nothing will probably ever happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will prepare migration for users with remembered wallet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@MiroslavProchazka MiroslavProchazka requested a review from a team January 3, 2025 13:58
@MiroslavProchazka MiroslavProchazka force-pushed the chore/coins-deprecation branch 6 times, most recently from 50973b7 to 0405953 Compare January 9, 2025 15:41
@MiroslavProchazka MiroslavProchazka marked this pull request as ready for review January 9, 2025 15:42
@MiroslavProchazka MiroslavProchazka force-pushed the chore/coins-deprecation branch 4 times, most recently from 22960bc to bd9da1f Compare January 9, 2025 16:01
@MiroslavProchazka MiroslavProchazka changed the title Coins deprecation (WIP) Coins deprecation Jan 9, 2025
Copy link
Contributor

@marekrjpolak marekrjpolak left a comment

Choose a reason for hiding this comment

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

Apart from mentioned, it looks good (= same as e.g. tgor deprecation).

| 'dogecoin'
| 'namecoin'
| 'vertcoin'
Copy link
Contributor

Choose a reason for hiding this comment

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

btg | dgb | nmc | vtc below should also be removed, right?

@@ -1176,4 +1178,56 @@ export const migrate: OnUpgradeFunc<SuiteDBSchema> = async (
return account;
});
}

// Deprecate Vertcoin (VTC) and other networks
if (oldVersion < 52) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot about this:

const VERSION = 51; // don't forget to add migration and CHANGELOG when changing versions!

@MiroslavProchazka MiroslavProchazka self-assigned this Jan 14, 2025
Copy link
Member

@tomasklim tomasklim left a comment

Choose a reason for hiding this comment

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

You forgot to remove icons from suite-common/icons, suite-common/icons-deprecated packages

const deprecatedNetworks = ['vtc', 'btg', 'nmc', 'dgb', 'dash'];

// Remove transactions related to deprecated networks
await updateAll(transaction, 'txs', tx => {
Copy link
Member

Choose a reason for hiding this comment

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

I think you miss graph, historicRates and sendFormDrafts

Copy link
Contributor

Choose a reason for hiding this comment

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

This is true, but historically we didn't handle these when removing coins, only when changing symbols (because we wanted to keep the data), so I believe this should be safe as well.

'vtc',
'zec',
],
testnet: ['test', 'regtest', 'tsep', 'thol', 'dsol', 'tada', 'txrp'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add "dsol" here

'zec',
],
testnet: ['test', 'regtest', 'tsep', 'thol', 'dsol', 'tada', 'txrp'],
mainnet: ['btc', 'eth', 'pol', 'bsc', 'ltc', 'etc', 'ada', 'xrp', 'bch', 'doge', 'zec'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add "sol" here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔎 Needs review
Development

Successfully merging this pull request may close these issues.

6 participants