-
Notifications
You must be signed in to change notification settings - Fork 329
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
Rework channel events #1859
Rework channel events #1859
Conversation
|
||
impl CloseInit { | ||
pub fn attributes(&self) -> Attributes { |
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.
imho this method should take ownership of self
rather than perform a bunch of clones hidden to the caller.
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.
Alternatively, we could implement From<CloseInit> for Attributes
for these events and remove this method.
This would make a nice parallel to the Protobuf
instances we have all over the codebase where we have: SomeDomainType: TryFrom<RawProtobufType>
and RawProtobufType: From<SomeDomainType>
.
What do you think @hu55a1n1?
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 that's a great idea. 👍 I just looked up usages of the attributes()
method and it seems we usually don't use the event after calling this function, so it's good to consume the event.
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.
Okay, I'll do that.
ad4c283
to
14f2bb2
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.
Great work @Wizdave97, thank you :)
Let's merge this for 0.12.1, ie. after #1870 has been merged and released. |
bf2ea37
to
a3c8a33
Compare
* refactor events * fmt * chore: cleanup * clippy fixes * Formatting * update channel event attributes conversion * cargo clippy * minor fix * add changelog Co-authored-by: Romain Ruetschi <romain.ruetschi@gmail.com>
Closes: https://github.com/informalsystems/ibc-rs/issues/718
Description
Refactor events channel events
PR author checklist:
Reviewer checklist: