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

[IBC] Create initial IBC module #842

Merged
merged 22 commits into from
Jun 27, 2023
Merged

[IBC] Create initial IBC module #842

merged 22 commits into from
Jun 27, 2023

Conversation

h5law
Copy link
Contributor

@h5law h5law commented Jun 18, 2023

Description

This PR introduces the base IBC module and partially implements ICS-24 (paths, keys and identifiers)

Issue

Fixes #794

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • Adds the IBC module interface in shared/modules/ibc_module.go
  • Implements the IBC module and IBC host interface
  • Adds ICS-24 path, keys and identifier logic
  • Add shared/core/types/commitments.go for IBC commitment type aliases
  • Adds custom errors related to IBC in shareds/core/types/errors.go

Testing

  • make develop_test; if any code changes were made
  • make test_e2e on k8s LocalNet; if any code changes were made
  • e2e-devnet-test passes tests on DevNet; if any code was changed
  • Docker Compose LocalNet; if any major functionality was changed or introduced
  • k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@reviewpad reviewpad bot added large Pull request is large waiting-for-review labels Jun 18, 2023
@h5law h5law requested a review from Olshansk June 18, 2023 13:55
@h5law h5law self-assigned this Jun 18, 2023
@h5law h5law added this to the M7: Pocket NoS (North Star) milestone Jun 18, 2023
@h5law h5law added the e2e-devnet-test Runs E2E tests on devnet label Jun 18, 2023
@codecov
Copy link

codecov bot commented Jun 18, 2023

Codecov Report

Patch coverage: 26.75% and project coverage change: -0.08 ⚠️

Comparison is base (2d4f789) 31.52% compared to head (89cc3b0) 31.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #842      +/-   ##
==========================================
- Coverage   31.52%   31.44%   -0.08%     
==========================================
  Files         107      110       +3     
  Lines        9034     9190     +156     
==========================================
+ Hits         2848     2890      +42     
- Misses       5846     5955     +109     
- Partials      340      345       +5     
Impacted Files Coverage Δ
ibc/host/keys.go 0.00% <0.00%> (ø)
runtime/bus.go 15.71% <0.00%> (-0.47%) ⬇️
shared/core/types/error.go 6.04% <0.00%> (-0.11%) ⬇️
ibc/host/identifiers.go 69.38% <69.38%> (ø)
ibc/host/prefix.go 100.00% <100.00%> (ø)

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

@h5law h5law mentioned this pull request Jun 18, 2023
20 tasks
ibc/docs/README.md Outdated Show resolved Hide resolved
ibc/docs/README.md Outdated Show resolved Hide resolved
ibc/docs/README.md Outdated Show resolved Hide resolved
ibc/docs/README.md Outdated Show resolved Hide resolved
- [Node Configuration](#node-configuration)
- [Components](#components)
- [ICS-24 Host Requirements](#ics-24-host-requirements)

Copy link
Contributor

Choose a reason for hiding this comment

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

Wdyt about adding a persistence (or something) section which covers what network state this module interacts with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a brief section to cover this but the rest of the persistence info will come in the ICS-24 Store PR as this is where interfacing with persistence actually comes into play

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

Looked at everything with the exception of ibc/docs/ics24.md

Screenshot 2023-06-22 at 4 02 40 PM

consensus/e2e_tests/utils_test.go Outdated Show resolved Hide resolved
shared/modules/ibc_module.go Outdated Show resolved Hide resolved
shared/modules/ibc_module.go Show resolved Hide resolved
shared/core/types/error.go Outdated Show resolved Hide resolved
runtime/configs/proto/ibc_config.proto Show resolved Hide resolved
ibc/host/keys.go Outdated Show resolved Hide resolved
ibc/docs/README.md Show resolved Hide resolved
ibc/docs/README.md Show resolved Hide resolved
ibc/docs/README.md Show resolved Hide resolved
ibc/docs/README.md Show resolved Hide resolved
option go_package = "github.com/pokt-network/pocket/runtime/configs";

message IBCConfig {
// If IBC is enabled by a node there are two possible states depending on the
Copy link
Member

Choose a reason for hiding this comment

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

🔥

f.Add("port")
}
}
f.Fuzz(func(t *testing.T, idType string) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

@@ -0,0 +1,24 @@
package host
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for splitting these into separate files!

Makes it so much easier to read and understand!

Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

@h5law I know you didn't re-request a review on this but I took another look and thing it's more than good enough.

There are probably some changes we can make, but I don't think anything is a blocker so let's merge it in and iterate on main.

@h5law h5law merged commit 55964eb into main Jun 27, 2023
9 checks passed
@h5law h5law deleted the ibc/initial_module branch June 27, 2023 08:11
bryanchriswhite added a commit that referenced this pull request Jun 28, 2023
* pokt/main:
  [IBC] Create initial IBC module (#842)
  [Persistence] Adds TreeStore logger (#852)
  [Persistence] Change root hash calculation to use an SMT (#843)
bryanchriswhite added a commit that referenced this pull request Jun 28, 2023
* refactor/unicast-router:
  fix: return error
  chore: cleanup unused garbage
  chore: add missing godoc comments
  chore: add submodule TECHDEBT comments
  chore: comment cleanup
  chore: cleanup unused test utils
  [IBC] Create initial IBC module (#842)
  [Persistence] Adds TreeStore logger (#852)
  [Persistence] Change root hash calculation to use an SMT (#843)
bryanchriswhite added a commit that referenced this pull request Jun 29, 2023
* feat/integrate-bg-router: (21 commits)
  chore: improve comment'
  wip: docs: architecture diagrams
  chore: add TECHDEBT comment
  Update E2E_FEATURE_LIST.md
  Update vault_test.go (#862)
  chore: return early
  chore: improve debug logging
  chore: improve comments
  chore: improve variable naming
  fix: `p2pModule#Send()` routing logic
  fix: interim background router bootstrapping
  chore: router logging improvements
  fix: return error
  chore: cleanup unused garbage
  chore: add missing godoc comments
  chore: add submodule TECHDEBT comments
  chore: comment cleanup
  chore: cleanup unused test utils
  [IBC] Create initial IBC module (#842)
  [Persistence] Adds TreeStore logger (#852)
  ...
bryanchriswhite added a commit that referenced this pull request Jun 29, 2023
…duce-submodule

* 'feat/integrate-bg-router' (early part): (21 commits)
  test: fix raintree message target test
  [P2P] refactor: unicast router (#844)
  chore: add TECHDEBT comment
  Update E2E_FEATURE_LIST.md
  Update vault_test.go (#862)
  chore: return early
  chore: improve debug logging
  chore: improve comments
  chore: improve variable naming
  fix: `p2pModule#Send()` routing logic
  fix: interim background router bootstrapping
  chore: router logging improvements
  fix: return error
  chore: cleanup unused garbage
  chore: add missing godoc comments
  chore: add submodule TECHDEBT comments
  chore: comment cleanup
  chore: cleanup unused test utils
  [IBC] Create initial IBC module (#842)
  [Persistence] Adds TreeStore logger (#852)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e-devnet-test Runs E2E tests on devnet ibc IBC specific changes large Pull request is large waiting-for-review
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[IBC] Create the initial IBC module
3 participants