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

feat: cardano preview testnet #7262

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

vladimirvolek
Copy link
Contributor

Adding new Cardano Preview Testnet network.
Legacy testnet will be terminated.

image

image

Copy link
Contributor

@mroz22 mroz22 left a comment

Choose a reason for hiding this comment

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

If I remember correctly packages/connect-common/files/coins.json should be autogenerated from connect-common. So this PR should be accompanied by a corresponding update of submodules

There is some read on this, hope it is up to date
https://github.com/trezor/trezor-suite/blob/develop/docs/packages/connect/supported-coins.md#the-pipeline

@vladimirvolek
Copy link
Contributor Author

If I remember correctly packages/connect-common/files/coins.json should be autogenerated from connect-common. So this PR should be accompanied by a corresponding update of submodules

There is some read on this, hope it is up to date https://github.com/trezor/trezor-suite/blob/develop/docs/packages/connect/supported-coins.md#the-pipeline

trezor/trezor-common#336

@vladimirvolek
Copy link
Contributor Author

@mroz22 please?

@mroz22
Copy link
Contributor

mroz22 commented Jan 6, 2023

waiting for trezor/trezor-firmware#2716 and submodule update

@mroz22 mroz22 added the cardano Related to Cardano (ADA) blockchain platform label Jan 7, 2023
@vladimirvolek
Copy link
Contributor Author

@mroz22 done

@@ -2253,25 +2294,6 @@
},
"url": "https://www.rinkeby.io"
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we did not update trezor-common with goerli.

could you please, @karliatto have a look? also we were about to get rid of tGOR etc. and replace it with tETH instead. can we do it now, or are we still waiting for blockbooks?

Copy link
Member

Choose a reason for hiding this comment

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

Can we coordinate this for the March release? It's the next release of FW.

Copy link
Member

Choose a reason for hiding this comment

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

@mroz22

But anyway after some investigations it looks to me that removing of tGOR has already started in trezor-common and that might be a reason why this is removed from here now by the automatic script. On the other side it looks like the version of submodules/trezor-common used in this PR is a bit outdated.

So tGOR for goerli was part of trezor-common as you can see here trezor/trezor-common@be35125#diff-478bd47ba2f3d059686166afd3b3630d9d2231e2550454d3e5f1eb64f7f788d0 but it is not anymore.

This is the commit that did the changes trezor/trezor-common@a551917#diff-52ccdfbdbb8d7df04c3575c60ece020902c2a4e2f171ce27d826f526ca688c76R39391

But it tGOR is still there in https://github.com/trezor/trezor-common/blob/master/defs/coins_details.json

It looks there are some changes happening in trezor-common, maybe @mmilata can give us more details. And maybe we need to update the update-coins.sh script to new version.

Choose a reason for hiding this comment

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

But it tGOR is still there in https://github.com/trezor/trezor-common/blob/master/defs/coins_details.json

don't look at that file .)

I was under the impression that we decided to keep tGOR and friends (bc it makes it easier to see on Trezor on which testnet you are), no?

current master: https://github.com/trezor/trezor-common/blob/master/defs/duplicity_overrides.json#L18-L19

Copy link
Member

Choose a reason for hiding this comment

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

Intention is to sync with upstream and avoid special handling.

Copy link
Contributor

@mroz22 mroz22 left a comment

Choose a reason for hiding this comment

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

ok thanks,
please keep tGOR record in coins.json - it would break this coin in suite. it looks like we forgot to edit firmware-common when doing this change.
also I would appreciate not having linting changes unless they are enforced by CI.

@mroz22 mroz22 self-assigned this Jan 20, 2023
@mroz22
Copy link
Contributor

mroz22 commented Jan 20, 2023

just please run yarn format and push the changes are it is ready for merge

@vladimirvolek
Copy link
Contributor Author

done

fix: update coins.json

fix: typo

fix: allow new testnet in desktop

fix: bl link

fix: message

fix: cardano ttl

chore: update trezor-common (b6503f5)

chore: update coins.js

fix: lint

fix: eth testnet

fix: tests
@mroz22 mroz22 merged commit 4c8e4dc into trezor:develop Jan 20, 2023
@tsusanka tsusanka added the review label Mar 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cardano Related to Cardano (ADA) blockchain platform Roadmap: Off
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants