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

[DDW-1211] Update @trezor/connect to v9.0.8 #3127

Merged
merged 11 commits into from
Jun 16, 2023

Conversation

tomislavhoracek
Copy link
Contributor

@tomislavhoracek tomislavhoracek commented Jun 13, 2023

This PR introduces @trezor/connect library update to version 9.0.8. In the scope of this PR, we also applied Trezor voting breaking changes.

Todos

  • Update @trezor/connect library to v9.0.8
  • Apply Trezor voting breaking changes

Testing Checklist


Review Checklist

Basics

  • PR assigned to the PR author(s)
  • input-output-hk/daedalus-dev and input-output-hk/daedalus-qa assigned as PR reviewers
  • If there are UI changes, Alexander Rukin assigned as an additional reviewer
  • All visual regression testing has been reviewed (assign run Chromatic label to PR to trigger the run)
  • PR has appropriate labels (release-vNext, feature/bug/chore, WIP)
  • PR link is added to a Jira ticket, ticket moved to In Review
  • PR is updated to the most recent version of the target branch (and there are no conflicts)
  • PR has a good description that summarizes all changes
  • PR contains screenshots (in case of UI changes)
  • CHANGELOG entry has been added to the top of the appropriate section (Features, Fixes, Chores) and is linked to the correct PR on GitHub
  • There are no missing translations (running yarn manage:translations produces no changes)
  • Text changes are proofread and approved (Jane Wild / Amy Reeve)
  • Japanese text changes are proofread and approved (Junko Oda)
  • Storybook works and no stories are broken (yarn storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly commented and documented
  • Code is properly typed with typescript types
  • React components are split-up enough to avoid unnecessary re-renderings
  • Any code that only works in main process is neatly separated from components

Testing

  • New feature/change is covered by acceptance tests
  • New feature/change is manually tested and approved by QA team
  • All existing acceptance tests are still up-to-date
  • New feature/change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review

  • Update Slack QA thread by marking it with a green checkmark

Copy link
Contributor

@marcin-mazurek marcin-mazurek left a comment

Choose a reason for hiding this comment

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

Hey Tomo, well done. Please take a look at one comment around the usage of @trezor/transport :)

Comment on lines 2 to 5
import {
CardanoAddressType,
CardanoCVoteRegistrationFormat,
} from '@trezor/transport/lib/types/messages';
Copy link
Contributor

Choose a reason for hiding this comment

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

Question - could we import from the root of the library instead?

Suggested change
import {
CardanoAddressType,
CardanoCVoteRegistrationFormat,
} from '@trezor/transport/lib/types/messages';
import { Messages } from '@trezor/transport';

Then we could refer to the types as follows:

Messages.CardanoAddressType
Messages.CardanoCVoteRegistrationFormat

If we import from library internals that is not considered the public API, our code can break much easier even with minor/patch updates.

Second question, @trezor/transport is not listed in our package.json. It seems like a dependency of dependency. Should we add it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marcin-mazurek

  1. I totally agree with you. Done in 0d150c9
  2. I think we don't need to list it since it is @trezor/connect subdependency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DominikGuzei what do you think about question / point 2 ? 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

My point was that dependencies of dependencies are not part of public API and could also change without notice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but then the types will be changed in the main / parent dependency since it is used there as well, and we will see that, right? I mean, I don't want to add one more lib if that is not necessary, and if we already have access to it. Also, I agree that is safer to list it and to use it directly. Now we should see what are pros and cons since we are using only types from it...

Copy link
Contributor

Choose a reason for hiding this comment

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

So I just executed yarn add @trezor/transport@^1.1.4 on the branch, which added an appropriate entry to package.json but made no changes to yarn.lock. So I think it doesn't have any impact/side effect besides making it clear in package.json that it is our direct dependency. If I use yarn add @trezor/transport@1.1.4 instead (without ^ - which is our preferred way of adding dependencies to Daedalus), it adds one minor entry to yarn.lock which should not affect anything either.

It's a really minor thing though and I'll let Dominik speak now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oke then, if there is nothing added in node_modules and if it is just linked to an existing submodule, let's list it since that is a safer way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but yeah, let's hear from @DominikGuzei before the final decision :)

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 if we are using it directly in our code (importing) then it's better to list it explicitly 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

source/renderer/app/utils/shelleyTrezor.ts Show resolved Hide resolved
Copy link
Member

@DominikGuzei DominikGuzei left a comment

Choose a reason for hiding this comment

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

Code LGTM 🚢

Copy link

@gabriela-ponce gabriela-ponce left a comment

Choose a reason for hiding this comment

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

Looks good on Windows and macOS. We need an additional QA review (in Linux, if possible)
@input-output-hk/daedalus-qa

@miorsufianiohk miorsufianiohk self-requested a review June 15, 2023 14:36
Copy link

@miorsufianiohk miorsufianiohk left a comment

Choose a reason for hiding this comment

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

LGTM. Tested on Linux. Great job @tomislavhoracek 👍

@marcin-mazurek marcin-mazurek merged commit 92bc587 into develop Jun 16, 2023
3 of 7 checks passed
@michalrus michalrus deleted the chore/ddw-1211-update-trezor-connect-to-v9.0.8 branch June 29, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants