Skip to content

Comments

[Chore] Registers the TreeStore onto the bus#856

Closed
dylanlott wants to merge 2 commits intochore/introduce-submodulefrom
chore/register-treestore-module
Closed

[Chore] Registers the TreeStore onto the bus#856
dylanlott wants to merge 2 commits intochore/introduce-submodulefrom
chore/register-treestore-module

Conversation

@dylanlott
Copy link
Contributor

Description

tl;dr- a one-liner that registers the TreeStore submodule to the bus.

This was enabled by a change made in #855 after the TreeStore was modularized that creates a concrete Submodule implementation and allows them to be passed to the bus registration function like full modules. This PR updates the TreeStore to take full advantage of that submodule interface change and expose it on the bus for use in external modules, e.g. the IBC module.

Issue

References a discussion abodut submodules and bus registration brought up during #808
Updates the TreeStore submodule to register itself on the bus as a submodule which was enabled by #855

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

  • Registers the TreeStore submodule on the main bus

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)

@dylanlott dylanlott added the persistence Persistence specific changes label Jun 23, 2023
@dylanlott dylanlott self-assigned this Jun 23, 2023
@dylanlott dylanlott requested a review from Olshansk June 23, 2023 19:28
@reviewpad reviewpad bot added small Pull request is small waiting-for-review labels Jun 23, 2023
@bryanchriswhite bryanchriswhite force-pushed the chore/introduce-submodule branch from 6730e6f to 11bc8fa Compare June 26, 2023 18:21
Copy link
Contributor

@h5law h5law left a comment

Choose a reason for hiding this comment

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

@dylanlott Could you also add a function to the bus like so GetTreeStore() in shared/modules/bus_module.go and runtime/bus.go so the module can be retirved from the registry?

Also not sure if the treestore module has it but just incase the module registry requires the GetModuleName() function in order to register the module

@bryanchriswhite bryanchriswhite force-pushed the chore/introduce-submodule branch from 92360c8 to fcb480e Compare June 27, 2023 15:56
@dylanlott
Copy link
Contributor Author

@dylanlott Could you also add a function to the bus like so GetTreeStore() in shared/modules/bus_module.go and runtime/bus.go so the module can be retirved from the registry?

Done 👍

Also not sure if the treestore module has it but just incase the module registry requires the GetModuleName() function in order to register the module

It existed, it was just in trees/trees.go. I moved it to trees/module.go though because it's better there.

@dylanlott dylanlott requested a review from h5law June 29, 2023 16:10
Copy link
Contributor

@h5law h5law left a comment

Choose a reason for hiding this comment

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

LFG!

Left a comment you need to use the treestore module name and type in the bus get function but besides that its 💶

@dylanlott
Copy link
Contributor Author

dylanlott commented Jun 29, 2023

LFG!

Left a comment you need to use the treestore module name and type in the bus get function but besides that its 💶

So, I was going to bring that up: We can't use the name of the tree store submodule, we have to access it through the persistence module's getter because it's a Submodule. Okay I see the problem now. Investigating a better solution now.

}

func (m *bus) GetTreeStoreModule() modules.TreeStoreModule {
return getModuleFromRegistry[modules.PersistenceModule](m, modules.PersistenceModuleName).GetBus().GetTreeStoreModule()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return getModuleFromRegistry[modules.PersistenceModule](m, modules.PersistenceModuleName).GetBus().GetTreeStoreModule()
return getModuleFromRegistry[modules.TreeStoreModule](m, modules.TreeStoreModuleName)

This works, its what I'm currently using. Once the getModuleFromRegistry takes Submodule not Module this can merge. As we register it to the bus this should be the way we retrieve not through persistence anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

@dylanlott
Copy link
Contributor Author

dylanlott commented Jun 29, 2023

Superseded by #861

@dylanlott dylanlott closed this Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

persistence Persistence specific changes small Pull request is small waiting-for-review

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants