apollo_propeller: add protobuf conversions for PropellerUnit#11062
Conversation
6ec30ec to
4f4de59
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @noamsp-starkware and @ShahakShama).
crates/apollo_propeller/src/unit.rs line 85 at r1 (raw file):
Previously, ShahakShama wrote…
Vec<u8>
Doesn't that mean that the proto crate will depend on this crate? and this crate will probably also depend on that crate because it need the conversion? Wouldn't that mean that we need to move unit to that crate as well?
crates/apollo_propeller/src/unit.rs line 86 at r1 (raw file):
Previously, ShahakShama wrote…
use ProtobufConversionError
Done.
crates/apollo_propeller/src/unit.rs line 140 at r1 (raw file):
Previously, ShahakShama wrote…
why not impl From/TryFrom like before
Done.
77c60f9 to
563ec59
Compare
4f4de59 to
51131e7
Compare
563ec59 to
e57443a
Compare
51131e7 to
bc4d36a
Compare
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 3 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware and @sirandreww-starkware).
crates/apollo_propeller/src/unit.rs line 85 at r1 (raw file):
Previously, sirandreww-starkware (Andrew Luka) wrote…
Doesn't that mean that the proto crate will depend on this crate? and this crate will probably also depend on that crate because it need the conversion? Wouldn't that mean that we need to move unit to that crate as well?
Move unit to that crate, and then you don't need to have the proto crate depend on this crate
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noamsp-starkware and @ShahakShama).
crates/apollo_propeller/src/unit.rs line 85 at r1 (raw file):
Previously, ShahakShama wrote…
Move unit to that crate, and then you don't need to have the proto crate depend on this crate
I don't want to do that.
It makes the propeller code scattered across 2 crates. Furthermore I don't need Vec<u8> conversion for PropellerUnit.
bc4d36a to
916b05e
Compare
e57443a to
74f9b1b
Compare
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
74f9b1b to
8881b8b
Compare
916b05e to
3979293
Compare
8881b8b to
15aba58
Compare
3979293 to
0c26b67
Compare
Merge activity
|
0c26b67 to
7635c8c
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware reviewed 3 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).

No description provided.