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

Add Rust Protobuf #1541

Merged
merged 24 commits into from Jun 8, 2021
Merged

Add Rust Protobuf #1541

merged 24 commits into from Jun 8, 2021

Conversation

jpochyla
Copy link
Contributor

Builds in top of #1540

WIP, needs many fixes in the Python side:

  • p2py build_protobuf
    • separate messages and enums, put enums into trezor.enums, put messages to mocks.
  • python unit tests

Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

I'm not very familiar with protobuf in general and our usage of it so was only able to do shallow review but this is looking very nice and efficient! Some minor nitpicks inline.

common/protob/pb2py Outdated Show resolved Hide resolved
core/embed/rust/src/protobuf/decode.rs Outdated Show resolved Hide resolved
core/src/trezor/wire/__init__.py Outdated Show resolved Hide resolved
core/embed/rust/src/protobuf/decode.rs Outdated Show resolved Hide resolved
core/embed/rust/src/protobuf/decode.rs Show resolved Hide resolved
core/embed/rust/src/protobuf/decode.rs Outdated Show resolved Hide resolved
core/embed/rust/src/protobuf/decode.rs Outdated Show resolved Hide resolved
core/embed/rust/src/protobuf/decode.rs Outdated Show resolved Hide resolved
core/embed/rust/src/protobuf/defs.rs Show resolved Hide resolved
core/embed/rust/src/protobuf/encode.rs Show resolved Hide resolved
core/embed/rust/src/protobuf/encode.rs Show resolved Hide resolved
core/src/apps/binance/helpers.py Show resolved Hide resolved
core/src/apps/common/layout.py Outdated Show resolved Hide resolved
@mmilata
Copy link
Member

mmilata commented May 3, 2021

I think I've mostly fixed the build in CI. Please take a look at my changes @ mmilata/rust-protobuf & feel free to cherry-pick/merge them to this PR.

@matejcik
Copy link
Contributor

matejcik commented Jun 3, 2021

@mmilata any ideas about the core T1 failure? https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/1314953794

@mmilata
Copy link
Member

mmilata commented Jun 3, 2021

@mmilata any ideas about the core T1 failure? https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/jobs/1314953794

Should be fixed after rebasing to master: 8c6b93e

Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

@matejcik great job! The bitcoin-only handling is a massive improvement. Though some enums still make it to the image:

$ tools/check-bitcoin-only core/build/firmware/firmware.bin
Decred details provided but Decred coin not specified.
DecredStakingSpendType
)C#trezor.enums.DecredStakingSpendType
.Q&trezor/enums/DecredStakingSpendType.py
UiConfirmDecredSSTXSubmission
trezor/enums/DecredStakingSpendType.py
NEMImportanceTransferMode
&trezor.enums.NEMImportanceTransferMode
NEMModificationType
 trezor.enums.NEMModificationType
NEMMosaicLevy
trezor.enums.NEMMosaicLevy
NEMSupplyChangeType
 trezor.enums.NEMSupplyChangeType
CardanoAddressParametersType
CardanoBlockchainPointerType
CardanoAddressType
trezor.enums.CardanoAddressType
CardanoCertificateType
#trezor.enums.CardanoCertificateType
CardanoPoolRelayType
!trezor.enums.CardanoPoolRelayType
LiskTransactionType
 trezor.enums.LiskTransactionType
StellarAssetType
MoneroAccountPublicAddress
MoneroMultisigKLRki
MoneroOutputEntry
MoneroRctKeyPublic
!MoneroTransactionDestinationEntry
MoneroTransactionRsigData
MoneroTransactionSourceEntry
TezosBallotType
trezor.enums.TezosBallotType
TezosContractType
trezor.enums.TezosContractType

Still I wish we could merge this soon and perhaps solve this separately as the diff is starting to get unwieldy.

common/protob/pb2py Outdated Show resolved Hide resolved
core/embed/rust/src/protobuf/defs.rs Outdated Show resolved Hide resolved
core/src/apps/bitcoin/sign_tx/decred.py Outdated Show resolved Hide resolved
API-compatibility with the original one is retained.

Now that we don't need to keep code parity with core, we could do some
changes that make life easier.

All generated classes are now in one file. This makes github diffs more
readable, at the cost of somewhat complicating inspecting individual
classes; however, that is something we shouldn't be doing anyway.

Enums are now implemented as enum.IntEnum.

The original class-level FIELDS member was restored.

Each field is now defined via protobuf.Field, which is easier to work
with in the codec, AND we're not stuffing defaults and flags into the
same field.
@matejcik matejcik marked this pull request as ready for review June 7, 2021 11:08
@matejcik matejcik requested a review from onvej-sl as a code owner June 7, 2021 11:08
@matejcik matejcik requested review from mmilata and removed request for prusnak, tsusanka, onvej-sl and andrewkozlik June 7, 2021 11:08
@matejcik
Copy link
Contributor

matejcik commented Jun 7, 2021

rebased, un-drafted, nits fixed
hw pipeline running in https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/pipelines/316065311
ready to merge when CI clears

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.

None yet

3 participants