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

orchestration - ICA Controller - deposit payment to ICA #9193

Closed
1 task done
0xpatrickdev opened this issue Apr 4, 2024 · 3 comments · Fixed by #10211
Closed
1 task done

orchestration - ICA Controller - deposit payment to ICA #9193

0xpatrickdev opened this issue Apr 4, 2024 · 3 comments · Fixed by #10211
Assignees
Labels
enhancement New feature or request

Comments

@0xpatrickdev
Copy link
Member

0xpatrickdev commented Apr 4, 2024

  1. As new user of an orchestration contract, I need to “deposit” (move) my fungible funds into a place controlled by orchestration so it can do things with it.

    • for assets starting on Agoric in a smart-wallet

      • Only chains available through vbank (by BLD stakers)
    • for assets starting on a remote Cosmos chain [into an agoric-controlled-ICA on the same remote chain]

  2. As a user of an orchestration contract, I need to withdraw assets from a LocalChain Account and ICA into my smart wallet

Tasks

  1. enhancement
    0xpatrickdev
@0xpatrickdev
Copy link
Member Author

0xpatrickdev commented Apr 4, 2024

The way I currently imagine this might work - is with an LCA and IBC Transfer:

  • E(chainAccount).deposit(payment)
  • ChainAccount sees if a LocalChainAccount is available in state. If not, one is requested.
  • E(LocalChainAccount).deposit(pmt)
  • E(LocalChainAccount).transfer(chainAccountAddr, amount)

@dckc
Copy link
Member

dckc commented Apr 4, 2024

this only works for brands that are registered in the vbank by BLD staker governance.
Does that suffice?

   const allegedPurse = E(bankAcct).getPurse(allegedBrand); 

p.s. see #9211

@dckc
Copy link
Member

dckc commented Apr 4, 2024

(tangential...)

   const allegedPurse = E(bankAcct).getPurse(allegedBrand); 

"alleged" seems misused there. The caller relies on the bankManager, right? it's not in a position to be suspicious of it. Seems like the result of getPurse has to be presumed correct.

mergify bot added a commit that referenced this issue Apr 25, 2024
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

<!-- Most PRs should close a specific Issue. All PRs should at least
reference one or more Issues. Edit and/or delete the following lines as
appropriate (note: you don't need both `refs` and `closes` for the same
one): -->

Like #9284, but also adds `ibc` protos from `ibc-go/v6`. Motivated by
needing `MsgTransfer` for #9193.

closes: #XXXX
refs: #9193

## Description

<!-- Add a description of the changes that this PR introduces and the
files that
are the most critical to review.
-->

### Security Considerations

<!-- Does this change introduce new assumptions or dependencies that, if
violated, could introduce security vulnerabilities? How does this PR
change the boundaries between mutually-suspicious components? What new
authorities are introduced by this change, perhaps by new API calls?
-->

### Scaling Considerations

<!-- Does this change require or encourage significant increase in
consumption of CPU cycles, RAM, on-chain storage, message exchanges, or
other scarce resources? If so, can that be prevented or mitigated? -->

### Documentation Considerations

<!-- Give our docs folks some hints about what needs to be described to
downstream users.

Backwards compatibility: what happens to existing data or deployments
when this code is shipped? Do we need to instruct users to do something
to upgrade their saved data? If there is no upgrade path possible, how
bad will that be for users?

-->

### Testing Considerations

<!-- Every PR should of course come with tests of its own functionality.
What additional tests are still needed beyond those unit tests? How does
this affect CI, other test automation, or the testnet?
-->

Added `ibc` to the snapshot tests for this package.

### Upgrade Considerations

<!-- What aspects of this PR are relevant to upgrading live production
systems, and how should they be addressed? -->

Unlike the `COSMOS_SDK` variable in `update-protos.sh`, `IBC_GO`
contains a version (v6) in the path which may require someone to update
the script when the `cosmos/ibc-go` version is bumped. If someone wants
to suggest a different approach that uses regex, that's more than
welcome.
@0xpatrickdev 0xpatrickdev added enhancement New feature or request and removed needs-design labels May 3, 2024
mergify bot added a commit that referenced this issue May 10, 2024
refs: #9342, #9193
stacked on

 - #9353

## Description

Provide localchain accounts only with the `bank` for the relevant
address, rather than giving them access to all accounts in the form of
the `bankManager`.

earlier discussion:
https://github.com/Agoric/agoric-sdk/pull/9342/files#r1594791187

### Security Considerations

bankManager was excess authority

### Scaling / Documentation / Testing Considerations

Nothing significant: no scaling changes; existing docs/tests suffice.

### Upgrade Considerations

code is not released / deployed
mergify bot added a commit that referenced this issue May 22, 2024
refs: #9193

## Description

Adds `.withdraw()` method to `LocalChainAccount` to satisfy:

As a user of an orchestration contract, I need to withdraw assets from a
LocalChain Account into my smart wallet

### Security Considerations


### Scaling Considerations


### Documentation Considerations


### Testing Considerations


### Upgrade Considerations
mergify bot added a commit that referenced this issue May 24, 2024
#9380)

refs: #9193

## Description

Adds `.transfer()` method to `LocalChainAccountKit`, working towards
`OrchestrationAccountI['transfer']` interface.


### Security Considerations

<!-- Does this change introduce new assumptions or dependencies that, if
violated, could introduce security vulnerabilities? How does this PR
change the boundaries between mutually-suspicious components? What new
authorities are introduced by this change, perhaps by new API calls?
-->

### Scaling Considerations

<!-- Does this change require or encourage significant increase in
consumption of CPU cycles, RAM, on-chain storage, message exchanges, or
other scarce resources? If so, can that be prevented or mitigated? -->

### Documentation Considerations

<!-- Give our docs folks some hints about what needs to be described to
downstream users.

Backwards compatibility: what happens to existing data or deployments
when this code is shipped? Do we need to instruct users to do something
to upgrade their saved data? If there is no upgrade path possible, how
bad will that be for users?

-->

### Testing Considerations

<!-- Every PR should of course come with tests of its own functionality.
What additional tests are still needed beyond those unit tests? How does
this affect CI, other test automation, or the testnet?
-->

### Upgrade Considerations

<!-- What aspects of this PR are relevant to upgrading live production
systems, and how should they be addressed? -->
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
0xpatrickdev added a commit that referenced this issue Jul 2, 2024
- removes .deposit() from CosmosOrchestrationAccount as #9193 stipulates this will be done via an LCA
- adds LocalAccountMethods type, although unused, for documenatation purposes. and, eventually, to support #9212
0xpatrickdev added a commit that referenced this issue Jul 3, 2024
- removes .deposit() from CosmosOrchestrationAccount as #9193 stipulates this will be done via an LCA
- adds LocalAccountMethods type, although unused, for documenatation purposes. and, eventually, to support #9212
0xpatrickdev added a commit that referenced this issue Jul 3, 2024
- removes .deposit() from CosmosOrchestrationAccount as #9193 stipulates this will be done via an LCA
- adds LocalAccountMethods type, although unused, for documenatation purposes. and, eventually, to support #9212
0xpatrickdev added a commit that referenced this issue Jul 3, 2024
- removes .deposit() from CosmosOrchestrationAccount as #9193 stipulates this will be done via an LCA
- adds LocalAccountMethods type, although unused, for documenatation purposes. and, eventually, to support #9212
0xpatrickdev added a commit that referenced this issue Jul 3, 2024
- removes .deposit() from CosmosOrchestrationAccount as #9193 stipulates this will be done via an LCA
- adds LocalAccountMethods type, although unused, for documenatation purposes. and, eventually, to support #9212
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"
mergify bot added a commit that referenced this issue Aug 9, 2024
#9870)

refs: #9193

## Description

Implements `.send()` and `.sendAll()` methods on CosmosOrchestrationAccount and LocalOrchestrationAccount

### Security Considerations
n/a

### Scaling Considerations
n/a

### Documentation Considerations
n/a

### Testing Considerations
Includes unit tests

### Upgrade Considerations
n/a, yet to be released
mergify bot added a commit that referenced this issue Aug 16, 2024
refs: #9193
closes: #9901

## Description
Adds an e2e test for `sendAnywhere.contract.js`, ensuring `zoeTools.localTransfer()` and `localOrchestrationAccount.transfer()` are working as expected.

Fixes a bug in `zoeTools.transfer()` where a vow chain was broken (we used `Promise.all()` on an array of vows instead of `vowTools.allVows()`.

Also satisfies this requirement in #9193:
- As new user of an orchestration contract, I need to “deposit” (move) my fungible funds into a place controlled by orchestration so it can do things with it.
 - for assets available through vbank starting on Agoric in a smart-wallet

### Security Considerations
n/a

### Scaling Considerations
n/a

### Documentation Considerations
n/a

### Testing Considerations
Includes tests for paths where bugs were discovered.

### Upgrade Considerations
n/a
mergify bot added a commit that referenced this issue Aug 16, 2024
refs: #9193
refs: #9784

## Description
These changes primarily implement the `.transfer` method on `CosmosOrchestrationAccount`. The changes require providing the exo with a `ChainHub` for looking up transferChannel info and `TimerService` for building a default timeout parameter. Notably, the `.transfer()` Promise resolves when the host chain submits the transfer to other remote chain. The `sendThenWaitForAck` logic that `LocalOrchestrationAccount` has is not yet implemented but tracked via #9784.

Other changes include:
- simplified `transferChannel` lookup logic in `LocalOrchestrationAccount`
- `/boot/tools/support.ts` changes to simplify `sendPacket` ack mocks for dibc bridge
- bootstrap tests for `LocalOrchAccount.transfer()` using `Transfer` invitationMaker
- `/boot/tools/support.ts` changes to ensure mock localChain addresses are unique
- `packetUtils` interface guard change for an observed, but seemingly benign, error

### Security Considerations
n/a

### Scaling Considerations
n/a

### Documentation Considerations
- [ ] how to best document the differences between LOA and COA for the interim?

### Testing Considerations
Includes unit and boot tests.

### Upgrade Considerations
n/a, unreleased code
mergify bot added a commit that referenced this issue Sep 19, 2024
closes: #9925
refs: #9193

## Description
- This PR primarily hardens the `zoeTools` implementation for orchestrated async-flows by:
  - ensuring payments handled in `zoeTools.localTransfer` are recovered to the original seat in the event of failure or partial failure
  - providing a `zoeTools.withdrawFromSeat` retriable function to facilitate withdrawing funds from a LocalChainAccount to a User seat. Ensures all changes are rolled back in the event of partial failure.
   - using `asVow` instead of `retriable` since these operations are not idempotent
- Creates `src/fixtures/zoe-tools.contract.js` to facilitate testing error paths
- To facilitate the change, `vowTools.allVowsSettled` was implemented in `@agoric/vow`. See #10077 which this PR depends on

### Security Considerations
Improves safety around Payment handling in async-flow contracts in `@agoric/orchestration`

### Scaling Considerations
n/a

### Documentation Considerations
Updates jsdoc strings for `localTransfer` and `withdrawToSeat`. As maintainer notes for considerations around `asVow` and promptness.

### Testing Considerations
Includes tests with different failure paths previously unaccounted for.

### Upgrade Considerations
Unreleased code that will be part of an npm release
mergify bot added a commit that referenced this issue Sep 20, 2024
refs: #9193

## Description
Adds `Deposit` and `Withdraw` invitationMakers to `LocalOrchestrationAccount`, leveraging `ZoeTools.localTransfer` and `ZoeTools.withdrawToSeat`

### Security Considerations
Involves withdrawing payments to a temporary seat, which can be risky.  Leverages `ZoeTools` which rolls back allocations in failure and partial failure scenarios.

### Scaling Considerations
n/a

### Documentation Considerations
I'm not sure how well we document the platform-provided `invitationMakers` and `.asContinuingOffer()` helper. We might consider doing so in the future.

### Testing Considerations
Includes new unit tests. Wallet Driver tests in an e2e environment are coming in a future PR after #9966 lands

### Upgrade Considerations
n/a, library code
@mergify mergify bot closed this as completed in #10211 Oct 4, 2024
@mergify mergify bot closed this as completed in 097ef82 Oct 4, 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
Projects
None yet
2 participants