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

Remove DenomTrace and DenomTraces querier rpc's #6421

Closed
3 tasks
colin-axner opened this issue May 29, 2024 · 5 comments · Fixed by #6866
Closed
3 tasks

Remove DenomTrace and DenomTraces querier rpc's #6421

colin-axner opened this issue May 29, 2024 · 5 comments · Fixed by #6866
Assignees
Labels
20-transfer needs discussion Issues that need discussion before they can be worked on

Comments

@colin-axner
Copy link
Contributor

Summary

DenomTrace and DenomTraces rpc were added back to fix build issues in e2e tests due to a dependency using the types. see aa1a5fd

Once the dependencies are resolved, we should be able to remove these rpc's outright and make the DenomTrace internal and only for migration handlers


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@colin-axner colin-axner added this to the v9.0.0 milestone May 29, 2024
@DimitrisJim
Copy link
Contributor

DimitrisJim commented May 29, 2024

In order to keep the context in issue, the error encountered in original PR was:

../../../../../../pkg/mod/github.com/misko9/go-substrate-rpc-client/[email protected]/rpc/ibc/query_denom.go:19:24: undefined: transfertypes.QueryDenomTraceResponse
../../../../../../pkg/mod/github.com/misko9/go-substrate-rpc-client/[email protected]/rpc/ibc/query_denom.go:30:17: undefined: transfertypes.QueryDenomTracesResponse

Seems go-substrate-rpc-client calls transfer's grpc client and requires references to the response types. This is an indirect dependency needed for the 08-wasm tests.

@DimitrisJim
Copy link
Contributor

RPCs have been removed, only the Resp types remain which will ideally be axed after go-substrate-rpc-client stops holding a ref to them.

@DimitrisJim
Copy link
Contributor

Re-slapping needs discussion. Our current blocker bumping interchaintest to commit which doesn't include the rpcs:

Bumping interchain test to include fork of go-substrate-rpc-client that does not include the DenomTrace Response types now brings in interchain-security which depends on ConnectionI which we removed :lolcry:. Can't win.

../../../../../go/pkg/mod/github.com/cosmos/interchain-security/[email protected]/x/ccv/types/expected_keepers.go:85:87: undefined: ibcexported.ConnectionI
FAIL	github.com/cosmos/ibc-go/e2e/tests/core/02-client [build failed]```

@colin-axner
Copy link
Contributor Author

Pulling off v9 milestone as we can get to it when we get to it. Would be nice before v9, but not strictly necessary

@colin-axner colin-axner removed this from the v9.0.0 milestone Jul 17, 2024
@DimitrisJim
Copy link
Contributor

DimitrisJim commented Jul 17, 2024

oh, think we can rm these now since I branched off misko's commit that removed these (well, I can just rm them myself if that wasn't the case since we're on my forks 😅). Will whip the broom out for em if my memory serves me right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
20-transfer needs discussion Issues that need discussion before they can be worked on
Projects
Status: Done 🥳
Development

Successfully merging a pull request may close this issue.

2 participants