-
Notifications
You must be signed in to change notification settings - Fork 637
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
Add counterparty port ID to controller portID #319
Changes from 3 commits
126434b
37ed326
40d0d3f
0f89d3b
3a99d33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,18 +3,47 @@ package types | |
import ( | ||
"encoding/json" | ||
"fmt" | ||
"strings" | ||
|
||
yaml "gopkg.in/yaml.v2" | ||
|
||
crypto "github.com/cosmos/cosmos-sdk/crypto/types" | ||
sdk "github.com/cosmos/cosmos-sdk/types" | ||
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" | ||
authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" | ||
|
||
connectiontypes "github.com/cosmos/ibc-go/modules/core/03-connection/types" | ||
) | ||
|
||
const ( | ||
IcaPrefix string = "ics27-1-" | ||
ICAPrefix string = "ics-27" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the spec specifies two different formats: my preference is not to reference any version number in the ports, we should never have a reason to change the port ID generation as backwards compatibility/migration would be a pain, it'd be a different module at that point Either There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets go with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually just remembered why I kept the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't this be represented in the actual interchain accounts version? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's say we have a bunch of interchain accounts registered with version 1. If we upgrade the architecture to version 2 we have to either:
I imagined the first choice to be easier and considered using the version in the port-id as a future-proofing step if it's ever required. It's a minimal addition that may save time in the future. It probably will never come up, but if it does it could be useful. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a DAO registers an account on the hub with version 1 of interchain accounts, but version 2 (hypothetically) ended up having a different architecture, we would need to either migrate this account (ownership + any tokens) to version a version 2 account or keep supporting version 1. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The channel would still be accessible on both the host and controller side even if versions changed? My main hesitation is it seems odd to overload the port ID with information that could be contained in places it is designated for (app version in the channel version) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, you're saying if both sides update to version 2 of interchain accounts, it will be possible to get the version string that was originally used to create the channel, and that we can use that to distinguish between accounts registered under different versions? Getting very edge case here, but what would happen if the channel closed? Would we still be able to access the version string? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It depends on the structure of tracking the active channels. The channel version is always accessible even if the channel is closed. The active channel mapping would just need to know the previous channel which was active There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, let's go with this for now. Before we go live with ICA I think it's a good idea to have some plan in place for how we would handle this kind of issue if it arises. |
||
) | ||
|
||
// GeneratePortID generates the portID for a specific owner | ||
// on the controller chain in the format: | ||
// | ||
// 'ics-27-<connectionSequence>-<counterpartyConnectionSequence>-<owner-address>' | ||
// https://github.com/seantking/ibc/tree/sean/ics-27-updates/spec/app/ics-027-interchain-accounts#registering--controlling-flows | ||
// TODO: update link to spec | ||
// TODO: | ||
func GeneratePortID(owner, connectionID, counterpartyConnectionID string) (string, error) { | ||
ownerID := strings.TrimSpace(owner) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should consider enforcing limits on ownerID |
||
if ownerID == "" { | ||
return "", sdkerrors.Wrap(ErrInvalidOwnerAddress, "owner address cannot be empty") | ||
} | ||
connectionSeq, err := connectiontypes.ParseConnectionSequence(connectionID) | ||
if err != nil { | ||
return "", err | ||
} | ||
counterpartyConnectionSeq, err := connectiontypes.ParseConnectionSequence(counterpartyConnectionID) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
portID := fmt.Sprintf("%s-%d-%d-%s", ICAPrefix, connectionSeq, counterpartyConnectionSeq, ownerID) | ||
return portID, nil | ||
} | ||
|
||
type InterchainAccountI interface { | ||
authtypes.AccountI | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
package types_test | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/cosmos/ibc-go/modules/apps/27-interchain-accounts/types" | ||
channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types" | ||
ibctesting "github.com/cosmos/ibc-go/testing" | ||
"github.com/stretchr/testify/suite" | ||
) | ||
|
||
type TypesTestSuite struct { | ||
suite.Suite | ||
|
||
coordinator *ibctesting.Coordinator | ||
|
||
chainA *ibctesting.TestChain | ||
chainB *ibctesting.TestChain | ||
} | ||
|
||
func (suite *TypesTestSuite) SetupTest() { | ||
suite.coordinator = ibctesting.NewCoordinator(suite.T(), 2) | ||
|
||
suite.chainA = suite.coordinator.GetChain(ibctesting.GetChainID(0)) | ||
suite.chainB = suite.coordinator.GetChain(ibctesting.GetChainID(1)) | ||
} | ||
|
||
func NewICAPath(chainA, chainB *ibctesting.TestChain) *ibctesting.Path { | ||
path := ibctesting.NewPath(chainA, chainB) | ||
path.EndpointA.ChannelConfig.PortID = types.PortID | ||
path.EndpointB.ChannelConfig.PortID = types.PortID | ||
path.EndpointA.ChannelConfig.Order = channeltypes.ORDERED | ||
path.EndpointB.ChannelConfig.Order = channeltypes.ORDERED | ||
path.EndpointA.ChannelConfig.Version = types.Version | ||
path.EndpointB.ChannelConfig.Version = types.Version | ||
|
||
return path | ||
} | ||
|
||
func TestTypesTestSuite(t *testing.T) { | ||
suite.Run(t, new(TypesTestSuite)) | ||
} | ||
|
||
func (suite *TypesTestSuite) TestGeneratePortID() { | ||
var ( | ||
path *ibctesting.Path | ||
owner string | ||
) | ||
var testCases = []struct { | ||
name string | ||
malleate func() | ||
expValue string | ||
expPass bool | ||
}{ | ||
{"success", func() {}, "ics-27-0-0-owner123", true}, | ||
{"success with non matching connection sequences", func() { | ||
path.EndpointA.ConnectionID = "connection-1" | ||
}, "ics-27-1-0-owner123", true}, | ||
{"invalid owner address", func() { | ||
owner = " " | ||
}, "", false}, | ||
{"invalid connectionID", func() { | ||
path.EndpointA.ConnectionID = "connection" | ||
}, "", false}, | ||
{"invalid counterparty connectionID", func() { | ||
path.EndpointB.ConnectionID = "connection" | ||
}, "", false}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
tc := tc | ||
suite.Run(tc.name, func() { | ||
suite.SetupTest() // reset | ||
path = NewICAPath(suite.chainA, suite.chainB) | ||
suite.coordinator.Setup(path) | ||
owner = "owner123" // must be explicitly changed | ||
|
||
tc.malleate() | ||
|
||
portID, err := types.GeneratePortID(owner, path.EndpointA.ConnectionID, path.EndpointB.ConnectionID) | ||
if tc.expPass { | ||
suite.Require().NoError(err, tc.name) | ||
suite.Require().Equal(tc.expValue, portID) | ||
} else { | ||
suite.Require().Error(err, tc.name) | ||
suite.Require().Empty(portID) | ||
} | ||
}) | ||
} | ||
} |
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.
Id -> ID
Historically we always used
ID
. We only use Id on proto defintions because of an issue with gRPC gateway not allowingID
. All other variables names in core IBC useID