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

story: developers can observe the progress of their ICA transactions #9066

Open
2 tasks
ivanlei opened this issue Mar 11, 2024 · 11 comments
Open
2 tasks

story: developers can observe the progress of their ICA transactions #9066

ivanlei opened this issue Mar 11, 2024 · 11 comments
Assignees
Labels
enhancement New feature or request needs-design

Comments

@ivanlei
Copy link
Contributor

ivanlei commented Mar 11, 2024

What is the Problem Being Solved?

#9065 details the ability to send ICA commands over an ICA account object.

This enhances that capability by logging well-structured information to a subtree of the contract's storage node that allows for following TX progress and debugging. This universal logging brings uniformity to error handling, debuggability, & TX recovery.

Description of the Design

current draft:

  • each asyncFlow/orchestration has an ID, which is used as a root for vstorage records of progress
  • orchestration publishes to an associated subscription underneath that key the current interesting operation in progress and it’s info (e.g., what channel and txn ID at the IBC level?)
  • there is a record associating the async flow with the walletSpendAction
  • in the case of continuing offers, that should tie the user’s action to the base object (eg.., the Portfolio)

Tasks

Security Considerations

Scaling Considerations

Test Plan

Upgrade Considerations

@ivanlei ivanlei added the enhancement New feature or request label Mar 11, 2024
@rowgraus
Copy link

Questions coming out of planning:

  1. What actions should be automatically stored?
  2. How do we want developers to specify additional information
  3. What information do users require in order to track their own transaction
  4. First user story

@turadg turadg self-assigned this Apr 9, 2024
@LuqiPan
Copy link
Contributor

LuqiPan commented Apr 30, 2024

From security's perspective, should we be worried about this being another factor that contributes to vstorage(hence disk) growth?

@turadg
Copy link
Member

turadg commented Apr 30, 2024

contributes to vstorage(hence disk) growth?

A concern, yes. It should not be exploitable because it's managed by the Orchestration API. The API can also delete old data so it doesn't persist in IAVL.

@dtribble
Copy link
Member

contributes to vstorage(hence disk) growth?

A concern, yes. It should not be exploitable because it's managed by the Orchestration API. The API can also delete old data so it doesn't persist in IAVL.

Indeed that's a motivator for having system-provided txn logging with sufficient info for most purposes

@dckc
Copy link
Member

dckc commented May 24, 2024

  • each asyncFlow/orchestration has an ID, which is used as a root for vstorage records of progress

Is the id scoped to the contract? (in particular: to the orchestration facade?)

It's not supposed to be global, right?

@dckc
Copy link
Member

dckc commented May 24, 2024

  • there is a record associating the async flow with the walletSpendAction

It's straightforward to point from a wallet's vstorage to a key of the contract's choosing (using offerToPublicSubscriberPaths).

Going the other way doesn't seem feasible. We don't need to go the other way, do we?

mergify bot added a commit that referenced this issue Jun 17, 2024
refs: #9042 
refs: #9193
refs: #9066

## Description

- **Motivation:** wallet clients need a way to view their address so they can send funds to it
- Updates `stakeAtom.contract.js` to create a vstorage entry for newly created accounts, keyed by `ChainAccount['address']` (e.g. `cosmos1testing1234`)
   - TODO: if we hit `UNPARSABLE_ADDRESS` in parseAddress we should throw, or use a fallback value
- Writes an empty string  to provided storageNode in `exos/cosmosOrchestrationAccount.js`
   - adds TODO for fleshing out vstorage requirements via #9066
  
 - Drive-by fixes:
    - use `AmountArg` consistently in `/exos/cosmosOrchestrationAccount.js`
    - remove `delegatorAddress` from `undelegate(Delegations[])` interface and implementation

### Security Considerations


### Scaling Considerations

This is some experimentation within a contract in order to get experience with what should be pushed down into the platform.

The approach delegates vstorage publishing responsibilities to guest contracts. We might consider posting information for all accounts to a shared global namespace.

### Documentation Considerations


### Testing Considerations

Tests, including ones for `swapExample.contract.js` and `facade.js`, were updated to accommodate these changes. Mocking facilities for `.getAddress()` are light - including in boostrap tests - so using `ChainAddress['address']` might lead to unexpected behavior (accounts overwriting each other in storage) in a testing environment.

### Upgrade Considerations
@aj-agoric aj-agoric assigned mitdralla and unassigned turadg Jul 1, 2024
0xpatrickdev added a commit that referenced this issue Jul 10, 2024
- previously, each orch account was given the contracts storage node. this creates a child node per account
- refs #9066, helping ensure orch account addresses are available for smart-wallet clients. left some suggestions in code comments for further improvements
0xpatrickdev added a commit that referenced this issue Jul 10, 2024
- previously, each orch account was given the contracts storage node. this creates a child node per account
- refs #9066, helping ensure orch account addresses are available for smart-wallet clients. left some suggestions in code comments for further improvements
0xpatrickdev added a commit that referenced this issue Jul 10, 2024
- previously, each orch account was given the contracts storage node. this creates a child node per account
- refs #9066, helping ensure orch account addresses are available for smart-wallet clients. left some suggestions in code comments for further improvements
@0xpatrickdev
Copy link
Member

When an ICA message lands on another chain, or any cross-chain ibc message for that matter, the following transaction shows up: MsgRecvPacket

On MsgRecvPacket, we have Packet.

Notably, the primary way to filter transactions on a remote chain is by using the following fields:

// identifies the port on the sending chain.
string source_port = 2;
// identifies the channel end on the sending chain.
string source_channel = 3;
// identifies the port on the receiving chain.
string destination_port = 4;
// identifies the channel end on the receiving chain.
string destination_channel = 5;

This seems to suggest we want to make these values available in vstroage, in addition to the account address we are currently publishing.

0xpatrickdev added a commit that referenced this issue Jul 10, 2024
- previously, each orch account was given the contracts storage node. this creates a child node per account
- refs #9066, helping ensure orch account addresses are available for smart-wallet clients. left some suggestions in code comments for further improvements
0xpatrickdev added a commit that referenced this issue Jul 10, 2024
- previously, each orch account was given the contracts storage node. this creates a child node per account
- refs #9066, helping ensure orch account addresses are available for smart-wallet clients. left some suggestions in code comments for further improvements
mergify bot added a commit that referenced this issue Jul 11, 2024
closes: #9673
refs: #9066
refs: #9042 

## Description
- Adds `basic-flow.contract.js`, proposal, and builder for testing `orchestrate` (`async-flow`) continuing invitations to facilitate testing. These are also the first bootstrap + multichain tests with `orchestrate` / `async-flow`.
 - Updates `asContinuingInvitation` and `getPublicTopics` methods to return Vows to satisfy membrane constraints
    - Ensures `storagePath`, part of `ContinuingOfferResult` is a string instead of a Promise/Vow so `smart-wallet` can easily handle this and the `async-flow` membrane is appeased (no promises)
- Updates chain facade implementations, ensuring each account has it's own storage node (towards #9066)

### Testing Considerations
Includes unit, bootstrap, and multichain (e2e) tests for an async-flow contract. E2E ensure accounts can be created on agoric, cosmos, and osmosis and return continuing invitations.

### Upgrade Considerations
This did not require any changes to smart-wallet. There is a change to `PublicTopicShape` to accept a promise or a string (previously just a promise), but this is only in contracts relaying on smart-wallet, not smart-wallet itself.
@0xpatrickdev
Copy link
Member

Until #9066, every interchain action has been triggered by a wallet offer. This meant that the actions taken by the LCA were clearly documented in published.wallet.
For scenarios where external triggers initiate interchain transactions, it seems it would be helpful to log something this vstorage to figure out something happened. This might suggest we want do vstorage writes when tx's are completed (and initiated).

@dckc
Copy link
Member

dckc commented Jul 16, 2024

Until #9066, every interchain action has been triggered by a wallet offer. This meant that the actions taken by the LCA were clearly documented in published.wallet. ...

There's a pattern where, for example, the vaultFactory publishes info about vaults and the wallet points to those parts of vstorage by path. It's used in watchUserVaults in dapp-inter.

Likewise, the account / tx info could be canonically published by the contract, and the wallet could get a pointer.

mergify bot added a commit that referenced this issue Jul 17, 2024
refs: #9042
refs: #9066
refs: #9193

## Description
- Enhances `LocalOrchestrationAccount` with `monitorTransfers`(vtransfer)  method to register handlers for incoming and outgoing ICS20 transfers.
  -   `writeAcknowledgement` (ability to confer acknowledgement or acknowledgement error to sending chain) capability is not exposed through the current orchestration api. users must work with `registerActiveTap` from `transfer.js` for this capability.
- Implements `auto-stake-it` contract that uses .monitorTransfers to react to incoming IBC transfers, delegating them via an InterchainAccount (ICA).
   - referred to as "stakeAtom" on the ticket, but this seemed like a better name. not to be confused with _restaking  (autocompounding rewards)_
- Introduces `PortfolioHolder` kit, combining `ContinuingOfferResults` from multiple `OrchestrationAccounts` into a single record.
- Adds VTransferIBCEvent type for `acknowledgementPacket` and `writeAcknowledgement` events and an example mock for unit testing

### Documentation
- Aims to improve documentation around vtransfer, monitorTransfers, registerTap, etc.

### Testing Considerations
- Includes unit tests that inspect bridge messages to observe relevant transactions firing. 
- Includes multichain test demonstrating the flow from #9042, f.k.a. "stakeAtom"
0xpatrickdev added a commit that referenced this issue Jul 31, 2024
- whether an address is parsable or not is not a consideration for the vstorage story (#9066)
- instead, this should be interpreted as an unexpected error if this path is reached
- throwing here would not solve much - this is a notifier divorced from the callsite. instead,
- the caller will experience an error when they try and use UNPARSABLE_CHAIN_ADDRESS
0xpatrickdev added a commit that referenced this issue Jul 31, 2024
- whether an address is parsable or not is not a consideration for the vstorage story (#9066)
- instead, this should be interpreted as an unexpected error if this path is reached
- throwing here would not solve much - this is a notifier divorced from the callsite. instead,
- the caller will experience an error when they try and use UNPARSABLE_CHAIN_ADDRESS
0xpatrickdev added a commit that referenced this issue Jul 31, 2024
- whether an address is parsable or not is not a consideration for the vstorage story (#9066)
- instead, this should be interpreted as an unexpected error if this path is reached
- throwing here would not solve much - this is a notifier divorced from the callsite. instead,
- the caller will experience an error when they try and use UNPARSABLE_CHAIN_ADDRESS
0xpatrickdev added a commit that referenced this issue Jul 31, 2024
- whether an address is parsable or not is not a consideration for the vstorage story (#9066)
- instead, this should be interpreted as an unexpected error if this path is reached
- throwing here would not solve much - this is a notifier divorced from the callsite. instead,
- the caller will experience an error when they try and use UNPARSABLE_CHAIN_ADDRESS
0xpatrickdev added a commit that referenced this issue Jul 31, 2024
- whether an address is parsable or not is not a consideration for the vstorage story (#9066)
- instead, this should be interpreted as an unexpected error if this path is reached
- throwing here would not solve much - this is a notifier divorced from the callsite. instead,
- the caller will experience an error when they try and use UNPARSABLE_CHAIN_ADDRESS
0xpatrickdev added a commit that referenced this issue Jul 31, 2024
- whether an address is parsable or not is not a consideration for the vstorage story (#9066)
- instead, this should be interpreted as an unexpected error if this path is reached
- throwing here would not solve much - this is a notifier divorced from the callsite. instead,
- the caller will experience an error when they try and use UNPARSABLE_CHAIN_ADDRESS
0xpatrickdev added a commit that referenced this issue Jul 31, 2024
- whether an address is parsable or not is not a consideration for the vstorage story (#9066)
- instead, this should be interpreted as an unexpected error if this path is reached
- throwing here would not solve much - this is a notifier divorced from the callsite. instead,
- the caller will experience an error when they try and use UNPARSABLE_CHAIN_ADDRESS
0xpatrickdev added a commit that referenced this issue Aug 7, 2024
- 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
0xpatrickdev added a commit that referenced this issue Aug 7, 2024
- 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
0xpatrickdev added a commit that referenced this issue Aug 7, 2024
- 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
0xpatrickdev added a commit that referenced this issue Aug 7, 2024
- 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
0xpatrickdev added a commit that referenced this issue Aug 7, 2024
- 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
@0xpatrickdev
Copy link
Member

Notes from talking with @michaelfig -

  • it might be a good idea to post this information from vat-orchestration or vat-ibc on behalf of all consumers
  • we should consider using srcPortID + srcConnectionID for the key instead of ChainAddress.value
    • the namespace we might consider is published.orchestration.ica
    • for the interim, while we figure out the schema, we might want to call this published.orchestration.debug.ica. And we can delete the node when we are done
    • we need to this about how to do this for LCA's, although only the address is relevant there
  • this information can change over time - specifically, if a channel is closed and reopened there will be a new channelID (portID, connectionID and ChainAddress should remain the same)
image

0xpatrickdev added a commit that referenced this issue Aug 8, 2024
- 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
0xpatrickdev added a commit that referenced this issue Aug 8, 2024
- 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
0xpatrickdev added a commit that referenced this issue Aug 8, 2024
- 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
0xpatrickdev added a commit that referenced this issue Aug 16, 2024
- 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
0xpatrickdev added a commit that referenced this issue Aug 16, 2024
- 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
0xpatrickdev added a commit that referenced this issue Aug 21, 2024
- 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
0xpatrickdev added a commit that referenced this issue Aug 27, 2024
- 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
kriskowal pushed a commit that referenced this issue Aug 27, 2024
- whether an address is parsable or not is not a consideration for the vstorage story (#9066)
- instead, this should be interpreted as an unexpected error if this path is reached
- throwing here would not solve much - this is a notifier divorced from the callsite. instead,
- the caller will experience an error when they try and use UNPARSABLE_CHAIN_ADDRESS
0xpatrickdev added a commit that referenced this issue Aug 29, 2024
- 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
mergify bot added a commit that referenced this issue Aug 29, 2024
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
@mhofman
Copy link
Member

mhofman commented Sep 18, 2024

  • there is a record associating the async flow with the walletSpendAction

Are we talking about async flow definition or async flow invocation here? I suppose the latter since there are many walletSpendAction each potentially triggering multiple (but more likely just one) invocation of a given async flow definition.

@turadg turadg changed the title orchestration - ICA Controller - Account - vstorage logging story: developers can observe the progress of their ICA transactions Oct 7, 2024
@turadg turadg assigned LuqiPan and unassigned mitdralla Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-design
Projects
None yet
Development

No branches or pull requests

9 participants