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.

Sign up for GitHub

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

Jump to bottom

Add coordinator Setup functions to the Path type #5397

Merged
merged 21 commits into from
Jan 24, 2024
Merged

Add coordinator Setup functions to the Path type #5397

merged 21 commits into from
Jan 24, 2024

Conversation

chandiniv1
Copy link
Contributor

closes: #3999

@chandiniv1 chandiniv1 requested a review from srdtrk as a code owner December 19, 2023 22:23
@chatton
Copy link
Contributor

chatton commented Jan 2, 2024

Thanks @chandiniv1, it looks like there are still some compilation errors, specifically uses of suite.coordinator.Setup (which is not defined)

@chandiniv1
Copy link
Contributor Author

Thanks @chandiniv1, it looks like there are still some compilation errors, specifically uses of suite.coordinator.Setup (which is not defined)

@chatton I have fixed the compilation errors

chatton
chatton reviewed Jan 3, 2024
View reviewed changes
Copy link
Contributor

@chatton chatton left a comment

Choose a reason for hiding this comment

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

Looks great! The only thing is that the docstrings seem to be a bit stale, I've proposed some suggestions for changes.

After this I think it looks good to merge, thank you!

testing/path.go Outdated Show resolved Hide resolved
testing/path.go Outdated Show resolved Hide resolved
testing/path.go Show resolved Hide resolved
testing/path.go Outdated Show resolved Hide resolved
@chandiniv1 chandiniv1 requested a review from chatton January 3, 2024 11:32
chatton
chatton approved these changes Jan 3, 2024
View reviewed changes
Copy link
Contributor

@chatton chatton left a comment

Choose a reason for hiding this comment

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

LGTM!

@crodriguezvega crodriguezvega added the priority PRs that need prompt reviews label Jan 15, 2024
colin-axner
colin-axner reviewed Jan 24, 2024
View reviewed changes
testing/coordinator.go Outdated Show resolved Hide resolved
colin-axner
colin-axner approved these changes Jan 24, 2024
View reviewed changes
Copy link
Contributor

@colin-axner colin-axner left a comment

Choose a reason for hiding this comment

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

Thanks @chandiniv1!

DimitrisJim
DimitrisJim reviewed Jan 24, 2024
View reviewed changes
Copy link
Contributor

@DimitrisJim DimitrisJim left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice, I'm seeing a couple of references still? (grep -inr "suite.coordinator.Setup" . --include="*_test.go" to find).

these were probably introduced in the merge of main. fixing them.

docs/docs/05-migrations/13-v8-to-v9.md Outdated Show resolved Hide resolved
testing/path.go Outdated Show resolved Hide resolved
DimitrisJim
DimitrisJim approved these changes Jan 24, 2024
View reviewed changes
Copy link
Contributor

@DimitrisJim DimitrisJim left a comment

Choose a reason for hiding this comment

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

lgtm! thanks so much for picking it up!

@DimitrisJim
Update testing/path.go
f9ecf0b 
remove docstring about being connected to ibc-transfer, that is done via CreateTransferChannels
@crodriguezvega crodriguezvega merged commit 01e6c7c into cosmos:main Jan 24, 2024
65 of 67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colin-axner colin-axner colin-axner approved these changes

@AdityaSripal AdityaSripal AdityaSripal approved these changes

@DimitrisJim DimitrisJim DimitrisJim approved these changes

@chatton chatton chatton approved these changes

@charleenfei charleenfei Awaiting requested review from charleenfei

@damiannolan damiannolan Awaiting requested review from damiannolan damiannolan is a code owner

@crodriguezvega crodriguezvega Awaiting requested review from crodriguezvega

@srdtrk srdtrk Awaiting requested review from srdtrk srdtrk is a code owner

Assignees
No one assigned
Labels
priority PRs that need prompt reviews
Projects
ibc-go
Status: Done 🥳
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add coordinator Setup functions to the Path type
6 participants
@chandiniv1 @chatton @DimitrisJim @AdityaSripal @colin-axner @crodriguezvega