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(ibc-core): port capability #1258

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Conversation

rnbguy
Copy link
Collaborator

@rnbguy rnbguy commented Jun 14, 2024

Closes: #519

  • a port-channel is bound to a module at ChanOpenInit and ChanOpenTry.
  • a module can't use a port-channel bound to another module. e.g. transfer module can't commit packets for "nft-transfer" port.
  • storing port capability at PortPath e.g. ""ports/{id}".
  • modules may release port at channel close.
  • custom port naming.

Description

Ref: https://github.com/cosmos/ibc/blob/main/spec/core/ics-005-port-allocation/README.md


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

Copy link

codecov bot commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 81.48148% with 20 lines in your changes missing coverage. Please review.

Project coverage is 67.34%. Comparing base (8424f69) to head (4c0d95f).

Files Patch % Lines
...ics24-host/types/src/identifiers/capability_key.rs 28.57% 10 Missing ⚠️
ibc-testkit/src/testapp/ibc/core/types.rs 55.55% 4 Missing ⚠️
...rc/testapp/ibc/applications/nft_transfer/module.rs 0.00% 3 Missing ⚠️
...s/ics721-nft-transfer/src/handler/send_transfer.rs 0.00% 2 Missing ⚠️
ibc-testkit/src/testapp/ibc/core/core_ctx.rs 97.14% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1258      +/-   ##
==========================================
+ Coverage   67.27%   67.34%   +0.07%     
==========================================
  Files         235      236       +1     
  Lines       23538    23635      +97     
==========================================
+ Hits        15836    15918      +82     
- Misses       7702     7717      +15     

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

@rnbguy
Copy link
Collaborator Author

rnbguy commented Jun 17, 2024

We do need to locally maintain a capability key for each module. Which should be used to authenticate the modules.

This should be unique to each Module type. This is the only way to make sure different modules can't commit packets on the same ports - even though a malicious module tries to forge a Module identifier.

The capability keys shouldn't be committed to the blockchain store - as they are local and probably distinct across different builds. We can use TypeId for this local capability key - but need to check if using TypeId is ok from a security perspective.

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

Successfully merging this pull request may close these issues.

[Router] Missing parts of ICS05 & ICS26 that deal with the Module management
1 participant