-
-
Notifications
You must be signed in to change notification settings - Fork 677
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
Add Rust Protobuf #1541
Conversation
46bb818
to
5d9843f
Compare
There was a problem hiding this 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.
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. |
@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 |
There was a problem hiding this 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.
This also includes the capability to build Rust protobuf blobs.
- properly exclude in the trezor/enums folder - generate Rust protobuf blobs in scons Split from "tools: Generate special Protobuf blobs for the Rust codec"
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.
rebased, un-drafted, nits fixed |
Builds in top of #1540
WIP, needs many fixes in the Python side:
p2py
build_protobuf
trezor.enums
, put messages tomocks
.