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.

Sign up for GitHub

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

Jump to bottom

orchestration - ICA Controller - Account - Close Connection Lifecycle #9192

Closed
0xpatrickdev opened this issue Apr 4, 2024 · 6 comments · Fixed by #9857
Closed

orchestration - ICA Controller - Account - Close Connection Lifecycle #9192

0xpatrickdev opened this issue Apr 4, 2024 · 6 comments · Fixed by #9857
Assignees
@0xpatrickdev

Comments

@0xpatrickdev
Copy link
Member

0xpatrickdev commented Apr 4, 2024 •
edited
Loading

In the course of #9114, we realized we need to give more thought to Closing accounts.

  • Considerations
    • should we revoke ports?
    • should we attempt to retrieve assets before closing connections?
    • what contract state / storage should be cleaned up?

https://github.com/Agoric/agoric-sdk/pull/9114/files#r1550578668
@0xpatrickdev 0xpatrickdev mentioned this issue Apr 4, 2024
Merged
6 tasks
@0xpatrickdev
Copy link
Member Author

Note: it does not seem that localchain.js has an equivalent concept of closing currently

@aj-agoric aj-agoric assigned mitdralla and unassigned iomekam Jul 1, 2024
@0xpatrickdev
Copy link
Member Author

0xpatrickdev commented Jul 1, 2024 •
edited
Loading

Also to consider: connections can be closed from underneath us in some error cases (i think ordered channels are more susceptible to closing than unordered channels). In the onClose handler, we might consider adding logic that attempts to reinitialize the channel, via a new identifier (iow, recover the ICA account).

@0xpatrickdev
Copy link
Member Author

0xpatrickdev commented Jul 1, 2024 •
edited
Loading

Something tangential to consider: localchain.js / vat-localchain does not have a concept of closing an account. Should it?

@LuqiPan LuqiPan assigned 0xpatrickdev and unassigned mitdralla Aug 5, 2024
@LuqiPan
Copy link
Contributor

LuqiPan commented Aug 5, 2024

Re-assigning to @0xpatrickdev for design

0xpatrickdev added a commit that referenced this issue Aug 7, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
ee1c4d0 
- adds .reopen() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .close(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. This handler is only called when external factors
  force a channel closure (iow - if .close() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
0xpatrickdev added a commit that referenced this issue Aug 8, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
7444b2e 
- adds .reopen() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .close(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. This handler is only called when external factors
  force a channel closure (iow - if .close() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
0xpatrickdev added a commit that referenced this issue Aug 8, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
38b8834 
- adds .reopen() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .close(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. This handler is only called when external factors
  force a channel closure (iow - if .close() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
This was referenced Aug 8, 2024
Merged
Merged
0xpatrickdev added a commit that referenced this issue Aug 8, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
16dd28b 
- adds .reopen() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .close(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. This handler is only called when external factors
  force a channel closure (iow - if .close() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
0xpatrickdev added a commit that referenced this issue Aug 8, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
9cc6ae2 
- adds .reopen() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .close(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. This handler is only called when external factors
  force a channel closure (iow - if .close() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
@0xpatrickdev
Copy link
Member Author

I worked with @michaelfig and this how we plan to handle the questions:

should we revoke ports?

  • No, the cost for keeping them around doesn't seem high. Also, a governance action to recover an accidentally revoked port (that still has assets) seems costly.
  • The Port is held in the IcaAccountKit after a holder calls .close() and can be later used to .reopen() the connection.

should we attempt to retrieve assets before closing connections?

  • No, there are too many considerations here. IcaAccountKit knows nothing about the assets held on a remote chain. It also might hold non ICS-20 assets that aren't easily retrievable back to the controller chain.
  • The Port preservation and .reopen() method mentioned above shouldn't make asset recovery a concern.

what contract state / storage should be cleaned up?

  • We clean up Connection, RemoteIbcAddress, and LocalIbcAddress. When a connection is re-established with .reopen(), there will be a new Connection object and new RemoteIbcAddress and LocalIbcAddress values (by way of a new channelID).
  • The ChainAddress object and Port remotable are preserved in state.
  • IcaAccountKit doesn't currently write to vstorage, so no cleanup is done there.
  • ComosOrchestrationAccount does not do any cleanup yet. I think we should create a new ticket to track cleanup there.
    • (Tangentially - LocalChainAccount has no concept of .close())

@0xpatrickdev
Copy link
Member Author

  • IcaAccountKit doesn't currently write to vstorage, so no cleanup is done there.
  • ComosOrchestrationAccount does not do any cleanup yet. I think we should create a new ticket to track cleanup there.

At least the first item here is dependent on #9066. I think #9587 will mark this closed if these items are tracked in a separate ticket (they seem out of scope to "ICA Controller", anyways, but this ticket was written some time ago) cc @mitdralla @LuqiPan

0xpatrickdev added a commit that referenced this issue Aug 9, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
b51c746 
- adds .reopen() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .close(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. This handler is only called when external factors
  force a channel closure (iow - if .close() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
0xpatrickdev added a commit that referenced this issue Aug 12, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
64c6e85 
- adds .reopen() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .close(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. This handler is only called when external factors
  force a channel closure (iow - if .close() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
0xpatrickdev added a commit that referenced this issue Aug 16, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
91ff040 
- adds .reopen() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .close(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. `reopen() will only fire when external factors
  force a channel closure (iow - if .close() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
0xpatrickdev added a commit that referenced this issue Aug 16, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
b808d43 
- adds .reopen() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .close(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. `reopen() will only fire when external factors
  force a channel closure (iow - if .close() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
0xpatrickdev added a commit that referenced this issue Aug 16, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
440401d 
- adds .reactivate() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .deactivate(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. `reactivate() will only fire when external factors
  force a channel closure (iow - if .deactivate() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
0xpatrickdev added a commit that referenced this issue Aug 16, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
b18263a 
- adds .reactivate() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .deactivate(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. `reactivate() will only fire when external factors
  force a channel closure (iow - if .deactivate() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
0xpatrickdev added a commit that referenced this issue Aug 21, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
55b79ec 
- adds .reactivate() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .deactivate(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. `reactivate() will only fire when external factors
  force a channel closure (iow - if .deactivate() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
0xpatrickdev added a commit that referenced this issue Aug 27, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
608a4bb 
- adds .reactivate() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .deactivate(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. `reactivate() will only fire when external factors
  force a channel closure (iow - if .deactivate() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
0xpatrickdev added a commit that referenced this issue Aug 27, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
2394575 
- adds .reactivate() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .deactivate(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. `reactivate() will only fire when external factors
  force a channel closure (iow - if .deactivate() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
0xpatrickdev added a commit that referenced this issue Aug 27, 2024
@0xpatrickdev
feat: close, reopen, and automatically reopen ICA channels
08d8e51 
- adds .reactivate() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .deactivate(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. `reactivate() will only fire when external factors
  force a channel closure (iow - if .deactivate() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
@mergify mergify bot closed this as completed in #9857 Aug 27, 2024
@mergify mergify bot closed this as completed in d9700fb Aug 27, 2024
kriskowal pushed a commit that referenced this issue Aug 27, 2024
@0xpatrickdev @kriskowal
feat: close, reopen, and automatically reopen ICA channels
b72ebcc 
- adds .reactivate() method to IcaAccountKit['holder'] to re-establish an ICA channel using the original requestedRemoteAddress. it's
  intended to be used internally to automatically re-establish a channel, but also exposed to holders if they wish to re-open their
  account after calling .deactivate(). includes corresponding 'ReopenAccount' invitationMaker on CosmosOrchestrationAccount.
- performs cleanup after a connection is closed - namely resetting localAddr, remoteAddr, and connection in state. chainAddress and
  port are preserved.
- adds logic to onClose() handler to automatically re-establish the ICA channel. `reactivate() will only fire when external factors
  force a channel closure (iow - if .deactivate() is called by the holder, this will not fire)
- updates network-fakes.ts to cache mockChainAddresses based on PortID:ConnectionID to mimic ICS-27 protocol

- refs: #9192
- refs: #9068
mergify bot added a commit that referenced this issue Aug 29, 2024
@mergify
9068 close account e2e (#9864)
3573033 
refs: #9068
refs: #9192

## Description
1. Includes e2e tests of different channel closing behaviors added in #9857:
 - ~~Automatically reopen a closed ICA channel (e.g. a packet timed out)~~ requires #9891 to complete. See  #9864 (comment)
 - `CosmosOrchestrationAccount` (`IcaAccout`) holder can close their account
 - `CosmosOrchestrationAccount` (`IcaAccount`) holder can reopen their account

2. 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 #9066.
  
3. 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@0xpatrickdev 0xpatrickdev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

9068 close account Agoric/agoric-sdk
4 participants
@mitdralla @LuqiPan @iomekam @0xpatrickdev