-
Notifications
You must be signed in to change notification settings - Fork 30
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 default genesis
to trait
#3164
Conversation
Depends on EspressoSystems/HotShot#3164
Avoids downstream implementations.
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 didn't see anything wrong, but might be better to wait for others more familiar with this part to approve it.
where | ||
<Self as BlockPayload>::Instance: Default, | ||
{ | ||
Self::from_transactions([], &Default::default()).unwrap() |
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 think a default implementation makes sense, though I'm a little uncomfortable with a default implementation that may panic depending on how a non-default (from_transactions
) is implemented.
It's not a huge deal since I guess this is just a startup panic, but is there any way to avoid this? e.g. are any of our from_transactions
actually fallible?
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.
On quick review it appears that the implementation on the sequencer is used everywhere. The only real failure case I see is from_namespace_offsets. I'm assuming that it isn't a huge deal b/c it is itself being unwrapped. But maybe @ggutoski, @jbearer would have more insight.
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.
My opinion: it's okay because it's just a startup panic, so if there's something wrong then we'll know immediately and there's zero risk of loss. If from_transactions
fails on an empty list then that's pretty bad and should be fixed. 😆
Regarding the current implementation details: please keep in mind that everything in sequencer/src/block
is currently being rewritten, so don't worry to much about what's currently in there.
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.
Approving because this change seems pretty innocuous to me. See my reply below.
Regarding this explainer comment:
https://github.com/EspressoSystems/espresso-sequencer/blob/77484507b7b8a5e94ecbc38dcae356e90d9bbba6/sequencer/src/block.rs#L67-L70
Has this "future update to hotshot" occurred yet? Seems like it's still a "workaround" and we're merely doing the workaround at the trait level instead of in the underlying implementation. Correct? I see no problem with that. (Indeed, it makes sense to me to take care of it at the trait level.) I just want to clarify status.
where | ||
<Self as BlockPayload>::Instance: Default, | ||
{ | ||
Self::from_transactions([], &Default::default()).unwrap() |
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.
My opinion: it's okay because it's just a startup panic, so if there's something wrong then we'll know immediately and there's zero risk of loss. If from_transactions
fails on an empty list then that's pretty bad and should be fixed. 😆
Regarding the current implementation details: please keep in mind that everything in sequencer/src/block
is currently being rewritten, so don't worry to much about what's currently in there.
Sorry I missed this comment. True, some discussion occurred with @jbearer where it was clarified that the appropriate action was to define this default function on the trait. I guess you could say that the work around is just being moved, but I'm not seeing a better solution. With this in HotShot we will be able to remove that comment and TODO on the sequencer side. |
Avoids downstream implementations.
We have a TODO in sequencer here:
https://github.com/EspressoSystems/espresso-sequencer/blob/77484507b7b8a5e94ecbc38dcae356e90d9bbba6/sequencer/src/block.rs#L65
This PR:
Adds a default implantation of
genesis
toBlockPayload
.This PR does not:
It should not change any functionality and only make changes that are required to complete the above.
Key places to review:
The intended change is in
BlockPayload
trait incrates/types/src/traits/block_contents.rs
. That defaultimplementation allows us to remove the implementation from
crates/example-types/src/block_types.rs
.