feat: auto-stake-it example contract #9666
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0xpatrickdev
commented
Jul 8, 2024
Deploying agoric-sdk with
|
| Latest commit: |
350ff5e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://103035cf.agoric-sdk.pages.dev |
| Branch Preview URL: | https://pc-localchain-monitor-transf.agoric-sdk.pages.dev |
mhofman
reviewed
Jul 9, 2024
0xpatrickdev
force-pushed
the
pc/localchain-monitor-transfers
branch
4 times, most recently
from
f91f3ed
to
8203772
Compare
0xpatrickdev
changed the title
|
Here are logs from the multichain-testing environment. Summary:
SwingSet onlyCombined (cosmos/comet + swingset) |
0xpatrickdev
force-pushed
the
pc/localchain-monitor-transfers
branch
from
8203772
to
3d80104
Compare
0xpatrickdev
commented
Jul 12, 2024
0xpatrickdev
commented
Jul 12, 2024
| { address: address, chainName }, | ||
| ); | ||
| // @ts-expect-error spread argument for concise code | ||
| const txRes = await client.sendIbcTokens(...transferArgs); |
There was a problem hiding this comment.
sendIbcTokens is deprecated, since it can't supply a memo field on the MsgTransfer (just the outer tx). Only using it here since client.simulate() and client.signAndBroadcast() kept yielding:
invalid source port ID: identifier cannot be blank: invalid ident
It would seem like an obvious error to address, but appears to be more of a red herring as sourcePort was definitely provided. I suspected a mismatch between the client registry and the encoder (@agoric/cosmic-proto), but registering a custom messge did not seem to do the trick either. I similarly had no luck with cosmjs-types, so maybe something else.
An approach maybe worth exploring is using the RPC client from telescope via @agoric/cosmic-proto.
There was a problem hiding this comment.
I think this is fine for this PR. Should I prioritize #9200 ?
turadg
reviewed
Jul 12, 2024
| t, | ||
| chainName, | ||
| ); | ||
| await sleep(1500); // wait for wallet to instantiate |
There was a problem hiding this comment.
this comment confuses me because the await above instantiates wallet. What fails without this await?
There was a problem hiding this comment.
The query that reads balances on the next line will fail, typically with a message that says this account doesn't exist yet. I've refactored to use retryUntilCondition and improved the wording.
The current faucet promise seems to resolve when the tx is submitted, not when it lands. See cosmology-tech/starship#417
| { address: address, chainName }, | ||
| ); | ||
| // @ts-expect-error spread argument for concise code | ||
| const txRes = await client.sendIbcTokens(...transferArgs); |
There was a problem hiding this comment.
I think this is fine for this PR. Should I prioritize #9200 ?
| t.truthy(hash.length, 'ibc denom hash found'); | ||
| t.log('ibc denom hash found', hash); | ||
|
|
||
| // 3. Find a osmosis validator to delegate to |
multichain-testing/test/support.ts
Outdated
| export const chainConfig = { | ||
| export type ChainName = keyof typeof chainInfo; | ||
|
|
||
| // TODO DRY with starship-chain-info.js |
0xpatrickdev
commented
Jul 12, 2024
There was a problem hiding this comment.
Thanks for the review, @turadg. Also will respond in waves.
Currently trying to harden the E2E test for CI. Certain edge cases only seem to hit in the initial run, so the iteration loop is a little slow (~8 mins to get everything running fresh).
| { address: address, chainName }, | ||
| ); | ||
| // @ts-expect-error spread argument for concise code | ||
| const txRes = await client.sendIbcTokens(...transferArgs); |
| @@ -0,0 +1,26 @@ | |||
| import { makeHelpers } from '@agoric/deploy-script-support'; | |||
| import { startAutoStakeIt } from '@agoric/orchestration/src/proposals/start-auto-stake-it.js'; | |||
| t, | ||
| chainName, | ||
| ); | ||
| await sleep(1500); // wait for wallet to instantiate |
There was a problem hiding this comment.
The query that reads balances on the next line will fail, typically with a message that says this account doesn't exist yet. I've refactored to use retryUntilCondition and improved the wording.
The current faucet promise seems to resolve when the tx is submitted, not when it lands. See cosmology-tech/starship#417
0xpatrickdev
force-pushed
the
pc/localchain-monitor-transfers
branch
3 times, most recently
from
1090d79
to
360a772
Compare
0xpatrickdev
force-pushed
the
pc/localchain-monitor-transfers
branch
4 times, most recently
from
8daecfa
to
eb938bf
Compare
0xpatrickdev
commented
Jul 16, 2024
Comment on lines
123
to
104
| return watch( | ||
| E(account).asContinuingOffer(), | ||
| this.facets.getInvitationMakersWatcher, | ||
| { action, invitationArgs }, | ||
| ); |
There was a problem hiding this comment.
PortfolioHolderKit is an endowment to the async-flow membrane, so want to make sure it's implemented that correctly.
-
Unlike LOA and COA, we don't need to make remote calls to retrieve public topics - they are already stored in state. This leads me to believe we can just return synchronously for
asContinuingOfferandgetPublicTopics. If this is not the case, we can wrap these inasVow(). -
Action, which ultimately returns an invitation, currently returns a Vow. In LOA and COA, we return promises for invitations since the calls are expected to be prompt (and use vows for the offerHandler). Here, should we be returning a Promise since we expect the remote call to.asContinuingOffer()to resolve promptly?
dckc
reviewed
Jul 16, 2024
| @@ -0,0 +1,200 @@ | |||
| import { M, mustMatch } from '@endo/patterns'; | |||
There was a problem hiding this comment.
@dtribble I think you've asked for this widget a few times
| const { cosmosInterchainService, marshaller, rootZone, timer, vowTools } = | ||
| bootstrap; | ||
|
|
||
| const { makeRecorderKit } = prepareRecorderKitMakers( |
There was a problem hiding this comment.
I really want to push this all the way into the board.
IOU an issue.
| 'PortfolioHolderKit', | ||
| { | ||
| invitationMakers: M.interface('InvitationMakers', { | ||
| Action: M.call(ChainNameM, M.string(), M.arrayOf(M.any())).returns( |
There was a problem hiding this comment.
I think this can be done by composition, but I don't recall the details.
@michaelfig ? @erights ?
If nothing else, add an XXX plz?
There was a problem hiding this comment.
I am lacking some context. What exactly are you indicating by "this" in "this can be done by composition"?
There was a problem hiding this comment.
In the call perparePortfolioHolder, it would be nice to be able to add add'l invitationMakers. This would seem to entail:
- include add'l fields in the interface guard
- include add'l methods in the
invitationMakersfacet
There was a problem hiding this comment.
Timebox expired but hope to revisit soon. Incomplete progress here: 7e886b4
| @@ -208,6 +212,17 @@ export type LocalAccountMethods = { | |||
| deposit: (payment: Payment<'nat'>) => Promise<void>; | |||
| /** withdraw a Payment from the account */ | |||
| withdraw: (amount: Amount<'nat'>) => Promise<Payment<'nat'>>; | |||
| /** | |||
| * Register a handler that receives an event each time ICS-20 transfers are | |||
| * sent or received by the underlying account. Each account may be associated | |||
turadg
reviewed
Jul 16, 2024
| t.log(`${chainName} makeAccount offer`); | ||
| const offerId = `${chainName}-makeAccountsInvitation-${Date.now()}`; | ||
|
|
||
| await doOffer({ |
There was a problem hiding this comment.
this abstraction doesn't seem to carry its weight.
Suggested change
| await doOffer({ | |
| await wallet.offers.executeOffer({ |
There was a problem hiding this comment.
It calls into seatLike, which has a bit of logic to ensure we get payouts. Seems like a bit more than this to inline.
Comment on lines
+139
to
+143
| * XXX consider a facet with a method for changing the validator | ||
| * | ||
| * XXX consider logic for multiple stakingAccounts + denoms |
There was a problem hiding this comment.
just checking that these aren't requirements for MVP (in which case they'd be FIXME)
| * @param {IA} invitationArgs | ||
| * @returns {Promise<Invitation<unknown, IA>>} | ||
| */ | ||
| Action(chainName, action, invitationArgs) { |
There was a problem hiding this comment.
"Action" is very vague to me. this is making an invitation, right?
consider MakeInvitation or ContinueInvitation
0xpatrickdev
force-pushed
the
pc/localchain-monitor-transfers
branch
from
dc65f00
to
92ac64b
Compare
Closed
turadg
approved these changes
Jul 17, 2024
0xpatrickdev
force-pushed
the
pc/localchain-monitor-transfers
branch
from
2045ec8
to
5226842
Compare
- pulls out chaib-hub creatorFacet for reuse with other contracts
- PortfolioHolder is a kit that holds multiple OrchestrationAccounts and returns a single invitationMaker and TopicRecord - the Action invitationMaker is designed to pass through calls to invitationMakers from sub-accounts, keyed by chainName - refs #9042, which requires multiple accounts in a single user offer flow
- adds a VTransferIBCEvent type, for acknowledgementPacket and writeAcknowledgement events
- monitorTransfers allows calls to register a handler to react to incoming and outgoing IBC ics20 transfers - includes VTRANSFER_IBC_EVENT<'acknowledgementPacket'> fixture from observed values in a multichain testing environment (see PR comments for logs) - refs: #9042
- creates an example contract that user .monitorTransfers to react to an incoming IBC transfer. when the transfer is received, it's sent to an ICA then delegated. Both accounts are put in a PortfolioHolder kit, which combines ContinuingOfferResults into a single record - includes logic to ignore outgoing transfers, uknown denoms, and unkown sourceChannels - does not include logic to look for a specific value in the transfer memo field, but this could be added - refs: #9042
- until #9643, `doOffer` should return a Promise. Then, it can return an offerResult
- ava logs do not appear until the end of a test. this is a scenarios where it's more useful to see the logs as they happen
0xpatrickdev
force-pushed
the
pc/localchain-monitor-transfers
branch
from
5226842
to
15e50f0
Compare
0xpatrickdev
added
automerge:rebase
0xpatrickdev
force-pushed
the
pc/localchain-monitor-transfers
branch
from
15e50f0
to
e1cc43a
Compare
- helper for building an ibc transfer message with @cosmjs/stargate signing client - hardcodes timeoutTimestamp until #9200, as we experienced weird flakes in CI
- adds portofolio holder to basic-flows.contract.js and tests wallet offers in bootstrap environment
0xpatrickdev
force-pushed
the
pc/localchain-monitor-transfers
branch
from
e1cc43a
to
350ff5e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refs: #9042
refs: #9066
refs: #9193
Description
LocalOrchestrationAccountwithmonitorTransfers(vtransfer) method to register handlers for incoming and outgoing ICS20 transfers.writeAcknowledgement(ability to confer acknowledgement or acknowledgement error to sending chain) capability is not exposed through the current orchestration api. users must work withregisterActiveTapfromtransfer.jsfor this capability.auto-stake-itcontract that uses .monitorTransfers to react to incoming IBC transfers, delegating them via an InterchainAccount (ICA).PortfolioHolderkit, combiningContinuingOfferResultsfrom multipleOrchestrationAccountsinto a single record.acknowledgementPacketandwriteAcknowledgementevents and an example mock for unit testingDocumentation
Testing Considerations