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

ICS5: Integrate Dynamic Capabilities into IBC #5872

Closed
wants to merge 4 commits into from

Conversation

AdityaSripal
Copy link
Member

Closes: #5542

Description

Use Dynamic Capability Keeper in IBC in accordance with spec

TODO: Route packets to modules given the portID's they own. Noting that portID != modulename


For contributor use:

  • 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

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@AdityaSripal AdityaSripal changed the base branch from master to ibc-alpha March 26, 2020 05:59
simapp/app.go Outdated Show resolved Hide resolved
@@ -33,38 +35,45 @@ func (k Keeper) ChanOpenInit(
connectionHops []string,
portID,
channelID string,
portCap capability.Capability,
Copy link
Member Author

Choose a reason for hiding this comment

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

Passed in by caller as described in spec:

Note: If the host state machine is utilising object capability authentication (see ICS 005), all functions utilising ports take an additional capability parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's correct. Working on updating the spec.

x/ibc/04-channel/keeper/handshake.go Outdated Show resolved Hide resolved
x/ibc/04-channel/keeper/keeper.go Outdated Show resolved Hide resolved
x/ibc/05-port/keeper/keeper.go Show resolved Hide resolved
@@ -33,7 +34,7 @@ func (k Keeper) isBounded(portID string) bool {
// Ports must be bound statically when the chain starts in `app.go`.
// The capability must then be passed to a module which will need to pass
// it as an extra parameter when calling functions on the IBC module.
func (k *Keeper) BindPort(portID string) sdk.CapabilityKey {
func (k *Keeper) BindPort(ctx sdk.Context, portID string) capability.Capability {
Copy link
Member Author

Choose a reason for hiding this comment

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

Wrote this function gleaning as much as i could from ICS-5 and capability implementation

From my understanding, a module will call this function. IBC will then create a capability and return it to the caller.

The calling module can then call ClaimCapability to own the capability and thus the port associated with the capability.

At this point, both the IBC module and the calling module own the capability and can authenticate it.

Is this the correct behavior?

Copy link
Contributor

@cwgoes cwgoes Mar 26, 2020

Choose a reason for hiding this comment

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

See updated spec:

function bindPort(id: Identifier): CapabilityKey {
    abortTransactionUnless(validatePortIdentifier(id))
    abortTransactionUnless(getCapability(portPath(id)) === null)
    capability = newCapability(portPath(id))
    return capability
}

x/ibc/keeper/keeper.go Outdated Show resolved Hide resolved
@cwgoes
Copy link
Contributor

cwgoes commented Mar 26, 2020

Thanks @AdityaSripal!

Please see the spec updates in cosmos/ibc#400 which should resolve outstanding questions.

simapp/app.go Outdated Show resolved Hide resolved
@@ -8,9 +8,9 @@ import (

// HandleMsgChannelOpenInit defines the sdk.Handler for MsgChannelOpenInit
func HandleMsgChannelOpenInit(ctx sdk.Context, k keeper.Keeper, msg types.MsgChannelOpenInit) (*sdk.Result, error) {
err := k.ChanOpenInit(
_, err := k.ChanOpenInit(
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what to do with the returned capability here. I see how a module can call the keeper's ChanOpenInit method, claim the returned capability and continue the handshake from there.

However, I don't see how to return the capability to an end user since it's only the same capability if the end user somehow gets the same reference passed to it. For example, I don't see how this would work at all in the current relayer flow since the relayer can't be given the channel capability here and then pass it to the next ChanOpenAck msg

Seems like enforcing capabilities will disallow bare creation of channels? Instead all channel creation would have to be mediated through the modules themselves which can bind ports and then start the channel handshake amongst themselves with users prompting the modules to move forward on the handshake

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like enforcing capabilities will disallow bare creation of channels? Instead all channel creation would have to be mediated through the modules themselves which can bind ports and then start the channel handshake amongst themselves with users prompting the modules to move forward on the handshake

This capability should be passed back to the module which has bound to the port - the module can then call ClaimCapability and use the capability to continue the handshake & send packets later on. The relayer can send the actual transactions; that's fine - the module just needs to claim the capability and fetch it (under some new name) when required.

Signer sdk.AccAddress `json:"signer"`
PortID string `json:"port_id"`
ChannelID string `json:"channel_id"`
PortCap capability.Capability `json:"port_capability"`
Copy link
Member Author

Choose a reason for hiding this comment

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

Putting capabilities in the user msgs here to make everything build, though I don't think it's possible for an end user outside of the state machine to ever pass in a capability correctly.

Either this functionality needs to be removed or it needs to somehow be routed first through the appropriate module that can inject the capability on behalf of the user

Copy link
Contributor

Choose a reason for hiding this comment

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

It needs to be routed through the module, see above comment.

@lgtm-com
Copy link

lgtm-com bot commented Mar 27, 2020

This pull request introduces 3 alerts when merging 06e7b80 into 3b48464 - view on LGTM.com

new alerts:

  • 3 for Useless assignment to local variable

@cwgoes
Copy link
Contributor

cwgoes commented Apr 6, 2020

@AdityaSripal Is this superseded by #5888?

@AdityaSripal
Copy link
Member Author

Closed in favor of #5888

@cwgoes cwgoes deleted the aditya/ics5 branch April 6, 2020 17:07
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.

Dynamic capability & routing implementation
3 participants