Skip to content
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

Modules: return owned value with ConnectionReader #102

Closed
5 tasks done
CharlyCst opened this issue Oct 29, 2020 · 1 comment
Closed
5 tasks done

Modules: return owned value with ConnectionReader #102

CharlyCst opened this issue Oct 29, 2020 · 1 comment
Assignees

Comments

@CharlyCst
Copy link

CharlyCst commented Oct 29, 2020

Crate

ibc

Summary

Return Option<ConnectionEnd> instead of Option<&ConnectionEnd> in ConnectionReader::connection_end.

Problem Definition

There are two motivations for this change:

  1. The ConnectionReader trait's methods mostly return owned values (client_state returns a Option<AnyClientState>, host_consensus_state returns an owned value too), the only exception being connection_end which returns an Option<&ConnectionEnd>. For the sake of consistency, all methods should return either owned or borrowed values.
    The ClientReader trait's methods return owned values, too.
  2. More importantly, as I expect that most stores will use a raw byte storage format (tell me if I'm wrong) and thus need to de-serialize and allocate a new struct, it makes more sense to return objects rather than references. Due to the ownership system, returning data owned by the connection_end function can probably be worked around, but it doesn't seems to be ideal.

Proposal

You can see the required changes here.

Note that is is a breaking change.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@adizere adizere self-assigned this Oct 29, 2020
@romac
Copy link
Member

romac commented Oct 29, 2020

Agreed, on both points! 👍

ancazamfir referenced this issue in informalsystems/hermes Nov 2, 2020
* ConnectionReader.connection_end returns owned val

* Removed redundant clones, updated changelog; ready to fix #347.

Co-authored-by: CharlyCst <castes.ch@gmail.com>
hu55a1n1 referenced this issue in hu55a1n1/hermes Sep 13, 2022
* ConnectionReader.connection_end returns owned val

* Removed redundant clones, updated changelog; ready to fix #347.

Co-authored-by: CharlyCst <castes.ch@gmail.com>
@hu55a1n1 hu55a1n1 transferred this issue from informalsystems/hermes Sep 29, 2022
shuoer86 pushed a commit to shuoer86/ibc-rs that referenced this issue Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants