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

fix(orchestration): chain-info #9712

Merged
merged 5 commits into from
Jul 15, 2024
Merged

fix(orchestration): chain-info #9712

merged 5 commits into from
Jul 15, 2024

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented Jul 14, 2024

refs: #9063

Description

Fixes bugs in unreleased code related to publishing IBCConnectionInfo to vstorage and parameterizing ICA channel creation.

Testing Considerations

These were surfaced in E2E testing. Includes an additional test to verify connection info between two well known chains, as current snapshots are a bit hard to decipher.

Upgrade Considerations

Changes unreleased code.

- Add `reverseConnInfo` utility to flip connection info perspective
- Modify `registerChain` to normalize connection info before storage
- Ensure consistent storage based on lexicographically smaller chain ID
- properly specify hostConnectionId (remote chain) and controllerConnectionId (local chain) parameters
Copy link

cloudflare-workers-and-pages bot commented Jul 14, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 746261e
Status: ✅  Deploy successful!
Preview URL: https://31735579.agoric-sdk.pages.dev
Branch Preview URL: https://pc-fix-chain-info.agoric-sdk.pages.dev

View logs

@0xpatrickdev 0xpatrickdev added the force:integration Force integration tests to run on PR label Jul 14, 2024
@0xpatrickdev 0xpatrickdev marked this pull request as ready for review July 14, 2024 18:47
@0xpatrickdev 0xpatrickdev requested a review from turadg July 14, 2024 18:47
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Good stuff. It's coming together!

@@ -53,8 +53,8 @@ function toConnectionEntry(ibcInfo, name, chainInfo) {
}
const [channel] = transferChannels;
const [channelFrom, channelTo] = fromChain1
? [channel.chain_2, channel.chain_1]
: [channel.chain_1, channel.chain_2];
? [channel.chain_1, channel.chain_2]
Copy link
Member

Choose a reason for hiding this comment

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

in hindsight a unit test for the direction would have helped. not worthwhile now imo

@0xpatrickdev 0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Jul 15, 2024
turadg and others added 2 commits July 15, 2024 14:48
- updates multichain-e2e.yml job to print validator and relayer logs
@mergify mergify bot merged commit 31a57fc into master Jul 15, 2024
76 checks passed
@mergify mergify bot deleted the pc/fix-chain-info branch July 15, 2024 15:04
mergify bot added a commit that referenced this pull request Jul 15, 2024
_incidental_

## Description
Motivated by #9712 (comment)

This fixes up ChainHub to return IBCConnectionInfo in the perspective of the directed edge implied by the chain arguments.

The data is the same in either direction, just reversed, so we store it just once. This adds the logic to normalize on save and denormalize on read.

### Security Considerations
none

### Scaling Considerations
Some extra compute when saving or reading a chain. Low call rate.

### Documentation Considerations
none

### Testing Considerations
New tests

### Upgrade Considerations
Not yet deployed
@@ -70,31 +70,31 @@ Generated by [AVA](https://avajs.dev).
],
[
'published.agoricNames.chainConnection.agoric-3_cosmoshub-4',
'{"blockHeight":"0","values":["{\\"body\\":\\"{\\\\\\"client_id\\\\\\":\\\\\\"07-tendermint-6\\\\\\",\\\\\\"counterparty\\\\\\":{\\\\\\"client_id\\\\\\":\\\\\\"07-tendermint-927\\\\\\",\\\\\\"connection_id\\\\\\":\\\\\\"connection-649\\\\\\",\\\\\\"prefix\\\\\\":{\\\\\\"key_prefix\\\\\\":\\\\\\"FIXME\\\\\\"}},\\\\\\"id\\\\\\":\\\\\\"connection-8\\\\\\",\\\\\\"state\\\\\\":3,\\\\\\"transferChannel\\\\\\":{\\\\\\"channelId\\\\\\":\\\\\\"channel-405\\\\\\",\\\\\\"counterPartyChannelId\\\\\\":\\\\\\"channel-5\\\\\\",\\\\\\"counterPartyPortId\\\\\\":\\\\\\"transfer\\\\\\",\\\\\\"ordering\\\\\\":0,\\\\\\"portId\\\\\\":\\\\\\"transfer\\\\\\",\\\\\\"state\\\\\\":3,\\\\\\"version\\\\\\":\\\\\\"ics20-1\\\\\\"}}\\",\\"slots\\":[]}","{\\"body\\":\\"{\\\\\\"client_id\\\\\\":\\\\\\"07-tendermint-927\\\\\\",\\\\\\"counterparty\\\\\\":{\\\\\\"client_id\\\\\\":\\\\\\"07-tendermint-6\\\\\\",\\\\\\"connection_id\\\\\\":\\\\\\"connection-8\\\\\\",\\\\\\"prefix\\\\\\":{\\\\\\"key_prefix\\\\\\":\\\\\\"FIXME\\\\\\"}},\\\\\\"id\\\\\\":\\\\\\"connection-649\\\\\\",\\\\\\"state\\\\\\":3,\\\\\\"transferChannel\\\\\\":{\\\\\\"channelId\\\\\\":\\\\\\"channel-5\\\\\\",\\\\\\"counterPartyChannelId\\\\\\":\\\\\\"channel-405\\\\\\",\\\\\\"counterPartyPortId\\\\\\":\\\\\\"transfer\\\\\\",\\\\\\"ordering\\\\\\":0,\\\\\\"portId\\\\\\":\\\\\\"transfer\\\\\\",\\\\\\"state\\\\\\":3,\\\\\\"version\\\\\\":\\\\\\"ics20-1\\\\\\"}}\\",\\"slots\\":[]}"]}',
'{"blockHeight":"0","values":["{\\"body\\":\\"{\\\\\\"client_id\\\\\\":\\\\\\"07-tendermint-6\\\\\\",\\\\\\"counterparty\\\\\\":{\\\\\\"client_id\\\\\\":\\\\\\"07-tendermint-927\\\\\\",\\\\\\"connection_id\\\\\\":\\\\\\"connection-649\\\\\\",\\\\\\"prefix\\\\\\":{\\\\\\"key_prefix\\\\\\":\\\\\\"FIXME\\\\\\"}},\\\\\\"id\\\\\\":\\\\\\"connection-8\\\\\\",\\\\\\"state\\\\\\":3,\\\\\\"transferChannel\\\\\\":{\\\\\\"channelId\\\\\\":\\\\\\"channel-5\\\\\\",\\\\\\"counterPartyChannelId\\\\\\":\\\\\\"channel-405\\\\\\",\\\\\\"counterPartyPortId\\\\\\":\\\\\\"transfer\\\\\\",\\\\\\"ordering\\\\\\":0,\\\\\\"portId\\\\\\":\\\\\\"transfer\\\\\\",\\\\\\"state\\\\\\":3,\\\\\\"version\\\\\\":\\\\\\"ics20-1\\\\\\"}}\\",\\"slots\\":[]}","{\\"body\\":\\"{\\\\\\"client_id\\\\\\":\\\\\\"07-tendermint-6\\\\\\",\\\\\\"counterparty\\\\\\":{\\\\\\"client_id\\\\\\":\\\\\\"07-tendermint-927\\\\\\",\\\\\\"connection_id\\\\\\":\\\\\\"connection-649\\\\\\",\\\\\\"prefix\\\\\\":{\\\\\\"key_prefix\\\\\\":\\\\\\"\\\\\\"}},\\\\\\"id\\\\\\":\\\\\\"connection-8\\\\\\",\\\\\\"state\\\\\\":3,\\\\\\"transferChannel\\\\\\":{\\\\\\"channelId\\\\\\":\\\\\\"channel-5\\\\\\",\\\\\\"counterPartyChannelId\\\\\\":\\\\\\"channel-405\\\\\\",\\\\\\"counterPartyPortId\\\\\\":\\\\\\"transfer\\\\\\",\\\\\\"ordering\\\\\\":0,\\\\\\"portId\\\\\\":\\\\\\"transfer\\\\\\",\\\\\\"state\\\\\\":3,\\\\\\"version\\\\\\":\\\\\\"ics20-1\\\\\\"}}\\",\\"slots\\":[]}"]}',
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to decode these vstorage items for the snapshot.

Copy link
Member

Choose a reason for hiding this comment

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

feel free

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants