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

9068 close account e2e #9864

Merged
merged 6 commits into from
Aug 29, 2024
Merged

9068 close account e2e #9864

merged 6 commits into from
Aug 29, 2024

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented Aug 8, 2024

refs: #9068
refs: #9192

Description

  1. Includes e2e tests of different channel closing behaviors added in 9068 close account #9857:
  1. Includes publish local and remote ibc addresses to vstorage to facilitate querying the ICA account (channel) info from an off-chain client.
  • NOTE: if an account is a reopened, the localAddress and remoteAddress originally published to vstorage will be stale. Same with some of the values stored in CosmosOrchestrationAccountKit's exo state. The ChainAddress will be the same, but the full address strings will contain new channelIDs. This is tech debt being taken on until story: developers can observe the progress of their ICA transactions #9066.
  1. Includes test to ensure clients cannot successfully submit MsgChanCloseInit for ICA and Transfer channels

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

n/a

Testing Considerations

This PR includes high fidelity tests with simulated chains.

Upgrade Considerations

n/a

Copy link

cloudflare-workers-and-pages bot commented Aug 8, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: bb6d53d
Status: ✅  Deploy successful!
Preview URL: https://e18fab08.agoric-sdk.pages.dev
Branch Preview URL: https://9068-close-account-e2e.agoric-sdk.pages.dev

View logs

@0xpatrickdev 0xpatrickdev force-pushed the 9068-close-account branch 2 times, most recently from 1063d9b to e1fcfae Compare August 12, 2024 14:56
@dckc
Copy link
Member

dckc commented Aug 13, 2024

taking myself off the reviewer list to focus on other things. LMK if that's not a good idea, @turadg and @LuqiPan

@dckc dckc removed their request for review August 13, 2024 17:36
@0xpatrickdev 0xpatrickdev force-pushed the 9068-close-account branch 3 times, most recently from b808d43 to 440401d Compare August 16, 2024 19:27
@0xpatrickdev 0xpatrickdev force-pushed the 9068-close-account-e2e branch 2 times, most recently from 3e75a73 to a1b4f9a Compare August 19, 2024 18:15
@0xpatrickdev
Copy link
Member Author

0xpatrickdev commented Aug 19, 2024

I removed the "auto reopen" test scenario for now, as we need #9891 to complete it:

unintentionalCloseAccountScenario (auto reopen)
/** The channel is closed for an unexpected reason and should automatically reopen */
const unintentionalCloseAccountScenario = test.macro({
  title: (_, chainName: string) =>
    `Automatically reopen closed channel on ${chainName}`,
  exec: async (t, chainName: string) => {
    const config = chainConfig[chainName];
    if (!config) return t.fail(`Unknown chain: ${chainName}`);

    const {
      wallets,
      provisionSmartWallet,
      vstorageClient,
      retryUntilCondition,
      useChain,
      hermes,
    } = t.context;

    const agoricAddr = wallets[chainName];
    const wdUser1 = await provisionSmartWallet(agoricAddr, {
      BLD: 100n,
      IST: 100n,
    });
    t.log(`provisioning agoric smart wallet for ${agoricAddr}`);

    const doOffer = makeDoOffer(wdUser1);
    t.log(`${chainName} makeAccount offer`);
    const offerId = `${chainName}-makeAccount-${Date.now()}`;

    await doOffer({
      id: offerId,
      invitationSpec: {
        source: 'agoricContract',
        instancePath: [contractName],
        callPipe: [['makeOrchAccountInvitation']],
      },
      offerArgs: { chainName },
      proposal: {},
    });
    const currentWalletRecord = await retryUntilCondition(
      () => vstorageClient.queryData(`published.wallet.${agoricAddr}.current`),
      ({ offerToPublicSubscriberPaths }) =>
        Object.fromEntries(offerToPublicSubscriberPaths)[offerId],
      `${offerId} continuing invitation is in vstorage`,
    );
    const offerToPublicSubscriberMap = Object.fromEntries(
      currentWalletRecord.offerToPublicSubscriberPaths,
    );

    const accountStoragePath = offerToPublicSubscriberMap[offerId]?.account;
    t.assert(accountStoragePath, 'account storage path returned');
    const address = accountStoragePath.split('.').pop();
    t.log('Got address:', address);

    const {
      remoteAddress,
      localAddress,
    }: CosmosOrchestrationAccountStorageState =
      await vstorageClient.queryData(accountStoragePath);
    const { rPortID, rChannelID } = parseRemoteAddress(remoteAddress);
    const { lPortID, lChannelID, lConnectionID } =
      parseLocalAddress(localAddress);

    const dst = {
      chainId: chainInfo['agoric'].chainId,
      channelID: lChannelID,
      portID: lPortID,
      connectionID: lConnectionID,
    };
    const src = {
      chainId: useChain(chainName).chainInfo.chain.chain_id,
      channelID: rChannelID,
      portID: rPortID,
    };
    console.log(
      `Initiating channelCloseInit for dst: ${JSON.stringify(dst)} src: ${JSON.stringify(src)}`,
    );
    const closeChannelTx = hermes.channelCloseInit(chainName, dst, src);
    console.log('closeChannelExec', closeChannelTx);

    const remoteQueryClient = makeQueryClient(
      await useChain(chainName).getRestEndpoint(),
    );
    const { channel } = await retryUntilCondition(
      () => remoteQueryClient.queryChannel(rPortID, rChannelID),
      // @ts-expect-error ChannelSDKType.state is a string not a number
      ({ channel }) => channel?.state === 'STATE_CLOSED',
      'ICA channel closed from Hermes closeChannelInit',
      {
        retryIntervalMs: 300,
        maxRetries: 10,
      },
    );
    t.is(
      channel?.state,
      // @ts-expect-error ChannelSDKType.state is a string not a number
      'STATE_CLOSED',
      'closed state is observed',
    );

    const { channels } = await retryUntilCondition(
      () => remoteQueryClient.queryChannels(),
      // @ts-expect-error ChannelSDKType.state is a string not a number
      ({ channels }) => findNewChannel(channels, { rPortID, lPortID }),
      `ICA channel is reopened on ${chainName} Host`,
    );
    const newChannel = findNewChannel(channels, { rPortID, lPortID });
    t.log('New Channel after Reopen', newChannel);
    if (!newChannel) throw new Error('Channel not found');
    const newAddress = JSON.parse(newChannel.version).address;
    t.is(newAddress, address, `same chain address is returned - ${chainName}`);
    t.is(
      newChannel.state,
      // @ts-expect-error ChannelSDKType.state is a string not a number
      'STATE_OPEN',
      `channel is open on ${chainName} Host`,
    );
    t.not(newChannel.channel_id, rChannelID, 'remote channel id changed');
    t.not(
      newChannel.counterparty.channel_id,
      lChannelID,
      'local channel id changed',
    );
  },
});

test.serial(unintentionalCloseAccountScenario, 'cosmoshub');
test.serial(unintentionalCloseAccountScenario, 'osmosis');

This path is currently covered by unit tests with a buildChannelCloseConfirmEvent mock. The imperative .deactivate() and .reactivate() calls in this proposed E2E test also help firm up confidence around overall correctness.

Most importantly, reactivate()and its tests show that the same module account address is provided via a new channel. This indicates that user assets are recoverable in the event of an ICA channel closure.

@0xpatrickdev 0xpatrickdev force-pushed the 9068-close-account-e2e branch 2 times, most recently from ad1ab81 to 3692790 Compare August 19, 2024 19:14
@0xpatrickdev 0xpatrickdev requested review from turadg and removed request for turadg August 20, 2024 17:16
@0xpatrickdev 0xpatrickdev marked this pull request as ready for review August 20, 2024 17:22
@turadg turadg self-assigned this Aug 21, 2024
@0xpatrickdev 0xpatrickdev force-pushed the 9068-close-account branch 3 times, most recently from f04fcb7 to 89bae79 Compare August 27, 2024 18:47
mergify bot added a commit that referenced this pull request Aug 27, 2024
closes: #9068
closes: #9192

## Description
- enables `deactivate()` on `CosmosOrchestrationAccount` to close a channel (deactivate an account). Performs state cleanup (#9192) but preserves `Port` object and `ChainAddress` in case a holder wants to `reactivate()` the channel (account).
- adds `reactivate()` method to `IcaAccountKit` to allow a holder to reactivate an account (reestablish the connection with a new channel) they chose to `.deactivate()`. The original `Port` and `requestedRemoteAddress` are reused to ensure the same ChainAddress is assigned.
- adds logic to automatically reopen a channel that's closed for external factors (e.g. packet timeout) (#9068) and perform state cleanup (#9192)
- updates testing harness to ensure we can simulate ICA Channel Closure/Reopening in unit tests


### Security Considerations
n/a

### Scaling Considerations
n/a

### Documentation Considerations
Includes add'l JSDoc  + mermaid diagram updates

### Testing Considerations
Includes unit tests with high fidelity mocks from observed behavior on sim chains. See #9864 for e2e tests. 

### Upgrade Considerations
We are looking to land this in the initial release of `vat-orchestration`.
Base automatically changed from 9068-close-account to master August 27, 2024 19:52
@0xpatrickdev 0xpatrickdev assigned 0xpatrickdev and unassigned turadg Aug 27, 2024
@0xpatrickdev 0xpatrickdev added the force:integration Force integration tests to run on PR label Aug 27, 2024
@0xpatrickdev 0xpatrickdev requested review from turadg and michaelfig and removed request for michaelfig and turadg August 28, 2024 13:42
multichain-testing/test/basic-flows.test.ts Outdated Show resolved Hide resolved
multichain-testing/test/basic-flows.test.ts Outdated Show resolved Hide resolved
multichain-testing/test/ica-channel-close.test.ts Outdated Show resolved Hide resolved
Comment on lines +67 to +56
/** The account holder chooses to close their ICA account (channel) */
const intentionalCloseAccountScenario = test.macro({
title: (_, chainName: string) => `Close and reopen account on ${chainName}`,
Copy link
Member

Choose a reason for hiding this comment

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

exemplary macro

multichain-testing/tools/hermes-tools.ts Outdated Show resolved Hide resolved
- publishes remoteAddress (RemoteIBCAddress) and localAddress (LocalIBCAddress) to vstorage for CosmosOrchestrationAccount
- goal is to faciliate off-chain clients, which need portId, connectionId, and channelId for the host and controller to perform queries
- a better design might publish these values individually, versus putting the burden of parsing on the client
- refs: #9066
- helper functions for extracting connectionID, portID, and channelID from a negotiated (Local|Remote)IBCAddress string
- helper function to install a contract from a proposal builder
- queries vstorage and skips installation and CoreEval if instance is found
- CosmosOrchAccount (IcaAccout) holder can deactivate their account (close channel)
- CosmosOrchAccount (IcaAccount) holder can reactivate their account (open new channel w same portID)
- ensures clients cannot initiate channelCloseInit
@0xpatrickdev 0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label Aug 29, 2024
@mergify mergify bot merged commit 3573033 into master Aug 29, 2024
86 checks passed
@mergify mergify bot deleted the 9068-close-account-e2e branch August 29, 2024 15:21
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.

3 participants