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

user story: AutoStake #9042

Closed
1 of 2 tasks
turadg opened this issue Mar 6, 2024 · 4 comments
Closed
1 of 2 tasks

user story: AutoStake #9042

turadg opened this issue Mar 6, 2024 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@turadg
Copy link
Member

turadg commented Mar 6, 2024

What is the Problem Being Solved?

e2e demonstration of user value of the Account interface in the Orchestration API

#8863 provides that but has more dependencies. This story is an earlier milestone.

Description of the Design

  1. User invokes offer to have StakeAtom contract create a localchain account
    • Then user has a localchain Cosmos account
  2. User IBC transfers ATOM (IBC Hub-Agoric) into their Agoric localchain account
    • StakeAtom-held localchain account receives assets
    • Memo field indicates invoking StakeAtom hook
    • ^ depends on the vtransfer PR
  3. StakeAtom creates ICA on Cosmos Hub
  4. StakeAtom triggers IBC send of ATOM to Cosmos Hub ICA
    • MsgTransfer with a memo indicating the Contract for acknowledgement
    • Localchain’s MsgTransfer sends via the transfer App
  5. StakeAtom receives ack it has completed
  6. StakeAtom triggers delegation to a Cosmos Hub validator

Tasks

  • not in scope: substory: user with ATOM in LCA can send it to a new Cosmos Hub ICA

Security Considerations

Scaling Considerations

Test Plan

Upgrade Considerations

@turadg turadg added the enhancement New feature or request label Mar 6, 2024
@turadg turadg self-assigned this Mar 6, 2024
@0xpatrickdev 0xpatrickdev assigned 0xpatrickdev and unassigned turadg Mar 13, 2024
0xpatrickdev added a commit to 0xpatrickdev/agoric-sdk that referenced this issue Apr 4, 2024
mergify bot added a commit that referenced this issue Apr 12, 2024
refs: #9042

## Description

In the course of reviewing
#9114 it wasn't clear what the
`/ibc-hop` encoding spec was. This PR separates an `ibc-utils.js` to try
to encapsulate that. @iomekam @michaelfig can you provide some
additional documentation on its design?

This also adds types I used to understand the code, dropping `any`
bindings from 338 to 86.

One type change was in `vows` to add the context param.

### 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? -->
0xpatrickdev added a commit to 0xpatrickdev/agoric-sdk that referenced this issue May 2, 2024
mergify bot added a commit that referenced this issue May 6, 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): -->

closes: #9072
refs: #9042 
refs: #9162 
## Description

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

- Binds an `icqcontroller-*` port and sends a query packet using
`CosmosQuery` (`/icq/v1/packet.js`) and `RequestQuery`
(`/tendermint/abci/types.js`).
- Adds `.queryBalance([denom])` method to `StakingAccountHolder` exo in
`StakeAtom` contract, using `QueryBalanceRequest`
(`/cosmos/bank/v1beta1/query.js`)
- adds `icq/v1` protos from
[cosmos/ibc-apps#8e64543](https://github.com/cosmos/ibc-apps/tree/18248c35e913b3ccba99a2756612b1fcb3370a0d/modules/async-icq/proto/icq/v1)
- adds `JsonSafe`, `typeUrlToGrpcPath`, and `toRequestQueryJson` helpers
to `@agoric/cosmic-proto`
- ensures we are using `ChainAddress` objects in all orchestration code
(#9162)

Able to confirm port creation and successful balances queries in e2e
testing:
<details>
<summary>relayer logs</summary>

```log
2024-04-05T01:57:06.907563Z  INFO ThreadId(29) worker.batch{chain=agoriclocal}:supervisor.handle_batch{chain=agoriclocal}:supervisor.process_batch{chain=agoriclocal}:worker.channel{channel=channel::channel-0/icqcontroller-0:agoriclocal->osmosis-test}: 🎊  osmosis-test => OpenTryChannel(OpenTry { port_id: icqhost, channel_id: channel-1, connection_id: connection-1, counterparty_port_id: icqcontroller-0, counterparty_channel_id: channel-0 }) at height 0-470
2024-04-05T01:57:06.907586Z  INFO ThreadId(29) worker.batch{chain=agoriclocal}:supervisor.handle_batch{chain=agoriclocal}:supervisor.process_batch{chain=agoriclocal}:worker.channel{channel=channel::channel-0/icqcontroller-0:agoriclocal->osmosis-test}: channel handshake step completed with events: OpenTryChannel(OpenTry { port_id: icqhost, channel_id: channel-1, connection_id: connection-1, counterparty_port_id: icqcontroller-0, counterparty_channel_id: channel-0 })
2024-04-05T01:57:11.647420Z  INFO ThreadId(48) worker.batch{chain=osmosis-test}:supervisor.handle_batch{chain=osmosis-test}:supervisor.process_batch{chain=osmosis-test}:worker.channel{channel=channel::channel-1/icqhost:osmosis-test->agoriclocal}: 🎊  agoriclocal => OpenAckChannel(OpenAck { port_id: icqcontroller-0, channel_id: channel-0, connection_id: connection-0, counterparty_port_id: icqhost, counterparty_channel_id: channel-1 }) at height 0-52
2024-04-05T01:57:11.647448Z  INFO ThreadId(48) worker.batch{chain=osmosis-test}:supervisor.handle_batch{chain=osmosis-test}:supervisor.process_batch{chain=osmosis-test}:worker.channel{channel=channel::channel-1/icqhost:osmosis-test->agoriclocal}: channel handshake step completed with events: OpenAckChannel(OpenAck { port_id: icqcontroller-0, channel_id: channel-0, connection_id: connection-0, counterparty_port_id: icqhost, counterparty_channel_id: channel-1 })
```
</details>

<img width="1557" alt="Screenshot 2024-04-19 at 4 47 17 PM"
src="https://github.com/Agoric/agoric-sdk/assets/11021913/d569bcfb-f0e0-43dc-8a0a-b87ae601a731">


### 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? -->
@dckc
Copy link
Member

dckc commented May 20, 2024

e2e demonstration of user value of Orchestration API

does that mean getChain and such? async flow?

today we (@rowgraus , @turadg , @0xpatrickdev , etc.) agreed that demonstrating only the Account API suffices here.

mergify bot added a commit that referenced this issue Jun 15, 2024
refs: #8896 

## Description

The current `stakeIca.contract.js` bundle is ~4.5 MB uncompressed. Any attempted installation resulted in **400 Bad Request: request body too large** .

This change adjusts `app.toml` and `config.toml` params like `max_body_bytes`, `max_header_bytes`, `max_txs_bytes`, `max_tx_bytes`, and `rpc-max-body-bytes`.

### Security Considerations

### Scaling Considerations

The settings may not match other environments, and developers may find their bundles are too large to install if they are only developing against this environment.

### Documentation Considerations

The `update-config.sh` is long and mostly copied from `cosmology-tech/starship`. I've added a note about its source and a separate commit for the adjustments made to it.

### Testing Considerations

Manually tested the parameters in the course of #9042. Expect a test to be checked in with the completion of #9042.

Running the **Multichain E2E Testing** job and ensuring the `Setup Starship Infrastructure` step passes should give us confidence the overrides are also working in CI.

### Upgrade Considerations
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
mergify bot added a commit that referenced this issue Jul 3, 2024
refs: #9042

## Description

This PR primarily improves unit testing around the `.undelegate()` flow and was motivated by a failure (inability to decode `completionTime`) discovered in E2E testing. 

- Add tests for cosmos-orchestration-account staking flows using mocked IBC messages
- Use `Timestamp` (from [google/protobuf/timestamp.proto](https://buf.build/protocolbuffers/wellknowntypes/docs/657250e6a39648cbb169d079a60bd9ba:google.protobuf#google.protobuf.Timestamp)) instead of `Date` for `@agoric/cosmic-proto` codegen for better endo compatibility
- Update `undelegate()` to use Timestamp for `wakeAt()` calculation

### Testing Considerations
This PR includes additional tests for cosmos-orchestration-account flows.

### Upgrade Considerations
This is a breaking change for `@agoric/cosmic-proto`, but not for the `agoric` namespace.
0xpatrickdev added a commit that referenced this issue Jul 9, 2024
- creates an async-flow based on requirements in #9042. Namely, when funds are deposited to an LCA, they are automically sent to an ICA and staked.
0xpatrickdev added a commit that referenced this issue Jul 15, 2024
- PortfolioHolder is a kit that holds multiple OrchestrationAccounts and returns a single invitationMaker and TopicRecord
- the Action invitationMaker is designed to pass through calls to invitationMakers from sub-accounts, keyed by chainName
- refs #9042, which requires multiple accounts in a single user offer flow
0xpatrickdev added a commit that referenced this issue Jul 15, 2024
- creates an example contract that user .monitorTransfers to react to an incoming IBC transfer. when the transfer is received, it's sent to an ICA
  then delegated. Both accounts are put in a PortfolioHolder kit, which combines ContinuingOfferResults into a single record
- includes logic to ignore outgoing transfers and uknown denoms
- includes VTRANSFER_IBC_EVENT<'acknowledgementPacket'> fixture from observed values in a multichain testing environment (see PR comments for logs)
- refs: #9042
0xpatrickdev added a commit that referenced this issue Jul 15, 2024
- auto-stake-it transfer tokens to an InterchainAccount and delegates them when received over IBC to a contract controlled account
- includes patches for @cosmjs/stargate, since we are using it to execute IBC transfers from external accounts
- refs: #9042
0xpatrickdev added a commit that referenced this issue Jul 16, 2024
- PortfolioHolder is a kit that holds multiple OrchestrationAccounts and returns a single invitationMaker and TopicRecord
- the Action invitationMaker is designed to pass through calls to invitationMakers from sub-accounts, keyed by chainName
- refs #9042, which requires multiple accounts in a single user offer flow
0xpatrickdev added a commit that referenced this issue Jul 16, 2024
- monitorTransfers allows calls to register a handler to react to incoming and outgoing IBC ics20 transfers
- includes VTRANSFER_IBC_EVENT<'acknowledgementPacket'> fixture from observed values in a multichain testing environment (see PR comments
  for logs)
- refs: #9042
0xpatrickdev added a commit that referenced this issue Jul 16, 2024
- creates an example contract that user .monitorTransfers to react to an incoming IBC transfer. when the transfer is received, it's sent to an ICA
  then delegated. Both accounts are put in a PortfolioHolder kit, which combines ContinuingOfferResults into a single record
- includes logic to ignore outgoing transfers, uknown denoms, and unkown sourceChannels
- does not include logic to look for a specific value in the transfer memo field, but this could be added
- refs: #9042
0xpatrickdev added a commit that referenced this issue Jul 16, 2024
- PortfolioHolder is a kit that holds multiple OrchestrationAccounts and returns a single invitationMaker and TopicRecord
- the Action invitationMaker is designed to pass through calls to invitationMakers from sub-accounts, keyed by chainName
- refs #9042, which requires multiple accounts in a single user offer flow
0xpatrickdev added a commit that referenced this issue Jul 16, 2024
- monitorTransfers allows calls to register a handler to react to incoming and outgoing IBC ics20 transfers
- includes VTRANSFER_IBC_EVENT<'acknowledgementPacket'> fixture from observed values in a multichain testing environment (see PR comments
  for logs)
- refs: #9042
0xpatrickdev added a commit that referenced this issue Jul 16, 2024
- creates an example contract that user .monitorTransfers to react to an incoming IBC transfer. when the transfer is received, it's sent to an ICA
  then delegated. Both accounts are put in a PortfolioHolder kit, which combines ContinuingOfferResults into a single record
- includes logic to ignore outgoing transfers, uknown denoms, and unkown sourceChannels
- does not include logic to look for a specific value in the transfer memo field, but this could be added
- refs: #9042
0xpatrickdev added a commit that referenced this issue Jul 16, 2024
- PortfolioHolder is a kit that holds multiple OrchestrationAccounts and returns a single invitationMaker and TopicRecord
- the Action invitationMaker is designed to pass through calls to invitationMakers from sub-accounts, keyed by chainName
- refs #9042, which requires multiple accounts in a single user offer flow
0xpatrickdev added a commit that referenced this issue Jul 16, 2024
- monitorTransfers allows calls to register a handler to react to incoming and outgoing IBC ics20 transfers
- includes VTRANSFER_IBC_EVENT<'acknowledgementPacket'> fixture from observed values in a multichain testing environment (see PR comments
  for logs)
- refs: #9042
0xpatrickdev added a commit that referenced this issue Jul 16, 2024
- creates an example contract that user .monitorTransfers to react to an incoming IBC transfer. when the transfer is received, it's sent to an ICA
  then delegated. Both accounts are put in a PortfolioHolder kit, which combines ContinuingOfferResults into a single record
- includes logic to ignore outgoing transfers, uknown denoms, and unkown sourceChannels
- does not include logic to look for a specific value in the transfer memo field, but this could be added
- refs: #9042
0xpatrickdev added a commit that referenced this issue Jul 16, 2024
- PortfolioHolder is a kit that holds multiple OrchestrationAccounts and returns a single invitationMaker and TopicRecord
- the Action invitationMaker is designed to pass through calls to invitationMakers from sub-accounts, keyed by chainName
- refs #9042, which requires multiple accounts in a single user offer flow
0xpatrickdev added a commit that referenced this issue Jul 16, 2024
- monitorTransfers allows calls to register a handler to react to incoming and outgoing IBC ics20 transfers
- includes VTRANSFER_IBC_EVENT<'acknowledgementPacket'> fixture from observed values in a multichain testing environment (see PR comments
  for logs)
- refs: #9042
0xpatrickdev added a commit that referenced this issue Jul 16, 2024
- creates an example contract that user .monitorTransfers to react to an incoming IBC transfer. when the transfer is received, it's sent to an ICA
  then delegated. Both accounts are put in a PortfolioHolder kit, which combines ContinuingOfferResults into a single record
- includes logic to ignore outgoing transfers, uknown denoms, and unkown sourceChannels
- does not include logic to look for a specific value in the transfer memo field, but this could be added
- refs: #9042
0xpatrickdev added a commit that referenced this issue Jul 17, 2024
- PortfolioHolder is a kit that holds multiple OrchestrationAccounts and returns a single invitationMaker and TopicRecord
- the Action invitationMaker is designed to pass through calls to invitationMakers from sub-accounts, keyed by chainName
- refs #9042, which requires multiple accounts in a single user offer flow
0xpatrickdev added a commit that referenced this issue Jul 17, 2024
- monitorTransfers allows calls to register a handler to react to incoming and outgoing IBC ics20 transfers
- includes VTRANSFER_IBC_EVENT<'acknowledgementPacket'> fixture from observed values in a multichain testing environment (see PR comments
  for logs)
- refs: #9042
0xpatrickdev added a commit that referenced this issue Jul 17, 2024
- creates an example contract that user .monitorTransfers to react to an incoming IBC transfer. when the transfer is received, it's sent to an ICA
  then delegated. Both accounts are put in a PortfolioHolder kit, which combines ContinuingOfferResults into a single record
- includes logic to ignore outgoing transfers, uknown denoms, and unkown sourceChannels
- does not include logic to look for a specific value in the transfer memo field, but this could be added
- refs: #9042
@0xpatrickdev
Copy link
Member

0xpatrickdev commented Jul 17, 2024

Memo field indicates invoking StakeAtom hook

Regarding this requirement, it's not implemented in the current revision of #9666. The current logic is to stake any stakeDenom (e.g. ATOM) tokens that come to the LCA address.

It would be trivial to add logic for this, but we are blocked on testing this in an E2E until #9200 - we don't have the ability to add memos to MsgTransfer with the current stargate client.

There is other logic present in the current revision that verifies many other parts of the packet -

  1. source channel matches the one we are expecting (e.g. channel-1)

  2. denom matches what we are expected (viewed from the perspective of the sender, not the receiver - e.g., uatom)

  3. the transfer is incoming, not outgoing (FungibleTokenPacketData.receiver)

receiveUpcall(event) {
trace('receiveUpcall', event);
// ignore packets from unknown channels
if (event.packet.source_channel !== this.state.sourceChannel) {
return;
}
const tx = /** @type {FungibleTokenPacketData} */ (
JSON.parse(atob(event.packet.data))
);
trace('receiveUpcall packet data', tx);
const { remoteDenom, localChainAddress } = this.state;
// ignore outgoing transfers
if (tx.receiver !== localChainAddress.value) {
return;
}
// only interested in transfers of `remoteDenom`
if (tx.denom !== remoteDenom) {
return;
}

Edit: I would consider what's currently there enough to close the ticket, but would be happy to keep this open until 1) #9200 is completed, and 2) memo logic is added.

0xpatrickdev added a commit that referenced this issue Jul 17, 2024
- PortfolioHolder is a kit that holds multiple OrchestrationAccounts and returns a single invitationMaker and TopicRecord
- the Action invitationMaker is designed to pass through calls to invitationMakers from sub-accounts, keyed by chainName
- refs #9042, which requires multiple accounts in a single user offer flow
0xpatrickdev added a commit that referenced this issue Jul 17, 2024
- monitorTransfers allows calls to register a handler to react to incoming and outgoing IBC ics20 transfers
- includes VTRANSFER_IBC_EVENT<'acknowledgementPacket'> fixture from observed values in a multichain testing environment (see PR comments
  for logs)
- refs: #9042
0xpatrickdev added a commit that referenced this issue Jul 17, 2024
- creates an example contract that user .monitorTransfers to react to an incoming IBC transfer. when the transfer is received, it's sent to an ICA
  then delegated. Both accounts are put in a PortfolioHolder kit, which combines ContinuingOfferResults into a single record
- includes logic to ignore outgoing transfers, uknown denoms, and unkown sourceChannels
- does not include logic to look for a specific value in the transfer memo field, but this could be added
- refs: #9042
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 pushed a commit that referenced this issue Aug 8, 2024
closes: XXX
refs: #9402 (comment) #9402 (comment)  #9407

## Description

#9407 explains that sometimes we used a `.optional(AmountShape)` guard to describe an `optAmountShape` argument.

#9402 (comment) #9402 (comment) discussed ways to fix this that were ultimately omitted from #9402 . This PR provides a form of the fix discussed in those comments. Three remaining controversies I'd like my reviewers to comment on:
- Whether the name of the exported pattern should be `AmountShapeShape` or `AmountPatternShape`. See my doc-comment on `AmountPatternShape` for my weak reasoning about why I chose this name.
- If we do go with `AmountPatternShape` for the stated reasons, should the corresponding parameter variables be changed from `optAmountShape` to `optAmountPattern`.
- Whatever the name of the exported pattern, should it be defined as 
   - `M.pattern()`
   - `harden({ brand: M.pattern(), value: M.pattern() })`
   - `harden({ brand: BrandShape, value: M.pattern() })`
   - something else

### Security Considerations

For almost all of the choices of resolving the above controversies, this PR remains a pure bug fix. All correct code that used to work will continue working the same way, and some correct code that used to incorrectly fail due to this bug will now start working correctly.

The exception would be if we both accepting the renaming of some existing occurrences of `M.pattern()` to `AmountPatternShape`/`AmountShapeShape`, and define this name as a pattern narrower than `M.pattern()`. In that case, existing (hypothetical) code that, for example, used `M.any()` in such an argument position would start failing.

### Scaling Considerations

None
### Documentation Considerations

None
### Testing Considerations

#9407 mentions how to reproduce the bug it reports. This PR should add that as a test, to verify that this PR fixes that bug.
### Upgrade Considerations

#9402 (comment) and #9402 (comment) explain the upgrade concern that was likely the cause for omitting this fix from #9042
@turadg
Copy link
Member Author

turadg commented Aug 12, 2024

Memo field routing deferred to #9877

@aj-agoric
Copy link

Closing as this was decided to be complete with moving functionality for memo field #9877

kriskowal pushed a commit that referenced this issue Aug 27, 2024
closes: XXX
refs: #9402 (comment) #9402 (comment)  #9407

## Description

#9407 explains that sometimes we used a `.optional(AmountShape)` guard to describe an `optAmountShape` argument.

#9402 (comment) #9402 (comment) discussed ways to fix this that were ultimately omitted from #9402 . This PR provides a form of the fix discussed in those comments. Three remaining controversies I'd like my reviewers to comment on:
- Whether the name of the exported pattern should be `AmountShapeShape` or `AmountPatternShape`. See my doc-comment on `AmountPatternShape` for my weak reasoning about why I chose this name.
- If we do go with `AmountPatternShape` for the stated reasons, should the corresponding parameter variables be changed from `optAmountShape` to `optAmountPattern`.
- Whatever the name of the exported pattern, should it be defined as 
   - `M.pattern()`
   - `harden({ brand: M.pattern(), value: M.pattern() })`
   - `harden({ brand: BrandShape, value: M.pattern() })`
   - something else

### Security Considerations

For almost all of the choices of resolving the above controversies, this PR remains a pure bug fix. All correct code that used to work will continue working the same way, and some correct code that used to incorrectly fail due to this bug will now start working correctly.

The exception would be if we both accepting the renaming of some existing occurrences of `M.pattern()` to `AmountPatternShape`/`AmountShapeShape`, and define this name as a pattern narrower than `M.pattern()`. In that case, existing (hypothetical) code that, for example, used `M.any()` in such an argument position would start failing.

### Scaling Considerations

None
### Documentation Considerations

None
### Testing Considerations

#9407 mentions how to reproduce the bug it reports. This PR should add that as a test, to verify that this PR fixes that bug.
### Upgrade Considerations

#9402 (comment) and #9402 (comment) explain the upgrade concern that was likely the cause for omitting this fix from #9042
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
Development

Successfully merging a pull request may close this issue.

4 participants