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

types for chain addresses #9162

Closed
turadg opened this issue Mar 28, 2024 · 2 comments · Fixed by #9662
Closed

types for chain addresses #9162

turadg opened this issue Mar 28, 2024 · 2 comments · Fixed by #9662
Assignees
Labels
enhancement New feature or request

Comments

@turadg
Copy link
Member

turadg commented Mar 28, 2024

What is the Problem Being Solved?

With orchestration there will be "chain address" for many chains and functions that take an address may only work for addresses on a certain chain.

Description of the Design

Use TS string templates to create a generic "Cosmos address" and then concrete types for some well known chains. Consider having special addresses for "validator" etc

Security Considerations

Scaling Considerations

Test Plan

Upgrade Considerations

@turadg turadg added the enhancement New feature or request label Mar 28, 2024
@turadg turadg self-assigned this Mar 28, 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? -->
@0xpatrickdev
Copy link
Member

0xpatrickdev commented May 6, 2024

#9312 contributed towards this. ~Two areas where I see remaining work -

  • LocalChainAccount should use a ChainAddress object instead of a string

  • finalize types in orchestration/src/types.d.ts:

    /** An address on some blockchain, e.g., cosmos, eth, etc. */
    export type ChainAddress = {
    /** e.g. 1 for Ethereum, agoric-3 for Agoric, cosmoshub-4 for Cosmos */
    chainId: string;
    address: string;
    // TODO what's the right way to scope the address? it's not chainId
    addressEncoding: 'bech32' | 'ethereum';
    };

    • do we need addressEncoding? I'm not sure we will be doing any encoding/decoding of pubkeys, so maybe no?
    • "etherum" encoding should maybe be hexadecimal
    • Tangential - we need to ensure chainId is readily available via Chain / ChainInfo

@0xpatrickdev
Copy link
Member

Discussion around renaming ChainAddress.address -> ChainAddress.value led me back here... More recent thoughts:

  • I think addressEncoding is helpful to preserve as it distinguishs accounts on cosmos-sdk chains that support hex and bech32. encoding, might be more concise
  • EVM chainIds are integers. We might want an additional human-readable field, chainName or shortName, to capture something like 'eth' or 'gno'. See eip-3770 which is unofficially adopted by many

This gist attempts to spec out a universal blockchain address type. The pubKey types are largely irrelevant since we are only dealing with contract-controlled accounts.

@mergify mergify bot closed this as completed in #9662 Jul 8, 2024
mergify bot added a commit that referenced this issue Jul 8, 2024
closes: #9162

## Description

#9162 is already mostly done. This revisits the property names with some improvements. Also cleans up nearby types and docs.

#9162 (comment) has some good ideas for future directions but this PR wraps up the requirements for release.

### Security Considerations
-

### Scaling Considerations
-

### Documentation Considerations
-

### Testing Considerations
-

### Upgrade Considerations
unreleased
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.

2 participants