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] Implement ICS-24 - Tracking IBC store transitions in the network state #847

Merged
merged 76 commits into from
Jul 13, 2023

Conversation

h5law
Copy link
Contributor

@h5law h5law commented Jun 20, 2023

Description

Summary generated by Reviewpad on 12 Jul 23 20:45 UTC

This pull request includes the following changes:

  • The file main_test.go was newly added to the repository and includes various changes such as package definition, imports, variable declaration, and function definitions related to IBC modules testing.

  • The file proofs_ics23.go includes changes in error handling for the functions createMembershipProof and createNonMembershipProof, replacing the error coreTypes.ErrCreatingProof with coreTypes.ErrIBCCreatingProof for more specific error messages.

  • The file keys_ics03.go was renamed from the ibc/host directory to the ibc/path directory. Additionally, the package declaration was changed from host to path. These changes seem related to the organization of the file within the codebase.

  • The config.validator2.json file includes changes in the ibc section, adding two new keys: private_key and stores_dir. The enabled key remains unchanged. The private_key key has been assigned a long string value, and the stores_dir key has been assigned the value "/var/ibc".

  • The file internal/testutil/ibc/mock.go includes renaming of the functions BaseIbcMock to BaseIBCMock and IbcMockWithHost to IBCMockWithHost. These changes update the function names to follow proper naming conventions.

  • The file includes the implementation of a store manager for IBC (Inter-Blockchain Communication), responsible for managing provable stores, caching, pruning, and restoring data.

  • The addition of a new function, GetIBCStoreUpdates, which retrieves the set of key-value pairs updated at the current height for the IBC store.

  • Changes in the state hashes used in the tests in the file state_test.go.

  • The file runtime/manager_test.go includes the addition of new fields PrivateKey and StoresDir to the IBC struct in the NewManagerFromReaders function.

  • Changes in the config.validator4.json file include modifications in the ibc section, adding two new properties: private_key and stores_dir. The enabled property remains unchanged.

  • The file prefix.go was renamed from the ibc/host package to the ibc/path package. The similarity index indicates that most of the content remains unchanged.

  • There was a change in error handling logic in a file involving the renaming of the error variable and addressing the error during the creation of a new public key.

  • The file gov.go includes changes in import statements, variable declarations, and function modifications related to new IBC store transaction fees.

  • The file persistence/types/ibc.go is a new file added to the codebase, containing functions and constants related to IBC.

  • The file persistence/types/ibc.go in the persistence package includes changes, introducing methods for setting and retrieving key-value pairs in the IBC store table.

  • The file persistence/trees.go includes changes related to the addition of a new tree and corresponding functions to handle updates for the tree.

  • The file utils_test.go includes changes in the casing of a function call from ibcUtils.IbcMockWithHost to ibcUtils.IBCMockWithHost.

  • The file ibc/host/path_test.go was renamed to ibc/path/path_test.go with no other changes in the file.

  • Changes in default variable values related to IBC functionality.

  • The file db.go includes a new function, initialiseIBCTables, responsible for initializing an IBC store table in a PostgreSQL database.

  • A new file, ibc.feature.wip, was added, containing integration and end-to-end tests for various scenarios related to IBC functionality.

  • Changes in the PROTOCOL_STATE_HASH.md file include additions and modifications related to the IBC state tree and the handling of IBCMessage objects.

  • The file ibc/host.go includes changes in comments, import statements, and the addition of fields and methods related to the host's interaction with the IBC store.

  • A new file, convert.go, was added in the ibc/types directory, containing functions and imports to work with IBC messages.

  • Changes in the utils_test.go file include modifications in the casing of a function call from ibcUtils.IbcMockWithHost to ibcUtils.IBCMockWithHost.

  • The file ibc/host/path_test.go was renamed to ibc/path/path_test.go with no other changes in the file.

  • Changes in default variable values related to IBC functionality.

  • A new file, db.go, was added, containing a function

Issue

Fixes #840

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

  • Add new IbcMessage protobuf with 2 types
    • UpdateIbcStore and PruneIbcStore
  • Track IBC store changes in the mempool
  • Update IBC state tree using these state transition messages

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)

@h5law h5law added the ibc IBC specific changes label Jun 20, 2023
@h5law h5law added this to the M7: Pocket NoS (North Star) milestone Jun 20, 2023
@h5law h5law self-assigned this Jun 20, 2023
@reviewpad reviewpad bot added the large Pull request is large label Jun 20, 2023
@h5law
Copy link
Contributor Author

h5law commented Jun 20, 2023

Note: This PR is based ontop of #843 (inturn also on #756), #842 and #845 but the main logic for the mempool interactions, ICS-23 proof generation/verification and ICS-24 stores is in place

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Patch coverage: 18.91% and project coverage change: -0.54 ⚠️

Comparison is base (89cc3b0) 31.44% compared to head (0a0a067) 30.91%.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           ibc/initial_module     #847      +/-   ##
======================================================
- Coverage               31.44%   30.91%   -0.54%     
======================================================
  Files                     110      114       +4     
  Lines                    9190     9595     +405     
======================================================
+ Hits                     2890     2966      +76     
- Misses                   5955     6282     +327     
- Partials                  345      347       +2     
Impacted Files Coverage Δ
ibc/store/provable_store.go 0.00% <0.00%> (ø)
persistence/trees/trees.go 0.00% <0.00%> (ø)
persistence/types/ibc.go 0.00% <0.00%> (ø)
shared/core/types/error.go 5.84% <0.00%> (-0.21%) ⬇️
shared/messaging/events.go 0.00% <ø> (ø)
utility/unit_of_work/transaction.go 45.20% <0.00%> (ø)
utility/unit_of_work/tx_message_handler.go 32.22% <0.00%> (-2.30%) ⬇️
utility/unit_of_work/tx_message_signers.go 37.50% <0.00%> (-6.25%) ⬇️
utility/utility_message_handler.go 0.00% <ø> (ø)
ibc/store/proofs_ics23.go 92.68% <92.68%> (ø)
... and 1 more

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

@h5law h5law changed the title [WIP][IBC] Implement ICS-23 and ICS-24 - Tracking IBC store transitions in the network state [WIP][IBC] Implement ICS-24 - Tracking IBC store transitions in the network state Jun 21, 2023
@h5law h5law changed the base branch from ibc/initial_module to ibc/ics23_integration June 21, 2023 20:12
@gitguardian
Copy link

gitguardian bot commented Jun 21, 2023

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

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.

Reviewed a handful of files and replied to the question around testing.

Lmk if it helps and also please rebased this and the other branches with master when you have a chance!

Makefile Show resolved Hide resolved
runtime/configs/proto/ibc_config.proto Show resolved Hide resolved
ibc/docs/ics24.md Outdated Show resolved Hide resolved
ibc/docs/ics24.md Outdated Show resolved Hide resolved
ibc/docs/ics24.md Outdated Show resolved Hide resolved
ibc/host.go Outdated Show resolved Hide resolved
ibc/host.go Show resolved Hide resolved
ibc/handle_message_test.go Outdated Show resolved Hide resolved
ibc/handle_message_test.go Show resolved Hide resolved
ibc/host.go Outdated Show resolved Hide resolved
@h5law h5law force-pushed the ibc/ics23_integration branch 2 times, most recently from 36a8056 to 8cd1fab Compare June 26, 2023 09:04
@h5law
Copy link
Contributor Author

h5law commented Jun 26, 2023

🚨 I REWROTE HISTORY 🚨

This PR was based off of numerous other branches which have now been merged into main. Because of this I have rebased entirely off of ibc/ics23_integration and this the commit history has changed.

@h5law h5law changed the title [WIP][IBC] Implement ICS-24 - Tracking IBC store transitions in the network state [IBC] Implement ICS-24 - Tracking IBC store transitions in the network state Jun 26, 2023
@h5law h5law added do not merge Prevent merging even with sufficient approvals e2e-devnet-test Runs E2E tests on devnet labels Jun 26, 2023
@h5law h5law marked this pull request as ready for review June 26, 2023 14:01
@h5law h5law requested a review from Olshansk June 26, 2023 14:01
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.

Reviewed most of the PR but still have to get into the meat of things

Screenshot 2023-06-28 at 10 19 52 PM

I think we should be able to get this in soon!

ibc/docs/ics24.md Outdated Show resolved Hide resolved
ibc/docs/ics24.md Show resolved Hide resolved
ibc/docs/ics24.md Show resolved Hide resolved
ibc/docs/ics24.md Outdated Show resolved Hide resolved
ibc/docs/ics24.md Outdated Show resolved Hide resolved
ibc/ibc.feature Outdated Show resolved Hide resolved
ibc/types/messages.go Show resolved Hide resolved
ibc/types/messages.go Show resolved Hide resolved
persistence/types/ibc.go Outdated Show resolved Hide resolved
persistence/trees/trees.go Outdated Show resolved Hide resolved
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.

Still not done but made progress and left a couple important comments. Thanks for bearing with the back & forth!

Screenshot 2023-07-03 at 7 20 18 PM

shared/modules/persistence_module.go Show resolved Hide resolved
utility/unit_of_work/gov.go Outdated Show resolved Hide resolved
ibc/host.go Outdated Show resolved Hide resolved
ibc/module.go Show resolved Hide resolved
ibc/docs/ics24.md Outdated Show resolved Hide resolved
ibc/store/provable_store.go Outdated Show resolved Hide resolved
persistence/trees/trees.go Outdated Show resolved Hide resolved
persistence/trees/trees.go Outdated Show resolved Hide resolved
ibc/store/provable_store.go Show resolved Hide resolved
ibc/store/provable_store.go Outdated Show resolved Hide resolved
@h5law h5law requested a review from Olshansk July 4, 2023 09:37
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.

Screenshot 2023-07-11 at 12 27 57 PM

@h5law Can you please merge with main before we take another look at this?

@h5law
Copy link
Contributor Author

h5law commented Jul 11, 2023

Can you please merge with main before we take another look at this?

@Olshansk Dunno why it did this, I merged with main earlier today o-o did again should be fine now

@Olshansk
Copy link
Member

@h5law Replied to some of the lingering comments. Re-request a review once done / replied to and I'll take a final look.

@h5law h5law requested a review from Olshansk July 12, 2023 20:45
@h5law h5law merged commit 1742f81 into main Jul 13, 2023
bryanchriswhite added a commit that referenced this pull request Jul 13, 2023
* pokt/main:
  [IBC] Make the IBC Host a submodule with access to the bus (#868)
  [IBC] Implement ICS-24 - Tracking IBC store transitions in the network state (#847)
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] Track and Include the IBC store in the network state (ICS-24)
3 participants