ADR14 M1 update #627

chcmedeiros · 2023-01-13T16:00:49Z

Support Milestone 1 of ADR14 implementation:

Rename old methods;
Add new methods.

ftheirs · 2023-01-13T16:56:29Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "@oasisprotocol/ledger",
-  "version": "0.2.3",
+  "version": "0.2.4",


To keep consistency with the new methods, getAddressAndPubKey was renamed to getAddressAndPubKey_ed25519 (new method is called getAddressAndPubKey_secp256k1)
To respect SemVer, major field should be bumped. However, if you don't want to make breaking changes in the API, it's possible to keep the old names.
I think that this should be decided by Oasis team 😄

Good point! I will bump the major so that we have a version that respects SemVer, since the changes in this PR ask for that. Then Oasis Team can decide to merge this as is or we change the old method names.

src/helperV1.js

matevz · 2023-01-16T13:47:03Z

I'm getting bunch of linter errors when running yarn serve. Can you check it out?

matevz · 2023-01-16T13:56:30Z

I'm getting bunch of linter errors when running yarn serve. Can you check it out?

Nevermind, it seems I had some leftovers from the old repo.

matevz

please add proper secp256k1 tests (the pubkey and address should match the ones from Ethereum when using testing mnemonics)
please add the secp256k1 address and ParaTime transaction signing also to the UI (e.g. by adding 4 new buttons)
Please fix the integration tests. I'm having issues running yarn test:integration. This https://github.com/chcmedeiros/ledger-js/blob/zondax-M1-update/tests/basic.ispec.js#L59 does not seem to be a vaild native Oasis address (it's too long). Should integration tests be working?

package.json

chcmedeiros · 2023-01-17T15:22:28Z

@matevz I have fixed the old tests that were not correct and added tests coverage for the new methods. Also tested it using a NanoS with the test mnemonic).
Regarding the UI (.vue) that example has some time, and we have deprecated it months ago.

matevz · 2023-01-19T09:38:29Z

tests/basic.ispec.js


-  expect(resp.return_code).toEqual(0x9000);
-  expect(resp.error_message).toEqual("No errors");
+    expect(resp.hex_address).toEqual("7dac3ca6b185b7778a7027a52c0cc3940d2e1ca0");


The standard BIP44 address for testing mnemonic equip will roof matter pink blind book anxiety banner elbow sun young is 95e5e3C1BDD92cd4A0c14c62480DB5867946281D. Where does discrepancy come from?

matevz · 2023-01-20T17:09:38Z

tests/basic.ispec.js

-  const transport = await TransportNodeHid.create(1000);
-  const app = new OasisApp(transport);
+    // Derivation path. First 3 items are automatically hardened!
+    const path = [44, 60, 0];


Please also cover the case for the 2nd key [44', 60', 0', 0, 1]. The helper function in helperV1.js will probably need to be updated or a new one added, but please don't break backward compatibility.

matevz · 2023-01-23T09:54:51Z

I'm getting compilation error after applying ac681ad. Can you take a look?

$ yarn build
yarn run v1.22.5
warning ../package.json: No license field
$ babel src --out-dir dist && yarn build-types
Successfully compiled 3 files with Babel (610ms).
warning ../package.json: No license field
$ tsc -p ./tsconfig.json
src/types.ts:39:30 - error TS2345: Argument of type 'Buffer | { return_code: number; error_message: string; } | { pk: Buffer; hex_address: string; return_code: any; error_message: string; }' is not assignable to parameter of type 'Response<Buffer | { pk: Buffer; hex_address: string; return_code: any; error_message: string; }>'.
  Type 'Buffer' is not assignable to type 'Response<Buffer | { pk: Buffer; hex_address: string; return_code: any; error_message: string; }>'.
    Type 'Buffer' is not assignable to type 'Buffer & { return_code: number; error_message: string; }'.
      Type 'Buffer' is missing the following properties from type '{ return_code: number; error_message: string; }': return_code, error_message

39   console.log(successOrThrow(await app.getAddressAndPubKey_secp256k1([44])).hex_address.trim());
                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/types.ts:39:77 - error TS2339: Property 'hex_address' does not exist on type 'Buffer | { pk: Buffer; hex_address: string; return_code: any; error_message: string; }'.
  Property 'hex_address' does not exist on type 'Buffer'.

39   console.log(successOrThrow(await app.getAddressAndPubKey_secp256k1([44])).hex_address.trim());
                                                                               ~~~~~~~~~~~

src/types.ts:40:30 - error TS2345: Argument of type 'Buffer | { return_code: number; error_message: string; } | { pk: Buffer; hex_address: string; return_code: any; error_message: string; }' is not assignable to parameter of type 'Response<Buffer | { pk: Buffer; hex_address: string; return_code: any; error_message: string; }>'.

40   console.log(successOrThrow(await app.showAddressAndPubKey_secp256k1([44])).hex_address.trim());
                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/types.ts:40:78 - error TS2339: Property 'hex_address' does not exist on type 'Buffer | { pk: Buffer; hex_address: string; return_code: any; error_message: string; }'.
  Property 'hex_address' does not exist on type 'Buffer'.

40   console.log(successOrThrow(await app.showAddressAndPubKey_secp256k1([44])).hex_address.trim());
                                                                                ~~~~~~~~~~~


Found 4 errors in the same file, starting at: src/types.ts:39

matevz

Great work, thanks.

pro-wh · 2023-04-17T22:45:56Z

@matevz are we waiting for anything before merging this?

kostko · 2023-04-18T06:38:11Z

Looks like the tests did not run for some reason.

matevz

I'm getting an error when running yarn build-example. @chcmedeiros Can you take a look and then merge this so we can add ParaTime support for Ledger to our javascript wallets as well.

matevz · 2023-04-18T08:48:35Z

src/index.js

+  const returnCode = errorCodeData[0] * 256 + errorCodeData[1];
+
+  const pk = Buffer.from(response.slice(0, 33));
+  const hex_address = Buffer.from(response.slice(33, 73)).toString();


Suggested change

const hex_address = Buffer.from(response.slice(33, 73)).toString();

const hexAddress = Buffer.from(response.slice(33, 73)).toString();

matevz · 2023-04-18T08:48:46Z

src/index.js

+
+  return {
+    pk,
+    hex_address,


Suggested change

hex_address,

hex_address: hexAddress,

yes, I will take a look

chcmedeiros · 2023-04-26T08:01:54Z

I'm getting an error when running yarn build-example. @chcmedeiros Can you take a look and then merge this so we can add ParaTime support for Ledger to our javascript wallets as well.

@matevz Regarding the vue-example that example has some time, and we have deprecated it months ago. If it's ok with you and since we have test coverage for the package with the real device we would remove that vue-example folder, avoiding more errors.

pro-wh · 2023-04-26T17:01:53Z

sure, sounds good. would reduce the dev dependency tree by a lot too

chcmedeiros · 2023-04-27T08:27:18Z

Vue example was removed. @matevz @pro-wh If you want to take a last look at it

matevz

@chcmedeiros yarn test:integration freezes on the second test for me using hardware ledger with the latest firmware you sent to me (UI fixes). Can you check it out? Also, what would be the easiest way to do Ledger tests inside CI?

chcmedeiros · 2023-05-02T16:58:17Z

Hey @matevz The M3-maintenance branch now has a version that is working here on the ledger-js, if you want to test. Regarding the ledger tests, I think the procedure would be to compile the app from the repository to get the .elf and then use our tool zemu (device emulator) combined with this repo to run the tests. This should require some more work, so maybe we can merge this, discuss these tests implementation and possibly do it in the future PR for SR25519.

matevz · 2023-05-04T14:19:49Z

Hey @matevz The M3-maintenance branch now has a version that is working here on the ledger-js, if you want to test. Regarding the ledger tests, I think the procedure would be to compile the app from the repository to get the .elf and then use our tool zemu (device emulator) combined with this repo to run the tests. This should require some more work, so maybe we can merge this, discuss these tests implementation and possibly do it in the future PR for SR25519.

I can confirm that integration tests now pass with Zondax/ledger-oasis@9b9c85a . Feel free to merge this PR.

chcmedeiros · 2023-05-04T14:26:05Z

@matevz I am not able to merge, probably don't haver permission to do so.

ftheirs reviewed Jan 13, 2023

View reviewed changes

pro-wh reviewed Jan 13, 2023

View reviewed changes

src/helperV1.js Outdated Show resolved Hide resolved

chcmedeiros requested a review from pro-wh January 16, 2023 08:44

matevz requested changes Jan 16, 2023

View reviewed changes

package.json Show resolved Hide resolved

chcmedeiros added 3 commits January 17, 2023 15:11

add new ADR14 methods

f21cd00

fix old tests and add coverage for new methods

0ea6805

update dependencies & version

fb41011

chcmedeiros force-pushed the zondax-M1-update branch from 0152d30 to fb41011 Compare January 17, 2023 15:17

chcmedeiros requested review from matevz and ftheirs and removed request for pro-wh, matevz and ftheirs January 17, 2023 15:22

matevz requested changes Jan 19, 2023

View reviewed changes

chcmedeiros requested a review from matevz January 19, 2023 17:25

fix secp256k1 address results

e72517b

matevz requested changes Jan 20, 2023

View reviewed changes

adding serializePathBip44 to keep backward compatibility

ac681ad

fix compilation error

fc4a4b0

chcmedeiros requested a review from matevz January 23, 2023 14:15

replace ParaTime with Runtime terms

5816982

matevz approved these changes Jan 23, 2023

View reviewed changes

chcmedeiros force-pushed the zondax-M1-update branch from de0978b to f358afa Compare February 6, 2023 14:14

New metadata + tx signing

f358afa

matevz requested changes Apr 18, 2023

View reviewed changes

small fix

3ec4ed6

remove vue example and dependencies

75aa321

update ci workflow

b552445

matevz requested changes Apr 27, 2023

View reviewed changes

matevz approved these changes May 4, 2023

View reviewed changes

matevz merged commit 9829c54 into oasisprotocol:master May 4, 2023
1 check passed

lukaw3d mentioned this pull request Aug 7, 2023

commit yarn.lock changes #637

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADR14 M1 update #627

ADR14 M1 update #627

chcmedeiros commented Jan 13, 2023

ftheirs Jan 13, 2023

chcmedeiros Jan 13, 2023

matevz commented Jan 16, 2023

matevz commented Jan 16, 2023

matevz left a comment •

edited

chcmedeiros commented Jan 17, 2023

matevz Jan 19, 2023

matevz Jan 20, 2023

matevz commented Jan 23, 2023

matevz left a comment

pro-wh commented Apr 17, 2023

kostko commented Apr 18, 2023

matevz left a comment

matevz Apr 18, 2023

matevz Apr 18, 2023

chcmedeiros Apr 18, 2023

chcmedeiros commented Apr 26, 2023

pro-wh commented Apr 26, 2023

chcmedeiros commented Apr 27, 2023

matevz left a comment

chcmedeiros commented May 2, 2023 •

edited

matevz commented May 4, 2023 •

edited

chcmedeiros commented May 4, 2023

	const hex_address = Buffer.from(response.slice(33, 73)).toString();
	const hexAddress = Buffer.from(response.slice(33, 73)).toString();

ADR14 M1 update #627

ADR14 M1 update #627

Conversation

chcmedeiros commented Jan 13, 2023

ftheirs Jan 13, 2023

Choose a reason for hiding this comment

chcmedeiros Jan 13, 2023

Choose a reason for hiding this comment

matevz commented Jan 16, 2023

matevz commented Jan 16, 2023

matevz left a comment • edited

Choose a reason for hiding this comment

chcmedeiros commented Jan 17, 2023

matevz Jan 19, 2023

Choose a reason for hiding this comment

matevz Jan 20, 2023

Choose a reason for hiding this comment

matevz commented Jan 23, 2023

matevz left a comment

Choose a reason for hiding this comment

pro-wh commented Apr 17, 2023

kostko commented Apr 18, 2023

matevz left a comment

Choose a reason for hiding this comment

matevz Apr 18, 2023

Choose a reason for hiding this comment

matevz Apr 18, 2023

Choose a reason for hiding this comment

chcmedeiros Apr 18, 2023

Choose a reason for hiding this comment

chcmedeiros commented Apr 26, 2023

pro-wh commented Apr 26, 2023

chcmedeiros commented Apr 27, 2023

matevz left a comment

Choose a reason for hiding this comment

chcmedeiros commented May 2, 2023 • edited

matevz commented May 4, 2023 • edited

chcmedeiros commented May 4, 2023

matevz left a comment •

edited

chcmedeiros commented May 2, 2023 •

edited

matevz commented May 4, 2023 •

edited