-
Notifications
You must be signed in to change notification settings - Fork 207
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
Conversation
38c7bc3
to
54d830f
Compare
1ecce05
to
ce019fa
Compare
ce019fa
to
1ffc1be
Compare
Deploying agoric-sdk with Cloudflare Pages
|
/** @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 */ |
There was a problem hiding this comment.
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.
/** @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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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', | ||
); |
There was a problem hiding this comment.
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 });
}
});
There was a problem hiding this comment.
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 });
}
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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/
?
There was a problem hiding this comment.
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
packages/orchestration/src/guards.js
Outdated
@@ -0,0 +1,7 @@ | |||
import { M } from '@endo/patterns'; |
There was a problem hiding this comment.
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()), |
There was a problem hiding this comment.
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]: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gather this is for use from the REPL.
Do we test it?
I'm surprised the homeKeys
test hasn't failed.
https://github.com/Agoric/agoric-sdk/blob/master/packages/boot/test/bootstrapTests/test-demo-config.ts#L42
Another candidate is..
https://github.com/Agoric/agoric-sdk/blob/master/packages/solo/test/test-home.js
There was a problem hiding this comment.
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 => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const prepareQueryConnection = zone => | |
export const prepareQueryConnectionKit = zone => |
*/ | ||
export const makeICQChannelAddress = ( | ||
controllerConnectionId, | ||
{ version = 'icq-1' } = {}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 howrethrowUnlessMissing
is used as acatch
handler, that will interpret the rejection and if it seems to be due to a missing method, it will swallow the error, otherwise willthrow
it again.
There was a problem hiding this comment.
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
/** @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 */ |
There was a problem hiding this comment.
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?', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
2347cca
to
e36559e
Compare
packages/cosmic-proto/package.json
Outdated
@@ -52,6 +56,14 @@ | |||
"types": "./dist/codegen/ibc/*.d.ts", | |||
"default": "./dist/codegen/ibc/*.js" | |||
}, | |||
"./icq/v1/*.js": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/v1
unnecessary, please remove
packages/cosmic-proto/package.json
Outdated
@@ -64,6 +76,14 @@ | |||
"types": "./dist/codegen/agoric/swingset/swingset.d.ts", | |||
"default": "./dist/codegen/agoric/swingset/swingset.js" | |||
}, | |||
"./tendermint/abci/*.js": { |
There was a problem hiding this comment.
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 {{ |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
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
* @import { AnyJson, RequestQueryJson } from '@agoric/cosmic-proto'; | |
* @import {AnyJson, RequestQueryJson} from '@agoric/cosmic-proto'; |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sourced from here:
agoric-sdk/packages/network/src/network.js
Line 676 in 160d332
bindPort: M.callWhen(Shape.Endpoint).returns(Shape.Vow$(Shape.Port)), |
).finish(); | ||
|
||
return JSON.stringify({ | ||
type: 1, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
/** @type {Base64Any<InterchainAccountPacketData>} */ ({ | ||
type: PacketType.TYPE_EXECUTE_TX, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
e78c7ad
to
06d745e
Compare
@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
to complement the existing |
@@ -0,0 +1,15 @@ | |||
syntax = "proto3"; | |||
|
|||
package icq.v1; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ bd88224
packages/cosmic-proto/src/helpers.ts
Outdated
*/ | ||
export type RequestQueryJson = JsonSafe<RequestQuery>; | ||
|
||
const QUERY_REQ_TYPEURL_RE = /^\/(\w+(?:\.\w+)*)\.Query(\w+)Request$/; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
bd88224
to
4f4c17f
Compare
- throws TypeError if string does not match REMOTE_ADDR_RE - not currently used as a runtime check in the ibc ProtocolImpl, but could be if [test-net-ibc-upgrade.ts](packages/boot/test/bootstrapTests/test-net-ibc-upgrade.ts) is amenable
4f4c17f
to
9f0ae09
Compare
closes: #9072
refs: #9042
refs: #9162
Description
icqcontroller-*
port and sends a query packet usingCosmosQuery
(/icq/v1/packet.js
) andRequestQuery
(/tendermint/abci/types.js
)..queryBalance([denom])
method toStakingAccountHolder
exo inStakeAtom
contract, usingQueryBalanceRequest
(/cosmos/bank/v1beta1/query.js
)icq/v1
protos from cosmos/ibc-apps#8e64543JsonSafe
,typeUrlToGrpcPath
, andtoRequestQueryJson
helpers to@agoric/cosmic-proto
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
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations
Upgrade Considerations