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

clean up {accept,implement}_interface #2977

Closed
pyramation opened this issue Jan 4, 2023 · 4 comments · Fixed by #3165
Closed

clean up {accept,implement}_interface #2977

pyramation opened this issue Jan 4, 2023 · 4 comments · Fixed by #3165
Assignees
Labels
type: code hygiene Clean up code but without changing functionality or interfaces

Comments

@pyramation
Copy link
Contributor

pyramation commented Jan 4, 2023

we need to keep in sync with using full-scoped interface names in protos (already merged in cosmos-sdk)

@pyramation pyramation changed the title clean up `{accept,implement}_interface clean up {accept,implement}_interface Jan 4, 2023
@pyramation
Copy link
Contributor Author

cc @AmauryM

@damiannolan
Copy link
Member

Hey, thanks for opening the issue. So, as I understand it, any protobuf defintions which make use of cosmos_proto.implements_interface should use the full typeURL excluding the preceding / which we normally see used in the sdk?

I think this is already covered for proposals here and here.

InterchainAccount uses this annoation here but it references a local/relative interface. Should this be updated to a fully qualified typeURL?

Aside from that the only other outstanding one I can think of is the feature branch work for ics20 authz. The TransferAuthorization should be updated to include the full typeURL here.

@pyramation
Copy link
Contributor Author

pyramation commented Jan 10, 2023

The issue was that, while originally the spec allowed for local/relative/full-path references, they weren't correct in that many folks just put arbitrary names as shown here. The best solution for now that was discussed with the SDK team was to simply remove local/relative paths and only use fully-scoped to remove any ambiguity.

Hey, thanks for opening the issue. So, as I understand it, any protobuf defintions which make use of cosmos_proto.implements_interface should use the full typeURL excluding the preceding / which we normally see used in the sdk?

yes :)

InterchainAccount uses this annoation here but it references a local/relative interface. Should this be updated to a fully qualified typeURL?

Aside from that the only other outstanding one I can think of is the feature branch work for ics20 authz. The TransferAuthorization should be updated to include the full typeURL here.

yea so anything not fully qualified, needs to be changed to have full path, otherwise it won't conform to the new standard.

@crodriguezvega crodriguezvega added the type: code hygiene Clean up code but without changing functionality or interfaces label Jan 11, 2023
@pyramation
Copy link
Contributor Author

also related CosmWasm/wasmd#1155

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: code hygiene Clean up code but without changing functionality or interfaces
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants