pokt-network/pocket

[Core] Preventing import cycles

bryanchriswhite opened this issue · 1 comments

bryanchriswhite commented

Objective

Establish a convention of strictly importing "from below".

Origin Document

Observations made while working on #855 have brought to my attention that we are not strictly adhering to this everywhere, resulting in import cycles waiting to happen. This makes refactoring feel a bit like a minefield which will lead to reluctance to do so.

When one encounters such a case, the resulting MVC has a tendency to explode in size and complexity, making it more difficult to reason through for author and reviewer alike.

Goals

  • Identify upside-down imports
  • Construct a holistic plan for how to restructure effected packages
  • Enact the plan

Deliverable

  • Add a section to the developer documentation
  • Identify all upside-down imports in (sub)module types and members
    • E.g.: /shared/modules/persistence_module.go imports /persistence/blockstore
    • #811
    • /shared/modules/persistence_module.go imports /persistence/indexer
  • Draft a (holistic) plan for how to restructure effected packages
    • Create shared interfaces in and/or moved shared types to (a) shared package(s)
  • Refactor each effected module, one-by-one
    • P2P
    • Persistence
    • ...

Non-goals / Non-deliverables

  • Identifying or fixing unrelated and non-critical upside-down imports (prioritize (sub)module APIs)
  • Unrelated refactoring; e.g. changing APIs, type definitions, etc.

General issue deliverables

  • Update the appropriate CHANGELOG(s)
  • Update any relevant local/global README(s)
  • Update relevant source code tree explanations
  • Add or update any relevant or supporting mermaid diagrams

Testing Methodology

  • golangci-lint: make lint
  • All tests: make test_all
  • LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md
  • k8s LocalNet: verify a k8s LocalNet is still functioning correctly by following the instructions here

Creator: @bryanchriswhite
Co-Owners: @dylanlott

Olshansk commented

@bryanchriswhite

  1. Could you provide a few more details about the specific import where you hit the issue? Code snippets are sufficient and no need for extensive explanations

  2. If you have any (rough) ideas of how you worked around in this case or thinking about it, it would help as well.

I understand cyclical dependencies but don't have a good understand of the problem from the context in this github issues as a standalone.