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

chore(blookbook): rename token type to standard #16286

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

Conversation

enjojoy
Copy link
Contributor

@enjojoy enjojoy commented Jan 9, 2025

Description

Rename token type to standard

Related Issue.

Resolve #16251

Screenshots

Screenshot 2025-01-13 at 12 16 35

Copy link

github-actions bot commented Jan 9, 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

@enjojoy enjojoy force-pushed the refactor/tokentype-standard branch 7 times, most recently from b531183 to 7b6fc20 Compare January 13, 2025 11:17
@enjojoy enjojoy force-pushed the refactor/tokentype-standard branch from 7b6fc20 to 6f68221 Compare January 13, 2025 11:28
@enjojoy enjojoy marked this pull request as ready for review January 13, 2025 11:29
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 the comments, this is not working. It's not enough to change the types, because they're also coming (e.g.) from Blockbook. That's why there's the generated blockbook-api file.

If we really want to restructure the data, we have to explicitly do it after we receive them in the old format (unless backend API is changed as well which I don't presume).

@@ -1,5 +1,7 @@
/* Do not change, this code is generated from Golang structs */
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this file should not be changed manually as it originates from Blockbook repo. Make the necessary changes in blockbook.ts file instead (if they're really necessary, which may not be this case; see the main comment).

@@ -115,7 +115,7 @@ export const transformTokenInfo = (
}

const info = tokens.map(token => ({
type: 'BLOCKFROST',
standard: 'BLOCKFROST',
Copy link
Contributor

Choose a reason for hiding this comment

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

If we wanna make the whole deprecation thing in a non-breaking way (which I don't insist on), all the currently outgoing data must be preserved and some new may be added => we want to signal that type can no longer be relied on, but it's not possible to remove it yet.

@@ -149,7 +149,7 @@ export const transformTokenInfo = (
} = tokenAccount.account.data;

return {
type: tokenProgramsInfo[program].tokenStandard,
standard: tokenProgramsInfo[program].tokenStandard,
Copy link
Contributor

Choose a reason for hiding this comment

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

dtto

@@ -868,7 +868,7 @@ export const migrate: OnUpgradeFunc<SuiteDBSchema> = async (
decimals: token.decimals,
fingerprint: token.name,
policyId,
type: token.type,
standard: token.standard,
Copy link
Contributor

Choose a reason for hiding this comment

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

Never ever change old migrations code, please. No standard had existed at that time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh this one was changed by mistake, thanks for the catch

@@ -327,7 +327,7 @@ export const transformAddresses = (
): AccountAddresses | undefined => {
if (!tokens || !Array.isArray(tokens)) return undefined;
const addresses = tokens.reduce((arr, t) => {
if (t.type !== 'XPUBAddress') return arr;
if (t.standard !== 'XPUBAddress') return arr;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit funny to have token standard XPUBAddress, but it was funny even as token type, so this is no problem. But, as there's no backend data transformation up to this point, field standard is never there so BTC accounts have no addresses and therefore are not shown in Suite (at least in my case 🤷‍♂️)

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 see, well XPUBAddress is there under "type" even in blockbook, alongside with ERC types...

Also for me Bitcoin accounts are displayed but they are empty, so maybe that's why I didn't notice the bug, will investigate

Copy link
Contributor

Choose a reason for hiding this comment

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

XPUBAddress is there under "type" even in blockbook

Yes I know, that's fine. But if they're coming with type, we can't detect them based on standard.

@enjojoy
Copy link
Contributor Author

enjojoy commented Jan 13, 2025

Apart from the comments, this is not working. It's not enough to change the types, because they're also coming (e.g.) from Blockbook. That's why there's the generated blockbook-api file.

If we really want to restructure the data, we have to explicitly do it after we receive them in the old format (unless backend API is changed as well which I don't presume).

Thank you, I feel like I'm going to learn a lot from this PR

@enjojoy
Copy link
Contributor Author

enjojoy commented Jan 13, 2025

I just agreed with @martinboehm that we will add standard on blockbook side and later deprecate type there.

Will proceed with this PR when it will be renamed

cc @marekrjpolak

@enjojoy
Copy link
Contributor Author

enjojoy commented Jan 14, 2025

Waiting for the completion of trezor/blockbook#1185

From blockbook side

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

Successfully merging this pull request may close these issues.

Unify token standard/type
2 participants