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

deps: @trezor/connect v9 #133

Merged
merged 4 commits into from
Jan 30, 2023
Merged

deps: @trezor/connect v9 #133

merged 4 commits into from
Jan 30, 2023

Conversation

mroz22
Copy link
Contributor

@mroz22 mroz22 commented Jul 13, 2022

Hi guys, @darkwing @sime

I made this PR #132 work, not sure if it is to your taste.

Regarding your questions @darkwing

The docs say version 9 is experimental -- would this be ready for release?

At the moment we are using this internally in trezor-suite (even in production) and now we would like to roll out @trezor/connect v 9 further onto 3rd party implementations. However no rigid deadline set yet. So right now we are trying to get one or two of them work (in PR) before we proceed

It looks like tests are failing on an initializing issue -- did initialization change in version 9?

It is not allowed to call TrezorConnect.init multiple times but this changes has been around for some time in version 8 as well. Could find which one brought this change exactly.

@mroz22
Copy link
Contributor Author

mroz22 commented Oct 13, 2022

Hello guys, updated this PR once again. We should be using @trezor/connect-web package here since @trezor/connect is intended to be used in node.js environment only.

I have tested this build locally with metamask-extension and it did work. The only problem is that I havent updated your tests here yet because I am not very familiar with mocha. Mocha tests will run happily against @trezor/connect but unfortunately they can't handle browser built-ins (document..) that are accessed by @trezor/connect-web

@mroz22
Copy link
Contributor Author

mroz22 commented Oct 13, 2022

actually, we could pass argument lazyLoad: true which postpones iframe creation until something is called on trezor. and since all the methods that would trigger iframe creation later on are stubbed in your tests, it actually never happens and we don't need to have document object available.
fixup: d064fa1

@danjm
Copy link
Contributor

danjm commented Oct 20, 2022

@mroz22 do we still need #132? I think that can be closed, but is there any work in there that is needed?

@mroz22
Copy link
Contributor Author

mroz22 commented Oct 20, 2022

@mroz22 do we still need #132? I think that can be closed, but is there any work in there that is needed?

it can be closed, everything is here already

@danjm
Copy link
Contributor

danjm commented Oct 20, 2022

I have created a branch on the extension repo that uses this branch to build the eth-trezor-keyring package. We can do QA against that before we merge this PR

MetaMask/metamask-extension#16241

@danjm
Copy link
Contributor

danjm commented Oct 20, 2022

a couple of lint errors

running yarn lint:fix should resolve that

@sime
Copy link
Contributor

sime commented Oct 24, 2022

@danjm Is request or statement? Shall we run yarn lint:fix ?

@mroz22
Copy link
Contributor Author

mroz22 commented Oct 26, 2022

pushed result of yarn lint:fix in 505fceb

@adonesky1
Copy link
Contributor

@mroz22 sorry for letting this get stale. Would you mind rebasing when you have a chance?

@mroz22
Copy link
Contributor Author

mroz22 commented Jan 20, 2023

sure. I also squashed the fixups, but no changes were made besides that.

@adonesky1
Copy link
Contributor

adonesky1 commented Jan 24, 2023

@mroz22 I think there may be an undeclared dependency (tslib) in @trezor/connect-web which is causing some issues here

@mroz22
Copy link
Contributor Author

mroz22 commented Jan 25, 2023

tried that locally, and I hate to say that, but it worked. will try again tomorrow with fresh head and possibly some fresh install of everything

@adonesky1
Copy link
Contributor

tried that locally, and I hate to say that, but it worked. will try again tomorrow with fresh head and possibly some fresh install of everything

Not sure I follow?

@mroz22
Copy link
Contributor Author

mroz22 commented Jan 26, 2023

I tried to get the same error you posted here locally on my machine.

steps roughly:

node --version 
v16.15.1
rm -rf node_modules
yarn
yarn test

and it did not give any error. can't it be something with CI cache possibly?

@adonesky1
Copy link
Contributor

hmmm it fails for me running it locally with the same steps. You must have tslib installed in node_modules for some reason? Can you check if tslib is in your node_modules/ folder?

@mroz22
Copy link
Contributor Author

mroz22 commented Jan 27, 2023

Got it.

  1. indeed I had some stranded node_modules folder in path yarn used to resolve modules
  2. tslib dependency is really missing. I will release it and post update to this PR on monday

@socket-security
Copy link

socket-security bot commented Jan 30, 2023

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

Ignoring: keccak@3.0.3, secp256k1@4.0.3

Powered by socket.dev

@mroz22
Copy link
Contributor Author

mroz22 commented Jan 30, 2023

Hi @adonesky1, I pushed changes (4b94610) which should fix the tslib issue. Could you please approve running the tests workflow? 🙏

@adonesky1
Copy link
Contributor

@SocketSecurity ignore keccak@3.0.3 secp256k1@4.0.3

Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@adonesky1 adonesky1 merged commit 4054e00 into MetaMask:main Jan 30, 2023
@adonesky1
Copy link
Contributor

@mroz22 could you actually bump @metamask/eth-sig-util version to latest (v5.0.2) in @trezor/connect-plugin-ethereum, we're trying to get v4 of @metamask/eth-sig-util out of our dependency graph

@mroz22
Copy link
Contributor Author

mroz22 commented Jan 31, 2023

@adonesky1 thanks, I created an issue for that. it should be resolved soon trezor/trezor-suite#7501

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.

4 participants