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

Add coordinator Setup functions to the Path type #3999

Closed
3 tasks
colin-axner opened this issue Jul 4, 2023 · 3 comments · Fixed by #5397
Closed
3 tasks

Add coordinator Setup functions to the Path type #3999

colin-axner opened this issue Jul 4, 2023 · 3 comments · Fixed by #5397
Labels
testing Testing package and unit/integration tests type: code hygiene Clean up code but without changing functionality or interfaces type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.

Comments

@colin-axner
Copy link
Contributor

Summary

The coordinator has the following functions:

type Coordinator interface {
    IncrementTime() 
    IncrementTimeBy(increment time.Duration)
    UpdateTime()
    UpdateTimeForChain(chain *TestChain)
    
    Setup(path *Path) 
    SetupClients(path *Path)
    SetupConnections(path *Path)

    CreateConnections(path *Path)
    CreateMockChannels(path *Path)
    CreateTransferChannels(path *Path)
    CreateChannels(path *Path)
    
    GetChain(chainID string) *TestChain
    
    CommitBlock(chains ...*TestChain)
    CommitNBlocks(chain *TestChain, n uint64)
}

The following functions should be moved to be under the path type:

type Path interface {
    Setup() 
    SetupClients()
    SetupConnections()
    
    CreateConnections()
    CreateChannels()
}

I think we could consider a way to reduce this API further (definitely keeping Setup(), but maybe reducing the redundancy between SetupConnections and CreateConnections)

Problem Definition

The coordinator is an unnecessary type in the setup of the path. It is more natural to simply reference the path type directly. This clears the way to reduce the usage of coordinator which in the long run may be an unnecessary layer of the testing pkg.

Proposal

Change:

    path := ibctesting.NewPath(suite.chainA, suite.chainB)
    suite.coordinator.Setup(path)

to

    path := ibctesting.NewPath(suite.chainA, suite.chainB)
    path.Setup()

NOTE: I recommend we leave the coordinator functions (deprecation) and add the functions to the path and replace our internal usage. After release of v8, then I think we can remove the unnecessary coordinator functions. Because v8 is a massive release that is intermixed with a SDK and cometbft update, I think we should limit unnecessary breaking changes


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner added testing Testing package and unit/integration tests type: code hygiene Clean up code but without changing functionality or interfaces type: refactor Architecture, code or CI improvements that may or may not tackle technical debt. labels Jul 4, 2023
@crodriguezvega crodriguezvega moved this to Backlog in ibc-go Nov 6, 2023
@colin-axner colin-axner added change: api breaking Issues or PRs that break Go API (need to be release in a new major version) and removed change: api breaking Issues or PRs that break Go API (need to be release in a new major version) labels Nov 7, 2023
@colin-axner
Copy link
Contributor Author

colin-axner commented Nov 8, 2023

Lets add the proposed functions to path and deprecate the legacy functions on the coordinator. I will open a separate issue to remove the deprecated functions. That way we can make this change and choose when to have users update at a reasonable time period

see #5063

@hoangdv2429
Copy link
Contributor

Can I work on this sir ?

@DimitrisJim
Copy link
Contributor

DimitrisJim commented Jan 25, 2024

a follow up Q, any reason why CreateMockChannels(path *Path) and CreateTransferChannels(path *Path) shouldn't be moved?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Testing package and unit/integration tests type: code hygiene Clean up code but without changing functionality or interfaces type: refactor Architecture, code or CI improvements that may or may not tackle technical debt.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants