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

chore: adding basic Msg service and RegisterAccount rpc boilerplate #2068

Merged
merged 7 commits into from
Aug 23, 2022

Conversation

damiannolan
Copy link
Member

@damiannolan damiannolan commented Aug 22, 2022

Description

  • Adding Msg service to interchain accounts controller
  • Adding RegisterAccount protobuf types and rpc
  • Registering codec types
  • Adding boilerplate to satisfy compiler

closes: #2051


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

}

// MsgRegisterAccountResponse defines the response for Msg/RegisterAccount
message MsgRegisterAccountResponse {}
Copy link
Contributor

@colin-axner colin-axner Aug 22, 2022

Choose a reason for hiding this comment

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

Is it useful to return the portID? I guess not since the generation of the portID is stateless?

The channelID would be useful? Actual implementation of that could be in a followup pr

Copy link
Member Author

Choose a reason for hiding this comment

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

I think If we're going to add the channelID we may need to break the RegisterInterchainAccount API to return it?
That is, depending on how we approach this...

I'm just thinking it would be need to be retrieved from the MsgChannelOpenInitResponse from sdk.Result.MsgResponses[0]: https://github.com/cosmos/ibc-go/blob/main/modules/apps/27-interchain-accounts/controller/keeper/account.go#L44

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to break the RegisterInterchainAccount API?

I was thinking we would make a private function registerInterchainAccount (with a slightly diff name) which both RegisterInterchainAccount and the rpc RegisterAccount call into, that way we can add more info to the API

Copy link
Member Author

Choose a reason for hiding this comment

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

Great point! That sounds good to me! :)

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.

Nice!

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

🚀

@damiannolan damiannolan enabled auto-merge (squash) August 23, 2022 08:22
@damiannolan damiannolan merged commit 5765fe7 into main Aug 23, 2022
@damiannolan damiannolan deleted the damian/2051-add-register-account-protos branch August 23, 2022 08:54
mergify bot pushed a commit that referenced this pull request Aug 24, 2022
…te (#2068)

* adding new controller msg service, register account types, register interfaces and boilerplate

* fixing typo

* fixing protodoc and regenerate godocs

* adding channel id to MsgRegisterAccountResponse

(cherry picked from commit 5765fe7)
damiannolan added a commit that referenced this pull request Aug 25, 2022
…te (#2068) (#2109)

* adding new controller msg service, register account types, register interfaces and boilerplate

* fixing typo

* fixing protodoc and regenerate godocs

* adding channel id to MsgRegisterAccountResponse

(cherry picked from commit 5765fe7)

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

Successfully merging this pull request may close these issues.

Add RegisterAccount rpc and proto message definitions
4 participants