apollo_propeller: add base types (Channel, ShardIndex, MessageRoot)#11048
Conversation
c8d84e7 to
d14f0e1
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 5 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @ShahakShama).
crates/apollo_propeller/src/types.rs line 7 at r1 (raw file):
Previously, ShahakShama wrote…
Remove these
I want to keep them, the file gets larger and I use these to separate different sections I can add a TODO to remove these later but I think they should remain if we don't want to split this file up
crates/apollo_propeller/src/types.rs line 10 at r1 (raw file):
Previously, ShahakShama wrote…
Update according to what we decided in the proto PR
As we said there, once we finish the PR stack we will change this all across the codebase
crates/apollo_propeller/src/types.rs line 12 at r1 (raw file):
Previously, ShahakShama wrote…
What's wrong with the regular display?
What is a regular display? you mean the one you get with Debug?
https://doc.rust-lang.org/rust-by-example/hello/print/print_display.html
I just wanted the ability to print normally. And for the print output to be a bit nicer, this is not that important for Channel and Shard Index but is important for MessageRoot.
What do you think we should do?
crates/apollo_propeller/src/types.rs line 23 at r1 (raw file):
Previously, ShahakShama wrote…
Same here
^
crates/apollo_propeller/src/types.rs line 34 at r1 (raw file):
Previously, ShahakShama wrote…
Same here
^
d14f0e1 to
b483341
Compare
685dbb1 to
2193ed5
Compare
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @noamsp-starkware and @ShahakShama).
crates/apollo_propeller/src/types.rs line 12 at r1 (raw file):
Previously, sirandreww-starkware (Andrew Luka) wrote…
What is a regular display? you mean the one you get with Debug?
https://doc.rust-lang.org/rust-by-example/hello/print/print_display.html
I just wanted the ability to print normally. And for the print output to be a bit nicer, this is not that important for Channel and Shard Index but is important for MessageRoot.
What do you think we should do?
From what I've seen in other places, we only implement a Display function if we really need it to look prettier (e.g., the debug dump is too cluttered). IMO if you don't need this, better just derive(Debug) and use that (less code to read is always welcome).
On the other hand, if you want this to work as a standalone crate, maybe there's motivation to be "complete" about user-facing functionality. I don't know. Whatever makes more sense to you.
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama made 2 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @sirandreww-starkware).
crates/apollo_propeller/src/types.rs line 7 at r1 (raw file):
Previously, sirandreww-starkware (Andrew Luka) wrote…
I want to keep them, the file gets larger and I use these to separate different sections I can add a TODO to remove these later but I think they should remain if we don't want to split this file up
If the file is too large then the correct thing to do is to split it
crates/apollo_propeller/src/types.rs line 12 at r1 (raw file):
Previously, guy-starkware wrote…
From what I've seen in other places, we only implement a Display function if we really need it to look prettier (e.g., the debug dump is too cluttered). IMO if you don't need this, better just derive(Debug) and use that (less code to read is always welcome).
On the other hand, if you want this to work as a standalone crate, maybe there's motivation to be "complete" about user-facing functionality. I don't know. Whatever makes more sense to you.
I agree with @guy-starkware 's less paragraph. Let's just delete this code
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama made 1 comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @sirandreww-starkware).
crates/apollo_propeller/src/types.rs line 10 at r2 (raw file):
#[derive(Debug, Default, PartialEq, Clone, Copy, Ord, PartialOrd, Eq, Hash)] pub struct Channel(pub u32);
Rename to ChannelId
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama made 1 comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @sirandreww-starkware).
crates/apollo_propeller/src/types.rs line 12 at r1 (raw file):
Previously, ShahakShama wrote…
I agree with @guy-starkware 's less paragraph. Let's just delete this code
first*
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama made 2 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @sirandreww-starkware).
crates/apollo_propeller/src/types.rs line 23 at r1 (raw file):
Previously, sirandreww-starkware (Andrew Luka) wrote…
^
^
crates/apollo_propeller/src/types.rs line 34 at r1 (raw file):
Previously, sirandreww-starkware (Andrew Luka) wrote…
^
^
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @sirandreww-starkware).
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 5 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @noamsp-starkware and @ShahakShama).
crates/apollo_propeller/src/types.rs line 7 at r1 (raw file):
Previously, ShahakShama wrote…
If the file is too large then the correct thing to do is to split it
correct
crates/apollo_propeller/src/types.rs line 12 at r1 (raw file):
Previously, ShahakShama wrote…
first*
Done.
crates/apollo_propeller/src/types.rs line 23 at r1 (raw file):
Previously, ShahakShama wrote…
^
Done.
crates/apollo_propeller/src/types.rs line 34 at r1 (raw file):
Previously, ShahakShama wrote…
^
Done.
crates/apollo_propeller/src/types.rs line 10 at r2 (raw file):
Previously, ShahakShama wrote…
Rename to ChannelId
I will put a TODO for now because I want to do this all over the code when it's all in. Do you accept?
2193ed5 to
7a08a3c
Compare
b483341 to
094b21c
Compare
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 2 files and all commit messages, and resolved 5 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware).
7a08a3c to
8bed384
Compare
094b21c to
4411fcc
Compare
4411fcc to
dab4f4b
Compare
8bed384 to
871b8eb
Compare
Merge activity
|
208ae26

No description provided.