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] Make the IBC Host a submodule with access to the bus #868

Merged
merged 145 commits into from
Jul 13, 2023

Conversation

h5law
Copy link
Contributor

@h5law h5law commented Jun 30, 2023

Description

Summary generated by Reviewpad on 13 Jul 23 10:41 UTC

This pull request includes several changes across multiple files. Here is a summary of the changes:

  1. In the file ibc/handle_message_test.go, the file was renamed to ibc/ibc_msg_mempool_test.go with a similarity index of 85%.
  2. In the same file, the imports were modified, and the strings package was added.
  3. The test function TestHandleMessage_ErrorAlreadyInMempool was renamed to TestEmitMessage_MessageAddedToLocalMempool. Some code within the renamed test function was also modified, including changes in the preparation of test data and the addition of a transaction to the mempool.
  4. The test function TestHandleMessage_BasicValidation_Message was renamed to TestIBCMessage_BasicValidation_Message.
  5. The test function TestHandleMessage_BasicValidation_Transaction was renamed to TestIBCMessage_BasicValidation_Transaction.
  6. A new test function TestHandleMessage_ErrorAlreadyInMempool was added to check for the error of having a duplicate transaction in the mempool.
  7. A new test function TestHandleMessage_ErrorAlreadyCommitted was added to check for the error of having an already committed transaction.
  8. The test function TestHandleMessage_GetIndexedMessage was modified to include changes in the preparation of the environment.
  9. The test function TestHandleMessage_AddToMempool was removed.

Additionally, other files such as treestore_module.go, emitter.go, submodule.go, config.validator4.json, persistence/ibc.go, defaults.go, bus_module.go, ibc_store_module.go, bulk_store_cache.go, ibc_host_module.go, config.validator4.json, persistence/test/benchmark_state_test.go, p2p/README.md, runtime/manager_test.go, shared/node.go, persistence/test/manager_test.go, shared/modules/bulk_store_cache.go, ibc.go, shared/node.go, ics24.md, main_test.go have also been modified.

Please let me know if you need more information or details about any specific change.

Issue

Fixes #854

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

  • Move IBC host into its own submodule
  • Enable local ProvableStore instances to emit events to alter the IBC state tree

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
Copy link
Contributor Author

h5law commented Jul 11, 2023

@Olshansk added tests to cover the flushing and pruning works on new height events

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 have some questions around the test. It might just be me but its not completely clear how the test guarantees what we expect.

@bryanchriswhite Do you mind taking a look at ibc/ibc_handle_event_test.go as well

runtime/configs/proto/ibc_config.proto Outdated Show resolved Hide resolved
ibc/ibc_handle_event_test.go Show resolved Hide resolved
ibc/ibc_handle_event_test.go Outdated Show resolved Hide resolved
ibc/ibc_handle_event_test.go Outdated Show resolved Hide resolved
ibc/ibc_handle_event_test.go Show resolved Hide resolved
ibc/ibc_handle_event_test.go Outdated Show resolved Hide resolved
ibc/ibc_handle_event_test.go Outdated Show resolved Hide resolved
ibc/ibc_handle_event_test.go Outdated Show resolved Hide resolved
ibc/ibc_handle_event_test.go Show resolved Hide resolved
ibc/ibc_handle_event_test.go Outdated Show resolved Hide resolved
@h5law h5law requested a review from Olshansk July 12, 2023 00:04
ibc/docs/ics24.md Show resolved Hide resolved
ibc/ibc_handle_event_test.go Outdated Show resolved Hide resolved
ibc/ibc_handle_event_test.go Outdated Show resolved Hide resolved
ibc/ibc_handle_event_test.go 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.

A couple more nits.

PTAL at #868 (comment) as well

@h5law h5law requested a review from Olshansk July 12, 2023 21:03
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 Please be cognizant of the branches (based branches, merge order etc...), since I just approved two of them.

I suggest:

  1. Merge the base to main
  2. Merge this one with main
  3. Push this one up + update base to main
  4. Sanity check this PR is good
  5. Merge this into main

Other than that, 🚢

@h5law h5law changed the base branch from ibc/initial_stores to main July 13, 2023 10:32
@h5law h5law removed the do not merge Prevent merging even with sufficient approvals label Jul 13, 2023
@h5law h5law merged commit a3a2b5c into main Jul 13, 2023
10 checks passed
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)
Olshansk added a commit that referenced this pull request Jul 13, 2023
## Description

Add back `shared/modules/mocks/mocks.go` so `make develop_test` works out of the box.

## Issue

This was the issue:

![Screenshot 2023-07-13 at 11 45 54 AM](https://github.com/pokt-network/pocket/assets/1892194/f4a11347-096c-4b37-b47d-278cff932a54)

Which was a result of the following change in #868:

![Screenshot 2023-07-13 at 11 43 40 AM](https://github.com/pokt-network/pocket/assets/1892194/4325c775-b4d6-4aa4-9edd-a05ac7e2e033)

And the following validates that this fix works:


![Screenshot 2023-07-13 at 11 44 00 AM](https://github.com/pokt-network/pocket/assets/1892194/3fb3e405-3451-458c-b230-62a5b632558f)


## Type of change

Please mark the relevant option(s):

- [ ] New feature, functionality or library
- [x] Bug fix
- [ ] Code health or cleanup
- [ ] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

- `gco HEAD@{4} shared/modules/mocks/mocks.go`

## Testing

- [x] `make develop_test`; if any code changes were made
- [ ] `make test_e2e` on [k8s LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md); if any code changes were made
- [ ] `e2e-devnet-test` passes tests on [DevNet](https://pocketnetwork.notion.site/How-to-DevNet-ff1598f27efe44c09f34e2aa0051f0dd); if any code was changed
- [ ] [Docker Compose LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md); if any major functionality was changed or introduced
- [ ] [k8s LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md); 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](https://go.dev/blog/godoc) on touched members (see: [tip.golang.org/doc/comment](https://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](https://mermaid-js.github.io) diagrams in the corresponding README(s)
- [ ] I have added, or updated, documentation and [mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*` if I updated `shared/*`README(s)
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] Refactor the IBC host to be a submodule with bus access
4 participants