Skip to content
This repository has been archived by the owner on Dec 15, 2021. It is now read-only.

upgrade to typescript and refactor protobuf layer #44

Merged
merged 10 commits into from
Nov 24, 2021
Merged

upgrade to typescript and refactor protobuf layer #44

merged 10 commits into from
Nov 24, 2021

Conversation

mroz22
Copy link
Contributor

@mroz22 mroz22 commented Oct 7, 2021

No description provided.

@mroz22 mroz22 force-pushed the new branch 7 times, most recently from 5bab7f2 to 27668ff Compare October 7, 2021 15:20
.eslintrc.js Show resolved Hide resolved
package.json Show resolved Hide resolved
@mroz22
Copy link
Contributor Author

mroz22 commented Oct 12, 2021

check:

  • second tsconfig
  • lib variant import

When working with JSON descriptors (i.e. generated by pbjs) and/or reflection only, see the light library (~16kb gzipped) that excludes the parser. CommonJS entry point is:
var protobuf = require("protobufjs/light");

  • long

If a proper way to work with 64 bit values (uint64, int64 etc.) is required, just install long.js alongside this library. All 64 bit numbers will then be returned as a Long instance instead of a possibly unsafe JavaScript number (see).

This is exactly what we want, we want to be "long" ready. So far there is for compatibility reason this part of code that turns long instance back into number

    if (field.long) {
        return value.toNumber();
    }

it is likely that we can remove this right away because trezor-connect tests don't ever trigger this condition. we should probably make sure that trezor-connect treats following protobuf types as strings: int64, uint64, sint64, fixed64, sfixed64

  • detective story of messages.json versions

@mroz22 mroz22 marked this pull request as ready for review October 12, 2021 18:27
@mroz22 mroz22 force-pushed the new branch 3 times, most recently from 6fb58d2 to e01425b Compare October 13, 2021 07:55
.eslintrc.js Show resolved Hide resolved
.prettierrc Show resolved Hide resolved
src/bridge/v2.ts Show resolved Hide resolved
@marekrjpolak marekrjpolak self-requested a review October 21, 2021 09:10
@mroz22 mroz22 force-pushed the new branch 5 times, most recently from e81ab12 to fc463da Compare November 8, 2021 16:44
@mroz22 mroz22 merged commit 3f4d767 into master Nov 24, 2021
@mroz22 mroz22 deleted the new branch November 24, 2021 15:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants