feat!: makeOsmosisSwap() using getDenomOn #9814
Conversation
- makeOsmosisSwap is now async - takes orch arg
using getVBankAssetInfo() in LocalChainFacade
- pass orch to makeOsmosisSwap - parameterize brands.Out
| @@ -39,6 +40,11 @@ export const OrchestratorI = M.interface('Orchestrator', { | |||
| getChain: M.call(M.string()).returns(Vow$(ChainInfoShape)), | |||
| makeLocalAccount: M.call().returns(Vow$(LocalChainAccountShape)), | |||
| getBrandInfo: M.call(DenomShape).returns(BrandInfoShape), | |||
| getDenomOn: M.call( | |||
| DenomShape, | |||
| M.remotable('Chain'), | |||
There was a problem hiding this comment.
it is remotable but it lives in the same vat. can we have a stricter type in that case? (curious, not blocking)
There was a problem hiding this comment.
no; patterns are pass-invariant, and "in the same vat" is the antithesis of pass-invariant.
| @@ -67,7 +73,7 @@ const prepareOrchestratorKit = ( | |||
| chainByName, | |||
| makeLocalChainFacade, | |||
| makeRemoteChainFacade, | |||
| vowTools: { watch, asVow }, | |||
| vowTools: { watch, asVow, when }, // TODO: no when | |||
There was a problem hiding this comment.
no idea; I guess I should look into it
| * | ||
| * @param baseDenom denom on issuing chain | ||
| * @param base issuing chain | ||
| * @param chain holding chain - connection to base chain must be regitered in chainHub |
| * @param chain holding chain - connection to base chain must be regitered in chainHub | ||
| * @returns denom on holding chain | ||
| */ | ||
| getDenomOn: < |
There was a problem hiding this comment.
new top level interface method will need @tribble review
There was a problem hiding this comment.
the name getDenomOn is sort of... uninspired
different arg names might help... orch.getDenomOn(baseDenom, issuingChain, holdingChain)
which suggests orch.getHoldingChainDenom(baseDenom, issuingChain, holdingChain)
| * @returns denom on holding chain | ||
| */ | ||
| getDenomOn: < | ||
| HoldingChain extends keyof KnownChains, |
There was a problem hiding this comment.
not strictly necessary that it's a known chain. but we can tackle that when we have more use of contract-specified chains
There was a problem hiding this comment.
I followed the structure of getBrandInfo(). I guess the comment applies to both?
| const asset = assets.find(a => a.brand === opts.brandOut); | ||
| if (!asset) throw Fail`${opts.brandOut} not registered in Agoric vbank`; |
There was a problem hiding this comment.
SwapExact is in terms of ERTP Amount with Brand, which limits the trade to assets in the Agoric vbank, which makes little sense
| @@ -86,8 +87,6 @@ export interface Chain<CI extends ChainInfo> { | |||
| */ | |||
| makeAccount: () => Promise<OrchestrationAccount<CI>>; | |||
| // FUTURE supply optional port object; also fetch port object | |||
|
|
|||
| // TODO provide a way to get the local denom/brand/whatever for this chain | |||
There was a problem hiding this comment.
getDenomOn() is intended to provide this.
It's on Orchestrator instead of Chain because the Chain implementation doesn't have ready access to chainHub. Perhaps we shouldn't let internal details like that drive the external API, but it was the easiest thing that seems to work.
There was a problem hiding this comment.
intended design is/was:
osmosis.getLocalDenom(baseDemon)
|
Documentation Considerations += let's document that the asset list is captured at some point in time and may trail updates etc... perhaps in / near https://docs.agoric.com/reference/vstorage-ref.html |
Deploying agoric-sdk with
|
| Latest commit: |
82e2040
|
| Status: | 🚫 Build failed. |
refs:
stacked on
Description
The main goal, in support of #8863, is:
to do that, I did:
wait->watchworkorchestration-api.d.ts. Does this even belong there? or should it be in some osmosis-specific file?While I was at it:
Security Considerations
IOU
Scaling Considerations
IOU
Documentation Considerations
see API changes above
Testing Considerations
some noted above
Upgrade Considerations
not yet deployed