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

Make configutil public #336

Merged
merged 3 commits into from
Nov 5, 2022
Merged

Make configutil public #336

merged 3 commits into from
Nov 5, 2022

Conversation

agouin
Copy link
Member

@agouin agouin commented Nov 3, 2022

Resolves #335

@agouin agouin requested a review from a team as a code owner November 3, 2022 13:11
@agouin agouin requested a review from boojamya November 3, 2022 13:11
Copy link
Contributor

@DavidNix DavidNix left a comment

Choose a reason for hiding this comment

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

I'm not a fan of github.com/strangelove-ventures/ibctest/v6/test package. It was a short-term solution to break an import cycle. It's poorly named imo. The functionality in there would be better to be part of ibctest root package, but the import cycle prevents it.

In any case, I may opt for github.com/strangelove-ventures/ibctest/v6/configutil vs. github.com/strangelove-ventures/ibctest/v6/test/configutil.

@agouin
Copy link
Member Author

agouin commented Nov 3, 2022

Played around with a few re-structuring ideas for the utils and the test rename. I'm not a huge fan of the utils being a bunch of dirs in the root dir, so landed on this. Thoughts?

@agouin agouin requested a review from DavidNix November 3, 2022 16:50
Copy link
Contributor

@DavidNix DavidNix left a comment

Choose a reason for hiding this comment

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

Thanks for the updates and apologies for my belated review.

The Go developer in me really doesn't want a util package name even if it's just an "umbrella" package. It's incredibly rare in Go and each time I see one in a Go codebase, I immediately think "These authors aren't professional Go developers."

However, that's just my perspective.

I'd be in favor of the simpler getting rid of the top-level util package anyway.

Just have ibctest/chain and ibctest/config packages.

If there's a good reason for a package wrapping chain and config, then perhaps testutil is a better name.

util/util.go Outdated
@@ -0,0 +1 @@
package util
Copy link
Contributor

Choose a reason for hiding this comment

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

It's customary to include a doc.go briefly stating the purpose of the package and any sub packages.

@agouin
Copy link
Member Author

agouin commented Nov 4, 2022

perhaps testutil is a better name.

I like this idea. Seems like both util packages can be merged into this single package so that there is no umbrella package and then we don't need 2 root dirs for utils.

@DavidNix
Copy link
Contributor

DavidNix commented Nov 4, 2022

perhaps testutil is a better name.

I like this idea. Seems like both util packages can be merged into this single package so that there is no umbrella package and then we don't need 2 root dirs for utils.

Works for me! Thanks for living with the churn on this one.

@agouin agouin merged commit f263944 into main Nov 5, 2022
@agouin agouin deleted the andrew/move_toml_to_public branch November 5, 2022 00:25
jtieri pushed a commit that referenced this pull request Jan 6, 2023
* Make configutil public

* Rename test package to chainutil. Make util packages subpackages of util package

* Consolidate into single testutil package
jtieri added a commit that referenced this pull request Jan 9, 2023
* fix: Unable to build ibctest binary (#326)

* feat: Add cosmos message poller (#325)

* fix: change import paths from v6 to v3

* Make configutil public (#336)

* Make configutil public

* Rename test package to chainutil. Make util packages subpackages of util package

* Consolidate into single testutil package

* fix: formatting issue in doc

* fix: change import path from test to testutil

* fix: go mod tidy

* packet-forward-middleware packet memo, retry on timeout, and atomic forwards (#306)

* Make examples its own module

* Make cosmos specific module and run in CI in separate task

* Update e2e test for memo refactor

* Update test to chain-level params

* Use gaia with pfm with configurable timeout and retries

* Update SendIBCTransfer uses

* fix interchain test

* Add GetClients method to Relayer and helper for getting transfer channel between chains

* Update packet forward test for multi-hop, add multi-hop refund test

* Update tests for atomic forwards

* Wait for a block after ack before checking balances

* reduce wait to 1 block

* Add multi-hop flow with refund through chain with native denom. Add assertions for escrow accounts

* Remove stale comment

* handle feedback

* Add TransferOptions

* fix: remove extra argument to NewMsgTransfer call

* build: go mod tidy

* chore: update gaia image version for packet forward tests

* fix: fallback to old version of the interchain accounts demo binary

Co-authored-by: David Nix <[email protected]>
Co-authored-by: Andrew Gouin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusion around ConfigFileOverrides
2 participants