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

feat: implementing SubmitTx gRPC endpoint #2147

Merged
merged 11 commits into from
Sep 1, 2022

Conversation

seantking
Copy link
Contributor

Description

closes: #2056


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

@seantking seantking force-pushed the sean/issue#2056-implement-rpc-handler-ica branch from 42a7a1c to 09c053a Compare August 30, 2022 13:00
@seantking seantking marked this pull request as ready for review August 31, 2022 13:19
@seantking
Copy link
Contributor Author

Shall I add an e2e for this test in a follow up PR?

@seantking seantking force-pushed the sean/issue#2056-implement-rpc-handler-ica branch from 28a487c to 743001a Compare August 31, 2022 13:27
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.

Great work. Nice changelog entry. See suggestions. Approving as I think any changes to the MsgSubmitTx fields should be done in a followup (if we go that direction)

Comment on lines +40 to +43
channelID, found := k.GetActiveChannelID(ctx, msg.ConnectionId, portID)
if !found {
return nil, sdkerrors.Wrapf(icatypes.ErrActiveChannelNotFound, "failed to retrieve active channel for port %s", portID)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be taking in the channelID as the message argument? What's the UX benefit here? I'm thinking about a future where we remove the active channel mapping

cc @AdityaSripal

Copy link
Member

Choose a reason for hiding this comment

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

I suppose we could, but i think in that future we will break APIs anyway. So not sure its worth optimizing for that now

}{
{
"success", func() {
owner = TestOwnerAddress
Copy link
Contributor

Choose a reason for hiding this comment

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

set to TestOwnerAddress as default before tc.malleate()

Memo: "memo",
}

timeoutTimestamp := suite.chainA.GetContext().BlockTime().Add(time.Minute).UnixNano()
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a suite.chainA.GetTimeoutHeight() function I think


tc.malleate() // malleate mutates test data

msg := types.NewMsgSubmitTx(owner, connectionID, clienttypes.NewHeight(0, 0), uint64(timeoutTimestamp), packetData)
Copy link
Contributor

Choose a reason for hiding this comment

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

use clienttypes.ZeroHeight() instead

@colin-axner
Copy link
Contributor

Shall I add an e2e for this test in a follow up PR?

#2039, #2063

Copy link
Member

@damiannolan damiannolan left a comment

Choose a reason for hiding this comment

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

Looks more or less good to go, left some nits on tests and we should return sequence in the msg response afaik!
Nice work!

return nil, sdkerrors.Wrap(channeltypes.ErrChannelCapabilityNotFound, "module does not own channel capability")
}

_, err = k.SendTx(ctx, chanCap, msg.ConnectionId, portID, msg.PacketData, msg.TimeoutTimestamp)
Copy link
Member

Choose a reason for hiding this comment

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

We can include the sequence in the MsgSubmitTxResponse, right?


icatypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types"
"github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types"
icacontrollertypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types"
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think we normally alias this as just controllertypes

var (
path *ibctesting.Path
owner string
connectionID string
Copy link
Member

Choose a reason for hiding this comment

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

do we need to expose this var to mutate, or can we just use path.EndpointA.ConnectionID and mutate the path in malleate()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's fine to leave as is 🤝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right actually, I updated it. Thanks


tc.malleate() // malleate mutates test data

msg := types.NewMsgSubmitTx(owner, connectionID, clienttypes.NewHeight(0, 0), uint64(timeoutTimestamp), packetData)
Copy link
Member

Choose a reason for hiding this comment

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

we could expose the msg as test func level var and mutate the fields - owner, connection, packetData..etc in malleate()

it would remove the need for 3 test func level vars I think

@seantking seantking enabled auto-merge (squash) September 1, 2022 07:24
@seantking seantking merged commit 9019adf into main Sep 1, 2022
@seantking seantking deleted the sean/issue#2056-implement-rpc-handler-ica branch September 1, 2022 07:49
mergify bot pushed a commit that referenced this pull request Sep 1, 2022
* feat: implementing SubmitTx gRPC endpoint

* test: adding failure cases

* add capability test

* chore: comment

* chore: cleanup

* chore: changelog

* import & sequence

* nits

* refactor: clean up tests

(cherry picked from commit 9019adf)

# Conflicts:
#	modules/apps/27-interchain-accounts/controller/keeper/msg_server.go
crodriguezvega pushed a commit that referenced this pull request Sep 7, 2022
* feat: implementing SubmitTx gRPC endpoint (#2147)

* feat: implementing SubmitTx gRPC endpoint

* test: adding failure cases

* add capability test

* chore: comment

* chore: cleanup

* chore: changelog

* import & sequence

* nits

* refactor: clean up tests

(cherry picked from commit 9019adf)

# Conflicts:
#	modules/apps/27-interchain-accounts/controller/keeper/msg_server.go

* resolving conflicts

Co-authored-by: Sean King <[email protected]>
Co-authored-by: Damian Nolan <[email protected]>
Co-authored-by: Carlos Rodriguez <[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.

Implement the rpc handler method for SendTx
4 participants