Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

[hardware-wallet] use prost! for protobuf handling #10674

Closed
wants to merge 3 commits into from

Conversation

ordian
Copy link
Collaborator

@ordian ordian commented May 17, 2019

Makes cargo audit happy. Haven't actually tested on hardware wallet, but it seems to compile.
https://github.com/paritytech/trezor-sys/compare/prost

@ordian ordian added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M5-dependencies 🖇 Dependencies. labels May 17, 2019
@ordian ordian added this to the 2.6 milestone May 17, 2019
@ordian ordian requested a review from niklasad1 May 17, 2019 17:31
@niklasad1
Copy link
Collaborator

cool, unfortunately I don't have access to a Ledger. Try to ping somebody in Berlin office to test it maybe @soc1c can? :)

Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

lgtm, but testing this on a Trezor seems important!

accounts/hw/src/trezor.rs Outdated Show resolved Hide resolved
accounts/hw/src/trezor.rs Outdated Show resolved Hide resolved
@dvdplm
Copy link
Collaborator

dvdplm commented May 19, 2019

There's a new protobuf version out that might help here: stepancheg/rust-protobuf#411

EDIT: would require upgrading to 2.6 and we're at 1.7.4 atm, so not sure which is more work tbh. And the problem of not knowing how to test it is still there.

@dvdplm
Copy link
Collaborator

dvdplm commented May 20, 2019

Fix back ported to 1.7.5 now. Maybe that's a better route?

@ordian ordian closed this May 20, 2019
@ordian ordian deleted the ao-fix-cargo-audit branch May 20, 2019 07:19
@@ -118,6 +118,12 @@ enum HidVersion {
V2,
}

fn encode_message<M: Message>(message: M) -> Vec<u8> {
let mut result = Vec::with_capacity(message.encoded_len());
message.encode(&mut result).expect("Vec has unlimited capacity; qed");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick, I don't agree with unlimited capacity because Vec::alloc will panic when capacity is bigger than isize::max_value on 32-bit platforms and usize::max_value on 64-bit platforms.

However, I think that is only theoretically and would be fine with some like message could be represented as Vec there encodable; qed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. M5-dependencies 🖇 Dependencies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants