-
Notifications
You must be signed in to change notification settings - Fork 51
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
build: impl display for fee and contractaddress structs #279
Conversation
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.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware)
a discussion (no related file):
please add a unit test that makes sure the output is nice-looking... i.e.
fn test_nice() {
assert_eq!(format("{}", patricia_key!(7_u8)), "0x7");
}
src/transaction.rs
line 649 at r1 (raw file):
Clone, Default, Display,
why is this needed? it wraps a u128, this should be displayed fine
Code quote:
Display,
e87e047
to
a02c3bb
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.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)
a discussion (no related file):
Previously, dorimedini-starkware wrote…
please add a unit test that makes sure the output is nice-looking... i.e.
fn test_nice() { assert_eq!(format("{}", patricia_key!(7_u8)), "0x7"); }
Done.
src/transaction.rs
line 649 at r1 (raw file):
Previously, dorimedini-starkware wrote…
why is this needed? it wraps a u128, this should be displayed fine
We talked about it later
a02c3bb
to
4c23cae
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.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware)
src/core.rs
line 309 at r2 (raw file):
Ord, derive_more:: Deref, Display,
why did this move down?
Code quote:
Display,
src/core.rs
line 311 at r2 (raw file):
Display, )] #[display(fmt = "{}", "_0.to_fixed_hex_string()")]
this works? cool
Code quote:
#[display(fmt = "{}", "_0.to_fixed_hex_string()")]
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.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware)
src/core.rs
line 309 at r2 (raw file):
Previously, dorimedini-starkware wrote…
why did this move down?
No special reason.
src/core.rs
line 311 at r2 (raw file):
Previously, dorimedini-starkware wrote…
this works? cool
yes, its possible if u use the derive_moe:Display macro
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)
src/core.rs
line 309 at r2 (raw file):
Previously, AvivYossef-starkware wrote…
No special reason.
please move back
4c23cae
to
d8b9146
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.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)
src/core.rs
line 309 at r2 (raw file):
Previously, dorimedini-starkware wrote…
please move back
I hope that I put it back to the same place
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.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)
This change is