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

feat(orchestration): icq balance query #9198

Merged
merged 10 commits into from
May 6, 2024
Merged

feat(orchestration): icq balance query #9198

merged 10 commits into from
May 6, 2024

Conversation

0xpatrickdev
Copy link
Member

@0xpatrickdev 0xpatrickdev commented Apr 5, 2024

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

Description

  • 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
  • adds JsonSafe, typeUrlToGrpcPath, and toRequestQueryJson helpers to @agoric/cosmic-proto
  • ensures we are using ChainAddress objects in all orchestration code (types for chain addresses #9162)

Able to confirm port creation and successful balances queries in e2e testing:

relayer logs
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 })
Screenshot 2024-04-19 at 4 47 17 PM

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

@0xpatrickdev 0xpatrickdev force-pushed the 8881-ica-send-message branch 5 times, most recently from 38c7bc3 to 54d830f Compare April 9, 2024 17:50
Base automatically changed from 8881-ica-send-message to master April 9, 2024 18:46
@0xpatrickdev 0xpatrickdev force-pushed the 9072-icq-query branch 2 times, most recently from 1ecce05 to ce019fa Compare April 10, 2024 18:14
Copy link

cloudflare-workers-and-pages bot commented Apr 22, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9f0ae09
Status: ✅  Deploy successful!
Preview URL: https://562db353.agoric-sdk.pages.dev
Branch Preview URL: https://9072-icq-query.agoric-sdk.pages.dev

View logs

@0xpatrickdev 0xpatrickdev marked this pull request as ready for review April 22, 2024 17:28
Comment on lines 13 to 14
/** @typedef {{ path: string; data: string; height?: bigint; prove?: boolean; }} QueryMsg like RequestQuery from Tendermint ABCI, but with a base64 encoded `data` key */
/** @typedef {Omit<ResponseQuery, 'key'|'value'> & { key: string; value: string; }} QueryResponse like ResponseQuery from Tendermint ABCI, but with a base64 encoded `key` and `value` keys */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try to hew closely to the external typedefs.

Suggested change
/** @typedef {{ path: string; data: string; height?: bigint; prove?: boolean; }} QueryMsg like RequestQuery from Tendermint ABCI, but with a base64 encoded `data` key */
/** @typedef {Omit<ResponseQuery, 'key'|'value'> & { key: string; value: string; }} QueryResponse like ResponseQuery from Tendermint ABCI, but with a base64 encoded `key` and `value` keys */
/** @typedef {RequestQuery & { data: string; }} QueryMsg like RequestQuery from Tendermint ABCI, but with a base64 encoded `data` key */
/** @typedef {ResponseQuery & { key: string; value: string; }} QueryResponse like ResponseQuery from Tendermint ABCI, but with a base64 encoded `key` and `value` keys */

That reveals an undocumented difference: this had made height and prove optional but they aren't in RequestQuery:

export interface RequestQuery {
    data: Uint8Array;
    path: string;
    height: bigint;
    prove: boolean;
}

Future improvements:

  • a helper to generate a base64 type from interface
  • update the codegen to make that the return type instead of unknown

Here's a helper we could use,

// TODO make codegen toJSON() return these instead of unknown
/**
 * Proto Any with arrays encoded as base64
 */
export type Base64Any<T> = {
  [Prop in keyof T]: T[Prop] extends Uint8Array ? string : T[Prop];
};

I'll try to incorporate that into #9219

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I think we should try to land #9219 first

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed default values for height and prove out of this implementation, but we might consider including them in the future. The default values we get from .fromPartial() (height: 0n, prove: false) are documented in the tests.

I'm not sure if these defaults are sensible, but responses seem to be reactive to state changes despite the height: 0n parameter. Perhaps, this parameter is more intended for historical queries.

prove is something we may want to investigate more - the ~only mention in cosmos/ibc-apps/modules/async-icq is a failing test which leads me to believe query proofs may not yet be supported:
https://github.com/cosmos/ibc-apps/blob/18248c35e913b3ccba99a2756612b1fcb3370a0d/modules/async-icq/keeper/relay_test.go#L132-L154

Comment on lines 190 to 198
const orchestration = await EV.vat('bootstrap').consumeItem('orchestration');

const account = await EV(orchestration).createQueryConnection('connection-0');
t.log('Query Account', account);
t.truthy(account, 'createAccount returns an account');
t.truthy(
matches(account, M.remotable('QueryConnection')),
'QueryConnection is a remotable',
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the role of the party making a connection?

This code suggests it's "As the BLD stakers, we can make query connections."
Is that the expected usage?

I suspect it's "As a contract (developer), I can make query connections."
I suggest expressing that as something like...

test('Query connection can be created', async t => {
  const contract1 = async ({ orchestration }) => {
    const account = await EV(orchestration).createQueryConnection('connection-0');
    t.log('Query Account', account);
    t.truthy(account, 'createAccount returns an account');
    t.truthy(
      matches(account, M.remotable('QueryConnection')),
      'QueryConnection is a remotable',
    );
  }

  // core eval context
  {
    const { runUtils } = t.context;
    const { EV } = runUtils;
    const orchestration = await EV.vat('bootstrap').consumeItem('orchestration');
    await contract1({ orchestration });
  }
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change in stakingAccountHolder.js suggests that the contract isn't expected to get all of orchestration but rather only the connection (account? are these synonyms?):

  {
    const { runUtils } = t.context;
    const { EV } = runUtils;
    const orchestration = await EV.vat('bootstrap').consumeItem('orchestration');
    const account = await EV(orchestration).createQueryConnection('connection-0');
    t.log('Query Account', account);
    await contract1({ queryConnection: account });
  }

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use this convention

@@ -102,7 +138,7 @@ export const prepareStakingAccountHolder = (baggage, makeRecorderKit, zcf) => {
// FIXME brand handling and amount scaling
const amount = {
amount: String(ertpAmount.value),
denom: 'uatom',
denom: this.state.baseDenom, // TODO use ertpAmount.brand
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO when? Do we have an issue number? Or did you mean to do it in this PR?

@@ -4,14 +4,18 @@ import {
MsgDelegate,
MsgDelegateResponse,
} from '@agoric/cosmic-proto/cosmos/staking/v1beta1/tx.js';
import {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/contracts suggests this contract is part of the product API. Is that the case? or is it an example?
It's a pattern we have come to regret in the zoe package: #7392

Maybe they belong in tools/?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now in /src/examples after rebasing

@@ -0,0 +1,7 @@
import { M } from '@endo/patterns';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conventional filename for this seems to be typeGuards.js:

$ find packages/ -name 'typeGuards.js'
packages/smart-wallet/src/typeGuards.js
packages/internal/src/typeGuards.js
packages/time/src/typeGuards.js
packages/zoe/src/typeGuards.js
packages/SwingSet/src/typeGuards.js
packages/governance/src/typeGuards.js
packages/ERTP/src/typeGuards.js

zone.exoClassKit(
'Orchestration',
{
self: M.interface('OrchestrationSelf', {
bindPort: M.callWhen().returns(M.remotable()),
bindPort: M.callWhen(M.record()).returns(M.remotable()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you meant that it can take a promise for a record, that needs M.await().
That would be unusual, though.

@@ -92,6 +92,13 @@ export const getManifestForOrchestration = (_powers, { orchestrationRef }) => ({
orchestrationVat: 'orchestration',
},
},

[addOrchestrationToClient.name]: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the pointers, but I have removed this from the PR

});

/** @param {Zone} zone */
export const prepareQueryConnection = zone =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const prepareQueryConnection = zone =>
export const prepareQueryConnectionKit = zone =>

*/
export const makeICQChannelAddress = (
controllerConnectionId,
{ version = 'icq-1' } = {},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it OK for version to be any string at all, including those containing /?
If so, I'd appreciate a test to say so explicitly.

Copy link
Member Author

@0xpatrickdev 0xpatrickdev Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good callout. I've added44adcf1 which documents the limited functionality better. This function will not be exposed to API consumers, so I didn't feel it necessary to validate version. As I'm writing this comment it feels like a good idea to add the @internal typedoc annotation.

Doing some additional testing, it seems like network / ibc do not throw if we provide an invalid remoteAddr to .connect(). Is this worth implementing? Looks like decodeRemoteIbcAddress, or at least the part that throws a TypeError, used in the onConnect handler could be leveraged in connect cc @michaelfig

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would support adding code to do a throw if it's an "invalid remoteAddr", but the layering must be correct.

Only the ibc implementation (not network) should have that knowledge. Support for other transports like Wormhole or Hyperlane may be required down the road, and their notion of an "invalid remoteAddr" will necessarily be different from IBC's.

Copy link
Member Author

@0xpatrickdev 0xpatrickdev Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok cool. Attempted this here:e1aac9c, please lmk what you think.

I was thinking to make ProtocolHandler['validateRemoteAddr'] optional, but wasn't sure the best way to verify the existence of a remote method (i.e. if (protocolHandler.validateRemoteAddr) protocolHandler.validateRemoteAddr())

Edit: removed in e928643, as this breaks the ping pong between ibcServerMock and ibcClientMock in test-net-ibc-upgrade.ts. Cost to update this test seems high from my POV.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... wasn't sure the best way to verify the existence of a remote method

The only way is to try it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way is to try it.
Yes, and if you see how rethrowUnlessMissing is used as a catch handler, that will interpret the rejection and if it seems to be due to a missing method, it will swallow the error, otherwise will throw it again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rethrowUnlessMissing - nice! was going to say it'd be handy to have a function/tool for this

Comment on lines 13 to 14
/** @typedef {{ path: string; data: string; height?: bigint; prove?: boolean; }} QueryMsg like RequestQuery from Tendermint ABCI, but with a base64 encoded `data` key */
/** @typedef {Omit<ResponseQuery, 'key'|'value'> & { key: string; value: string; }} QueryResponse like ResponseQuery from Tendermint ABCI, but with a base64 encoded `key` and `value` keys */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I think we should try to land #9219 first

t.truthy(typeof result.height === 'bigint');
t.truthy(
result.value.length === 0,
'XXX why do we get an empty Uint8Array here?',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this just XXX or do we need to resolve it before merge?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what the expected behavior should be here. It doesn't seem we need this field, but it's surprising to see it provided in the response.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed this assertion for now

data: QueryBalanceRequest.encode(
QueryBalanceRequest.fromPartial({
address: chainAddress,
denom: denom || this.state.baseDenom,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default params belong in the function signature

decodeBase64(result.key),
);
if (!balance) throw Fail`Error parsing result ${result}`;
// TODO, return Amount? cast amount to bigint?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth a ticket to ensure we come back to this. maybe there is one already

@@ -102,7 +138,7 @@ export const prepareStakingAccountHolder = (baggage, makeRecorderKit, zcf) => {
// FIXME brand handling and amount scaling
const amount = {
amount: String(ertpAmount.value),
denom: 'uatom',
denom: this.state.baseDenom, // TODO use ertpAmount.brand
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, ticket

);
this.state.icaControllerNonce += 1;
this.state[opts.nonceKey] += 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be before the remote call? the remote might throw with partial failure and a subsequent request would use the same value. cc @iomekam

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_I am going to rebase this PR on top of #9228 once ready, so please disregard this logic. _

Was not thinking of the partial failure case, although it seems harmless as the object containing Port is not returned to the user until the Port + Connection are successfully instantiated. Nonetheless, this seems like it wouldn't comply with the Checks Effects Interactions pattern and it's better to make the state update before the remote call.

@@ -52,6 +56,14 @@
"types": "./dist/codegen/ibc/*.d.ts",
"default": "./dist/codegen/ibc/*.js"
},
"./icq/v1/*.js": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/v1 unnecessary, please remove

@@ -64,6 +76,14 @@
"types": "./dist/codegen/agoric/swingset/swingset.d.ts",
"default": "./dist/codegen/agoric/swingset/swingset.js"
},
"./tendermint/abci/*.js": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/abci unnecessary, please remove

*/
port =>
/**
* @type {{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider naming this type State

* @import { RecorderKit, MakeRecorderKit } from '@agoric/zoe/src/contractSupport/recorder.js';
* @import { Baggage } from '@agoric/swingset-liveslots';
* @import {AnyJson} from '@agoric/cosmic-proto';
* @import { AnyJson, RequestQueryJson } from '@agoric/cosmic-proto';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no padding for types lists, only for object properties

Suggested change
* @import { AnyJson, RequestQueryJson } from '@agoric/cosmic-proto';
* @import {AnyJson, RequestQueryJson} from '@agoric/cosmic-proto';

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged, updated one of the files I touched during other feedback and will prefer this going forward

zone.exoClassKit(
'Orchestration',
{
self: M.interface('OrchestrationSelf', {
bindPort: M.callWhen().returns(M.remotable()),
allocateICAControllerPort: M.callWhen().returns(
NetworkShape.Vow$(NetworkShape.Port),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this Vow$ thing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sourced from here:

bindPort: M.callWhen(Shape.Endpoint).returns(Shape.Vow$(Shape.Port)),

).finish();

return JSON.stringify({
type: 1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic number. please import or name it with a const

Copy link
Member Author

@0xpatrickdev 0xpatrickdev May 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Referencing InterchainPacketData and the Type.TYPE_EXECUTE_TX enum now: cb1a005

Comment on lines +38 to +39
/** @type {Base64Any<InterchainAccountPacketData>} */ ({
type: PacketType.TYPE_EXECUTE_TX,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@0xpatrickdev 0xpatrickdev requested review from dckc and turadg May 3, 2024 00:34
@0xpatrickdev
Copy link
Member Author

0xpatrickdev commented May 3, 2024

@turadg I added caaec18 since your last review so we don't have to manually type gRPC paths (and can derive them from typeUrls)

P.S. seems like something we could upstream to @cosmology/telescope. Would expect to see something ~like:

type QueryBalanceRequest = {
  grpcPath: string;
  toRequestQuery: (req: QueryBalanceRequest) => { path: string, data: Uint8Array };
}

to complement the existing typeUrl property and toProtoMsg method.

@@ -0,0 +1,15 @@
syntax = "proto3";

package icq.v1;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's unclear to me whether these (icq/v1) should live here (top level) or inside /ibc/applications. When placed in that directory, i see Error: Dependency Not Found icq/v1/icq.proto during yarn codegen. A concern is that the correct typeUrls may not be generated with this folder structure. Its seems we'd need to update this line to package ibc.applications.icq.v1, but not sure there is any sort of precedent for editing these files.

cc @michaelfig for insights

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming for .proto files is relative to the most immediate proto/ directory. It is important not to change the package ... lines to match a different directory structure. That will break the generated proto codecs. Instead just move the files into proto/${packageName.replaceAll('.', '/'), as was correctly done in this PR.

return this.state.icqConnections.get(controllerConnectionId)
.connection;
}
// allocate a new Port for every Connection
Copy link
Member Author

@0xpatrickdev 0xpatrickdev May 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is the correct behavior. An alternative is to bind a single icqcontroller-* port during the orchestration-proposal.js. Happy to fix here, or create a small task to capture the work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems worth its own issue; something like:

  • optimize ICQ port allocation

There I'd like us to find an ICQ spec that tells us what the right answer is, and/or do some integration testing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #9317 and included it as a code comment

* }} ICQConnectionKitState
*/

/** @param {Zone} zone */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd appreciate a more full docstring here.

In particular, let's please cite any external design constraints, such as the ICQ spec that we're conforming to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bd88224

*/
export type RequestQueryJson = JsonSafe<RequestQuery>;

const QUERY_REQ_TYPEURL_RE = /^\/(\w+(?:\.\w+)*)\.Query(\w+)Request$/;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really get the motivation for this but I'm not opposed.

Consider using named capture groups tho.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Motivation for the validation inside the function, or the function itself?

@0xpatrickdev 0xpatrickdev added the automerge:rebase Automatically rebase updates, then merge label May 3, 2024
@mergify mergify bot merged commit 8e94b2a into master May 6, 2024
63 checks passed
@mergify mergify bot deleted the 9072-icq-query branch May 6, 2024 04:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

orchestration - chain - ICQ Balance Query
4 participants