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

Implement ICS-26 router for RecvPacket in {Validation,Execution}Context #377

Merged
merged 16 commits into from
Jan 31, 2023

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Jan 26, 2023

Work towards: #322

Changed the scope of this PR to include only recvPacket, since it turned out to have many subtleties to get right.

Description

As with the other PRs, this one focuses on correctness. The more we get closer to being done with #322, the more it becomes obvious that we need a refactor. Let's hold off until we're completely done though; it will be easier to make a big refactor after #279.


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).

@plafer plafer changed the title Plafer/322 packets Implement ICS-26 router for packets in {Validation,Execution}Context Jan 26, 2023
@codecov
Copy link

codecov bot commented Jan 26, 2023

Codecov Report

Base: 60.42% // Head: 58.76% // Decreases project coverage by -1.67% ⚠️

Coverage data is based on head (147ec47) compared to base (8ed5c7e).
Patch coverage: 4.02% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #377      +/-   ##
==========================================
- Coverage   60.42%   58.76%   -1.67%     
==========================================
  Files         124      124              
  Lines       15665    16116     +451     
==========================================
+ Hits         9466     9470       +4     
- Misses       6199     6646     +447     
Impacted Files Coverage Δ
crates/ibc/src/applications/transfer/context.rs 42.17% <0.00%> (-1.94%) ⬇️
.../src/applications/transfer/relay/on_recv_packet.rs 0.00% <0.00%> (ø)
...s/ibc/src/clients/ics07_tendermint/client_state.rs 43.37% <0.00%> (-2.69%) ⬇️
crates/ibc/src/core/context.rs 4.73% <0.00%> (-1.12%) ⬇️
crates/ibc/src/core/ics02_client/client_state.rs 75.00% <ø> (ø)
crates/ibc/src/core/ics04_channel/handler.rs 96.66% <0.00%> (-0.41%) ⬇️
.../ibc/src/core/ics04_channel/handler/recv_packet.rs 42.08% <0.00%> (-38.07%) ⬇️
...ore/ics04_channel/handler/write_acknowledgement.rs 83.01% <ø> (-1.20%) ⬇️
crates/ibc/src/core/ics26_routing/context.rs 43.84% <ø> (ø)
crates/ibc/src/mock/client_state.rs 72.69% <0.00%> (-3.39%) ⬇️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@plafer plafer changed the title Implement ICS-26 router for packets in {Validation,Execution}Context Implement ICS-26 router for onRecvPacket in {Validation,Execution}Context Jan 26, 2023
@plafer plafer changed the title Implement ICS-26 router for onRecvPacket in {Validation,Execution}Context Implement ICS-26 router for RecvPacket in {Validation,Execution}Context Jan 26, 2023
@plafer plafer marked this pull request as ready for review January 27, 2023 16:49
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.

All good 👌
I just left some thoughts to get also your take on them

@Farhad-Shabani Farhad-Shabani merged commit e845af6 into main Jan 31, 2023
@Farhad-Shabani Farhad-Shabani deleted the plafer/322-packets branch January 31, 2023 19:15
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
…ntext` (#377)

* packet validation  scaffoldinig

* recv_packet validation

* remove todo

* recv_packet_validate()

* execute scaffolding

* Module::on_recv_packet_execute

* token transfer on_recv_packet (with a FIXME)

* recv_packet_execute

* refactor expression for readability

* add comment

* ack cannot be empty

* token transfer ack

* clippy

* fix token transfer recv_packet

* typo
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.

Make ValidationContext and ExecutionContext implement the ICS-26 router
2 participants