-
Notifications
You must be signed in to change notification settings - Fork 610
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
Conversation
…nterfaces and boilerplate
proto/ibc/applications/interchain_accounts/controller/v1/tx.proto
Outdated
Show resolved
Hide resolved
} | ||
|
||
// MsgRegisterAccountResponse defines the response for Msg/RegisterAccount | ||
message MsgRegisterAccountResponse {} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
…cosmos/ibc-go into damian/2051-add-register-account-protos
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
…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]>
Description
Msg
service to interchain accounts controllerRegisterAccount
protobuf types and rpccloses: #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.
docs/
) or specification (x/<module>/spec/
)godoc
comments.Unreleased
section inCHANGELOG.md
Files changed
in the Github PR explorerCodecov Report
in the comment section below once CI passes