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.

Sign up for GitHub

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

Jump to bottom

modules: Change all *Reader traits to return Result instead of Option #1283

Merged
merged 10 commits into from
Sep 10, 2021
Merged

modules: Change all *Reader traits to return Result instead of Option #1283

merged 10 commits into from
Sep 10, 2021

Conversation

yito88
Copy link
Contributor

@yito88 yito88 commented Aug 12, 2021 •
edited by romac
Loading

Closes: cosmos/ibc-rs#73

Description

ClientReader returns Result instead of Option so that we can handle errors

For contributor use:

  • Added a changelog entry, using unclog.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@romac romac added the modules label Aug 17, 2021
@hu55a1n1
Copy link
Member

Thanks for the PR @yito88! 🎉
I just added some comments. Also, since we're moving towards no_std compliance, we should try to avoid std code in the modules. See #1156.

@romac romac changed the title ClientReader returns Result Change ClientReader trait to return Results Aug 24, 2021
@hu55a1n1
Copy link
Member

Hi @yito88, LGTM! 👌 I think we all agree that it would be useful to have methods of all *Reader sub-traits of Ics26Context return Results instead of Options. Would you be willing to contribute to that effort? If so, would you like to have a different PR for that or would you like to do that work in this PR itself?

@yito88
Copy link
Contributor Author

yito88 commented Aug 25, 2021

@hu55a1n1 Thank you for reviewing!
Yes, I'll do that. I'd like to continue in this PR because, though the changes are not few, the return type of readers should be consistent and it would be straightforward.

@romac
Copy link
Member

romac commented Aug 25, 2021

That's great to hear, thanks @yito88!

@romac romac changed the title Change ClientReader trait to return Results modules: Change all *Reader traits to return Result instead of Option Aug 26, 2021
@yito88 @romac
Update .changelog/unreleased/improvements/ibc/1268-reader-result.md
b4ea5d2 
Co-authored-by: Romain Ruetschi <romain.ruetschi@gmail.com>
hu55a1n1
hu55a1n1 reviewed Sep 3, 2021
View reviewed changes
Copy link
Member

@hu55a1n1 hu55a1n1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @yito88! 🙏 The error handling code looks much cleaner now with more use of the question mark operator (?). 👌
I have added some comments mostly related to removing redundant error variants. I think there is an opportunity for more cleanup if we add an Ics03Connection variant to Ics04Error, because that will allow us to reuse errors from Ics03Error (and by extension Ics02Error using the Ics03Error::Ics02Client variant) and avoid redundant errors such as Ics04Error::ClientNotFound, MissingClientState, etc. I think there is already a lot of work done in this PR, so I would suggest we open a different issue/PR for that.

modules/src/application/ics20_fungible_token_transfer/relay_application_logic/send_transfer.rs Show resolved Hide resolved
modules/src/ics03_connection/handler/conn_open_ack.rs Show resolved Hide resolved
modules/src/ics03_connection/handler/conn_open_init.rs Show resolved Hide resolved
modules/src/ics03_connection/version.rs Show resolved Hide resolved
modules/src/ics03_connection/version.rs Show resolved Hide resolved
modules/src/ics03_connection/handler/conn_open_ack.rs Show resolved Hide resolved
@yito88
Copy link
Contributor Author

yito88 commented Sep 7, 2021 •
edited
Loading

@hu55a1n1 Thank you for your comments! That makes sense 👍
I tried to clean up these errors at https://github.com/yito88/ibc-rs/tree/yuji/cleanup_errors based on this branch.

--
I opened an issue cosmos/ibc-rs#72 and a PR #1334 for cleaning up the errors

This was referenced Sep 29, 2022
Closed
Merged
@hu55a1n1
Copy link
Member

hu55a1n1 commented Sep 8, 2021

Thanks again @yito88! I have resolved all my comments that you've addressed in #1334. I'll let @romac and @adizere weigh in on my other suggestion regarding HashSet for Version::features.

adizere
adizere approved these changes Sep 9, 2021
View reviewed changes
Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying @cezarad if she wants to have a look here, since she modified these files significantly as well.

I only had a minor question. This looks ready LGTM! Many thanks @yito88 !

modules/src/ics03_connection/error.rs Show resolved Hide resolved
modules/src/ics03_connection/version.rs Show resolved Hide resolved
@adizere adizere merged commit 22420af into informalsystems:master Sep 10, 2021
@yito88 yito88 deleted the yuji/client_reader_result branch September 10, 2021 07:08
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
@yito88 @romac
modules: Change all *Reader traits to return Result instead of `O…
3a8a14a 
…ption` (informalsystems#1283)

* ClientReader returns Result

* fix: remove unneeded into()

* add other errors

* remove new errors

* ConnectionReader returns Result

* ChannelReader returns Result

* PortReader returns Result

* modify the change log

* Update .changelog/unreleased/improvements/ibc/1268-reader-result.md

Co-authored-by: Romain Ruetschi <romain.ruetschi@gmail.com>

Co-authored-by: Romain Ruetschi <romain.ruetschi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hu55a1n1 hu55a1n1 hu55a1n1 left review comments

@adizere adizere adizere approved these changes

@romac romac romac left review comments

@ancazamfir ancazamfir Awaiting requested review from ancazamfir

Assignees

@hu55a1n1 hu55a1n1

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yito88 @hu55a1n1 @romac @adizere