feat(cosmos-orch-account): expose .executeEncodedTx
#10341
Conversation
Deploying agoric-sdk with
|
| Latest commit: |
3262f6c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0b598281.agoric-sdk.pages.dev |
| Branch Preview URL: | https://pc-expose-execute-encoded-tx.agoric-sdk.pages.dev |
| @@ -317,17 +328,15 @@ export interface IBCMsgTransferOptions { | |||
| * | |||
| * @see {OrchestrationAccountI} | |||
| */ | |||
| export type CosmosChainAccountMethods<CCI extends CosmosChainInfo> = | |||
| export type CosmosChainAccountMethods<CCI extends CosmosChainInfo> = Pick< | |||
There was a problem hiding this comment.
this Pick looks arbitrary. I thought maybe it was IcaAccount without the getters, but it also omits executeTx. Is that because it's not yet implemented? Not a good reason when defining types.
Whatever the criteria are for this set of methods, please name it as a type and document it there. Alternately define it as an interface that IcaAccount extends.
There was a problem hiding this comment.
An interface that IcaAccount extends makes sense to me. Do you have a suggestion for the name of the type? IcaAccountBase(Methods) is the best I've come up with so far.
Regarding executeTx - I suggest we remove this from IcaAccount until there's a story that requires it. The original idea was to keep it to support Proto3Json messages, but it doesn't seem any chain has adopted this yet for ICAs. Having executeTx show up in typeahead in a flow for a non-agoric account seems like something we should avoid. cc @dtribble
RE the other methods omitted -
getAddressalready appears onOrchestrationAccountIgetRemoteAddress,getLocalAddress, andgetPortseem like lower level things an orchestration developer shouldn't be concerned with. However, if we feel these should be lifted to theCosmosOrchAccountexo, I wouldn't be opposed
There was a problem hiding this comment.
"Base" is a smell to me. How about IcaActions? I agree the getters shouldn't be available in an Orchestration flow.
+1 to removing executeTx until we have a story that requires it.
There was a problem hiding this comment.
+1 to removing executeTx until we have a story that requires it.
It turns out this is a breaking change for IcaAccountKit which was deployed as part of vat-orchestration in u17. The upgrade considerations seem low since consumers do not depend on it, but I'll layer it in a separate PR.
| @@ -143,6 +145,9 @@ export const IcaAccountHolderI = M.interface('IcaAccountHolder', { | |||
| ...stakingAccountQueriesMethods, | |||
| deactivate: M.call().returns(VowShape), | |||
| reactivate: M.call().returns(VowShape), | |||
| executeEncodedTx: M.call(M.arrayOf(Proto3Shape)) | |||
There was a problem hiding this comment.
Changing the Orch account interfaces requires @dtribble approval.
| (CCI extends { | ||
| icaEnabled: true; |
There was a problem hiding this comment.
CosmosChainAccountMethods now assumes that ICA methods are available for every value in CosmoChainInfo, regardless of of whether they have ICA enabled. Why is this legitimate?
There was a problem hiding this comment.
ICA seems to be enabled for all chains we currently have in ChainHub. When we add chains that do not have ICA enabled, we can add icaEnabled to the CosmosChainInfo type (it’s not currently there so this evaluates to false for every chain).
I also think it’d be better to leverage this generic parameter in the makeAccount function versus here:
makeAccount: () => CI extends
| { icaEnabled: true }
| { chainId: `agoric${string}` }
? Promise<OrchestrationAccount<CI>>
: never;
0xpatrickdev
force-pushed
the
pc/expose-execute-encoded-tx
branch
3 times, most recently
from
e3038f5 to
d683f60
Compare
- fixes `CosmosChainAccountMethods` that depended on non-existent `icaEnabled` parameter - only expose `IcaAccount` methods available on `OrchestrationAccount<CosmosChainInfo>` - removes `IcaAccount` and `StakingAccountQueries` methods from `OrchestrationAccount<agoric>` - improve `monitorTransfers` doc string
0xpatrickdev
force-pushed
the
pc/expose-execute-encoded-tx
branch
from
d683f60 to
3262f6c
Compare
closes: #10345
Description
.executeEncodedTxonCosmosOrchestrationAccountso developers can send custom messagesOrchestrationAccounttypeSecurity Considerations
N/A
Scaling Considerations
N/A
Documentation Considerations
Tests show how to use it and make sense of the response.
Testing Considerations
Includes unit tests and type tests
Upgrade Considerations
Library code for an NPM release