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

Refactor connection/channel identifier handling #329

Closed
colin-axner opened this issue Nov 24, 2020 · 3 comments · Fixed by #334
Closed

Refactor connection/channel identifier handling #329

colin-axner opened this issue Nov 24, 2020 · 3 comments · Fixed by #334
Assignees

Comments

@colin-axner
Copy link
Contributor

address significant changes

I will handle this since the changes are non-trivial

@colin-axner colin-axner added this to the 1.0 IBC milestone Nov 24, 2020
@colin-axner colin-axner self-assigned this Nov 24, 2020
@colin-axner
Copy link
Contributor Author

colin-axner commented Nov 25, 2020

@AdityaSripal and I discussed a potential architecture refactor. The scope of the following relates only to the client/connection/channel stack starting from a config and state based relaying and ending once the config is full and the client is created, the connections are open and the channels are open.

We can envision the relayer stack having two core components, client and the core relayer. From the client level we want to allow partially filled config or fully filled config files to be passed to the core relayer.

We will need to modify path end validation and path validation to allow certain fields to be optionally left empty. These fields should include clientID counterpartyClientID connectionID counterpartyConnectionID channelID and counterpartyChannelID. Validation should include bottom up logic. If the connectionID is filled then the clientIDs must be filled and the same is true for channels. If the channelIDs are filled then the client and connection IDs must be filled.

The fields left empty and the current state of filled in field determines what handshake step or client creation step to begin at and continue from.

After path validation is modified. We can assume the case where the user fills in the minimal possible amount of fields begins by creating a client on both chains. This code will largely remain the same.

  • If the clientID is empty it creates a client on source
  • If the clientID is filled it creates a client on source if it does not exist
  • apply the same two conditions for counterparty client id and the destination chain

If the clientID or counterpartyClientID is empty then after creation of the client, write the identifier to the config.

Once clients are created we can move to the connection handshake. The logic will be as follows:

if connectionID == "" && counterpartyConnectionID == "" {
   // case OpenInit on source
}
if connectionID == "" && counterpartyConnectionID != "" {
    // case OpenTry  on source
}
if connectionID != "" && counterpartyConnectionID == "" {
   // case OpenTry on counterparty
}
// otherwise continue based on the states of the connection handshakes
// this should be the same as the existing code

Since connection identifiers need to be updated after the corresponding step, we propose modifying the CreateConnectionSteps to execute the messages as it creates it rather than returning the relayMsgs struct.

case OpenInit on source:
    // create open init message
    // create tx with open init msg and update msg
    // execute message 
    // if msg was successful, parse the events in the open init message and set the connecitonID

// etc

case OpenTry on destination:
    // create open try message
    // create tx with open try msg and update msg
    // execute message 
    // if msg was successful, parse the events in the open try message and set the counterpartyConnectionID

// etc

// end of create connections function
// return if the tx was successful and if this was the last step

The function that calls the create connections function should only handle the looping logic related to keeping a failed counter and retrying the step. We want to remove the message execution from this step since it isolates the logic necessary to respond to message specific logic embedded in events.

The connection logic will be duplicated for channels. It should basically be identical but calling channel specific functions.

Once clients are created, connections are open and channels are open a boolean should be set to true in the config indicating that the relayer is ready to begin relaying packets and that paths should be validation fully.

This proposal reduces UX complexity for the user, allows us to refactor to support the lastest SDK changes and do so in a minimal fashion. The only code changes will largely be to path validation, connection handshake and channel handshake handling (not construction).

I plan to start sketching some of this work in parallel with the typed events work specifically because this refactor only relies on having access to a function ParseOpenInitEvents which could use typed events or a brute force parsing (in the case typed events isn't merged before a stargate release is cut)

cc @jackzampolin

@AdityaSripal
Copy link
Member

AdityaSripal commented Nov 25, 2020

Thanks for the write-up @colin-axner . Note channel ORDER and port IDS must always be specified by user.

If a user does specify a connection/channel ID, these must be IDs that were already generated by the respective chains. So we must check that these IDs not only exist, but they are also consistent with the rest of the config.
If a user specifies connection IDs, we must check that connectionID exists AND the connection is actually using the underlying clients specified in the config.
Similarly with channelIDs, they must exist and reference the underlying connections in the config.

Even if we get a fully fleshed out config from the user, we should still go through the config and validate that it is consistent. Ie, the specified channels are built on top of specified connections, which are built on top of specified clients. (This should be happening even with the current architecture, though I'm not sure if it is checked or not).

Since the relayer's Chain struct can now have an incomplete PathEnd, we should have a fullyConfigured boolean flag that is set after the handshake is complete and the config is verified and written. Once this flag is set, the Chain can start being used to relay msgs

@jackzampolin
Copy link
Member

jackzampolin commented Nov 25, 2020

One thing I'm thinking now is that we don't expose users to client, connection or channel identifiers at all and the Path struct could look like the following now:

# go
type Path struct {
    Src PathEnd
    Dst PathEnd
}

type PathEnd struct {
    ChainID string
    PortID string
    Order string
}
# json
{
    "src": {
        "chain-id": "ibc0",
        "port-id: "transfer",
        "order": "UNORDERED",
    }
    "dst": {
        "chain-id": "ibc0",
        "port-id: "transfer",
        "order": "UNORDERED",
    }
}

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 a pull request may close this issue.

3 participants