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

feat: add From derive for ICS26 messages #938

Merged
merged 1 commit into from
Nov 2, 2023
Merged

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Oct 31, 2023

Add derive_more::From to MsgEnvelope and encapsulated enums so that
it’s easier to construct those objects without having to spell out
their names or variants.


PR author checklist:

  • Added changelog entry, using unclog.
  • [-] Added tests.
  • [-] Linked to GitHub issue.
  • [-] Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

Add derive_more::From to MsgEnvelope and encapsulated enums so that
it’s easier to construct those objects without having to spell out
their names or variants.
@codecov
Copy link

codecov bot commented Oct 31, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (553bc79) 67.65% compared to head (6b44b25) 67.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #938      +/-   ##
==========================================
+ Coverage   67.65%   67.73%   +0.07%     
==========================================
  Files         130      130              
  Lines       16415    16397      -18     
==========================================
  Hits        11106    11106              
+ Misses       5309     5291      -18     
Files Coverage Δ
crates/ibc/src/core/ics02_client/msgs.rs 88.88% <100.00%> (ø)
crates/ibc/src/core/ics03_connection/msgs.rs 86.66% <100.00%> (ø)
crates/ibc/src/core/ics04_channel/msgs.rs 77.27% <100.00%> (ø)
crates/ibc/src/core/msgs.rs 1.03% <100.00%> (ø)

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR. That's fine with us, but before we go:
Could you provide us with the specific location where you have constructed the message in this manner?
To provide some context, historically, the conversion flow for nearly all use cases has involved decoding an incoming IBC message from a proto/raw message and subsequently crafting the corresponding domain IBC message type. (So, this newly derived From shouldn't be needed) Hence, any information that helps us coming up with a more versatile entry point is appreciated.

@mina86
Copy link
Contributor Author

mina86 commented Nov 2, 2023

So far I’ve needed those specific conversion function in unit tests where I craft the messages by hand. Having said that, my current smart contract code (written for Solana using Anchor) expects the message to be borsh-serialised (which is what Anchor uses).

@Farhad-Shabani Farhad-Shabani added this to the v0.48.0 milestone Nov 2, 2023
@Farhad-Shabani Farhad-Shabani merged commit 599644c into cosmos:main Nov 2, 2023
12 checks passed
@mina86 mina86 deleted the b branch November 29, 2023 20:39
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
Add derive_more::From to MsgEnvelope and encapsulated enums so that
it’s easier to construct those objects without having to spell out
their names or variants.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants