-
Notifications
You must be signed in to change notification settings - Fork 226
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: zoeTools.localTransfer error paths #9902
Conversation
Deploying agoric-sdk with
|
Latest commit: |
ea73ef7
|
Status: | ✅ Deploy successful! |
Preview URL: | https://cf7f029a.agoric-sdk.pages.dev |
Branch Preview URL: | https://pc-send-anywhere-fixups.agoric-sdk.pages.dev |
f78755c
to
c537e65
Compare
7c43470
to
e571565
Compare
08ac3f2
to
97c708a
Compare
97c708a
to
7cec00d
Compare
ff9c7d0
to
bf74f48
Compare
e1b5f2e
to
891b473
Compare
2ceca8a
to
e661353
Compare
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 work. I've only done a quick reading pass. I'll come back for more thorough logic checking
paymentsKwr, | ||
); | ||
console.debug(depositResponse); | ||
throw Fail`One or more deposits to LCA failed. ${q(errors)}`; |
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.
because Fail
throws, I think we're developing some confusion by using throw Fail
. WDYT of only using Fail for the check || Fail
assertion pattern?
throw Fail`One or more deposits to LCA failed. ${q(errors)}`; | |
throw makeError`One or more deposits to LCA failed. ${q(errors)}`; |
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 onboard! This makes things clearer.
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.
Do you have a preference for makeError('One or more deposits to LCA failed. ${q(errors)}')
vs makeError('One or more deposits to LCA failed', undefined, { errors }
?
I've implemented the latter since that's what the interface was guiding towards. A draw back is that the nested error(s) only appears in logs.
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 usually defer to @Chris-Hibbert on error elision
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 think this is fine. I wouldn't expect the errors
to reveal any sensitive details, and we believe we've returned the funds to the originator.
How does the end user become aware of the problem? Or if this is a tool for implementors to use, how can they detect the problem?
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'm beginning to think @turadg misdirected this question to me, and meant @kriskowal. I don't know much about makeError
--that's deep inside endo.
we anticipate some level of error reporting in vstorage
that sounds very helpful.
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 meant you @Chris-Hibbert because I recall your attention to redaction in Inter Protocol
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 see a problem 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.
I think that throw makeError
is more theoretically sound, but throw Fail
has the same effect (the throw
is unreachable because Fail
throws, that subsequent code is unreachable), and provides a more obvious, local indication that the following code is unreachable.
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.
throw Fail
has the same effect here but not always. I think we should avoid using throw Fail
,
3f568f1
to
a33514c
Compare
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.
comments so far.
I may be away for an unknown period this afternoon, so sending what I have now.
for (const chainName of ['agoric', 'cosmoshub']) { | ||
chainHub.registerChain(chainName, fetchedChainInfo[chainName]); | ||
} | ||
for (const brand of values(zcf.getTerms().brands)) { |
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 complain if no infos are found? Wouldn't it normally be a surprise if none were found? Maybe it would be helpful to return a count of the number that were found?
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.
In a production-grade example, I agree this could be improved with more helpful messaging. But since this is just a test fixture, some checks to catch errors earlier are skipped.
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 a @file
comment that this is not exemplary and takes shortcuts to support testing
@@ -63,21 +80,102 @@ export const makeZoeTools = (zone, { zcf, vowTools }) => { | |||
// ); | |||
|
|||
// Now all the `give` are accessible, so we can move them to the localAccount` | |||
const payments = await Promise.all( | |||
keys(give).map(kw => E(userSeat).getPayout(kw)), |
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.
There can be payouts that aren't in give
. Where are those handled?
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.
give
is maybe a poor variable name - I didn't choose to rename it with these changes but can. give
is a PaymentKeywordRecord
so all payments should be accounted for.
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.
paid
?
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 misspoke, it's an AmountKeywordRecrod
not a PaymentKeywordRecord
. What about amounts
? Alternatively, we could use fromAmounts
here and toAmounts
for withdrawToSeat
, mimicking atomicRearrange
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.
There aren't two seats here, so amounts
is fine.
AIUI, withdrawToSeat
may have two seats, but the same amounts are transferred from and to, so amounts
works there as well.
* `srcLocalAccount`. Supports multiple items in the `amounts` | ||
* {@link PaymentKeywordRecord}. | ||
*/ | ||
const withdrawToSeat = retriable( |
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 like to see a test of this. (I didn't miss it, did I?)
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.
There are a few in /test/utils/zoe-tools.test.ts
:
- 'withdraw (withdrawToSeat) from LCA with insufficient balance'
- 'withdraw (withdrawToSeat) happy path'
- 'withdrawToSeat, unknown brand'
- and near 'withdrawToSeat recovers from: simulated sendAll failure' in 'zoeTool.localTransfer error path' (
withdrawSeat
is used bylocalTransfer
to recover)
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.
oh, sorry. hidden by large diffs not rendered by default
const amt = await E(zoe).getInvitationDetails(inv); | ||
t.is(amt.description, 'send'); | ||
|
||
const anAmt = ist.make(SIMULATED_ERRORS.TIMEOUT); |
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.
That's distracting. There's nothing error-ish about this use, AFAICT. I'd prefer
const anAmt = ist.make(SIMULATED_ERRORS.TIMEOUT); | |
const arbitraryNat = 37n; | |
const anAmt = ist.make(arbitraryNat); |
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.
We've been using the SIMULATED_ERRORS
constant to make it clear we are intentionally throwing an error in the test:
agoric-sdk/packages/vats/tools/fake-bridge.js
Lines 171 to 180 in 66e4cc0
/** | |
* Constants that can be used to force an error state in a bridge transaction. | |
* Typically used for the LocalChainBridge which currently accepts all messages | |
* unless specified otherwise. Less useful for the DibcBridge which rejects all | |
* messages unless specified otherwise. | |
*/ | |
export const SIMULATED_ERRORS = { | |
TIMEOUT: 504n, | |
BAD_REQUEST: 400n, | |
}; |
The IBC bridge (used here) isn't trained to throw on these magic constants - it throws if it receives a message that doesn't have a handler registered via addMockAck
. But the Localchain
bridge is trained to do this, so I think consistently adopting this practice makes intentional errors clearer.
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.
But the Localchain bridge is trained to do this,
It throws when it sees Amounts with these magic values? It can be difficult to inject errors, so I'll go along with this.
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.
Yes, see fakeLocalChainBridgeTxMsgHandler
:
agoric-sdk/packages/vats/tools/fake-bridge.js
Lines 193 to 228 in c1c5a6f
export const fakeLocalChainBridgeTxMsgHandler = (message, sequence) => { | |
switch (message['@type']) { | |
// TODO #9402 reference bank to ensure caller has tokens they are transferring | |
case '/ibc.applications.transfer.v1.MsgTransfer': { | |
if (message.token.amount === String(SIMULATED_ERRORS.TIMEOUT)) { | |
throw Error('simulated unexpected MsgTransfer packet timeout'); | |
} | |
// like `JsonSafe<MsgTransferResponse>`, but bigints are converted to numbers | |
// FIXME should vlocalchain return a string instead of number for bigint? | |
return { | |
sequence, | |
}; | |
} | |
case '/cosmos.bank.v1beta1.MsgSend': { | |
if (message.amount[0].amount === String(SIMULATED_ERRORS.BAD_REQUEST)) { | |
throw Error('simulated error'); | |
} | |
return /** @type {JsonSafe<MsgSendResponse>} */ ({}); | |
} | |
case '/cosmos.staking.v1beta1.MsgDelegate': { | |
if (message.amount.amount === String(SIMULATED_ERRORS.TIMEOUT)) { | |
throw Error('simulated packet timeout'); | |
} | |
return /** @type {JsonSafe<MsgDelegateResponse>} */ ({}); | |
} | |
case '/cosmos.staking.v1beta1.MsgUndelegate': { | |
return /** @type {JsonSafe<MsgUndelegateResponse>} */ ({ | |
// 5 seconds from unix epoch | |
completionTime: { seconds: '5', nanos: 0 }, | |
}); | |
} | |
// returns one empty object per message unless specified | |
default: | |
return {}; | |
} | |
}; |
It can be difficult to inject errors
+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.
Thanks @Chris-Hibbert ! I too will be afk and plan to revisit this tomorrow AM
* `srcLocalAccount`. Supports multiple items in the `amounts` | ||
* {@link PaymentKeywordRecord}. | ||
*/ | ||
const withdrawToSeat = retriable( |
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.
There are a few in /test/utils/zoe-tools.test.ts
:
- 'withdraw (withdrawToSeat) from LCA with insufficient balance'
- 'withdraw (withdrawToSeat) happy path'
- 'withdrawToSeat, unknown brand'
- and near 'withdrawToSeat recovers from: simulated sendAll failure' in 'zoeTool.localTransfer error path' (
withdrawSeat
is used bylocalTransfer
to recover)
const amt = await E(zoe).getInvitationDetails(inv); | ||
t.is(amt.description, 'send'); | ||
|
||
const anAmt = ist.make(SIMULATED_ERRORS.TIMEOUT); |
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.
We've been using the SIMULATED_ERRORS
constant to make it clear we are intentionally throwing an error in the test:
agoric-sdk/packages/vats/tools/fake-bridge.js
Lines 171 to 180 in 66e4cc0
/** | |
* Constants that can be used to force an error state in a bridge transaction. | |
* Typically used for the LocalChainBridge which currently accepts all messages | |
* unless specified otherwise. Less useful for the DibcBridge which rejects all | |
* messages unless specified otherwise. | |
*/ | |
export const SIMULATED_ERRORS = { | |
TIMEOUT: 504n, | |
BAD_REQUEST: 400n, | |
}; |
The IBC bridge (used here) isn't trained to throw on these magic constants - it throws if it receives a message that doesn't have a handler registered via addMockAck
. But the Localchain
bridge is trained to do this, so I think consistently adopting this practice makes intentional errors clearer.
@@ -63,21 +80,102 @@ export const makeZoeTools = (zone, { zcf, vowTools }) => { | |||
// ); | |||
|
|||
// Now all the `give` are accessible, so we can move them to the localAccount` | |||
const payments = await Promise.all( | |||
keys(give).map(kw => E(userSeat).getPayout(kw)), |
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.
give
is maybe a poor variable name - I didn't choose to rename it with these changes but can. give
is a PaymentKeywordRecord
so all payments should be accounted for.
for (const chainName of ['agoric', 'cosmoshub']) { | ||
chainHub.registerChain(chainName, fetchedChainInfo[chainName]); | ||
} | ||
for (const brand of values(zcf.getTerms().brands)) { |
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.
In a production-grade example, I agree this could be improved with more helpful messaging. But since this is just a test fixture, some checks to catch errors earlier are skipped.
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.
Thanks for letting me see this. I don't see anything objectionable. I'll leave it to others with more context to approve.
* `srcLocalAccount`. Supports multiple items in the `amounts` | ||
* {@link PaymentKeywordRecord}. | ||
*/ | ||
const withdrawToSeat = retriable( |
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.
oh, sorry. hidden by large diffs not rendered by default
} | ||
} | ||
t.log('withdrawToSeat recovers from: simulated sendAll failure '); | ||
{ |
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's the benefit of putting multiple apparently separate tests into a single ava test run? The usual guideline is that independent tests should have separate test names, so their failures can be reported individually.
The usual exceptions are that execution must be in a particular order, or that setup is expensive. Neither seems to be the case 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.
setup is expensive
This was the main reasoning, for the 6 nested conditions in this file I think it'd be an extra ~90 LOC. We haven't been diligent about enforcing 1 test assertion in 1 test run in this package, but usually use the optional 3rd message
argument to help report the failure.
82c4902
to
2bf7f89
Compare
6273cb1
to
92821e9
Compare
@@ -63,21 +80,102 @@ export const makeZoeTools = (zone, { zcf, vowTools }) => { | |||
// ); | |||
|
|||
// Now all the `give` are accessible, so we can move them to the localAccount` | |||
const payments = await Promise.all( | |||
keys(give).map(kw => E(userSeat).getPayout(kw)), |
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.
There aren't two seats here, so amounts
is fine.
AIUI, withdrawToSeat
may have two seats, but the same amounts are transferred from and to, so amounts
works there as well.
/** | ||
* Transfer the `give` a seat to a local account. | ||
* Transfer the `give` of a seat to a local account. If any of the deposits |
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.
* Transfer the `give` of a seat to a local account. If any of the deposits | |
* Transfer the `give` amounts from srcSeat to localAccount. If any of the deposits |
or after renaming,
* Transfer the `give` of a seat to a local account. If any of the deposits | |
* Transfer the amounts from srcSeat to localAccount. If any of the deposits |
@@ -63,21 +80,106 @@ export const makeZoeTools = (zone, { zcf, vowTools }) => { | |||
// ); | |||
|
|||
// Now all the `give` are accessible, so we can move them to the localAccount` | |||
const payments = await Promise.all( |
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.
update the comment above.
947b150
to
c3babe6
Compare
## Description - adds vowTools helper that mimics the behavior of `Promise.allSettled` to support #9902 - tests unstorable results path which resulted in a type guard change ### Security Considerations n/a ### Scaling Considerations n/a ### Documentation Considerations Updates `vow/README.md` with information about `VowTools` ### Testing Considerations Includes unit tests ### Upgrade Considerations n/a, library code
Base branch is changed to master. Please re-run the integration tests by adding 'force:integration' label. |
30a3f27
to
6a8aebc
Compare
); | ||
} catch (e) { | ||
await withdrawToSeat(contractState.localAccount, seat, give); | ||
throw seat.fail(makeError(`IBC Transfer failed ${q(e)}`)); |
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.
seat.fail()
returns nothing. I see that our docs suggest this, but it doesn't make any sense to me.
I see the pattern in some of our example contracts, but not our production contracts.
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 would you expect to see instead?
With:
seat.fail(makeError(`IBC Transfer failed ${q(e)}`));
return;
I see Promise resolved with undefined
.
With only:
seat.fail(makeError(`IBC Transfer failed ${q(e)}`));
Execution continues and the seat.exit()
a few lines down causes a rejection with Error: seat has been exited
.
I'm using yarn test test/examples/send-anywhere.test.ts -m 'failed ibc transfer returns give'
to hit this path.
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.
Another option considered a while back was:
seat.exit();
throw makeError(`IBC Transfer failed ${q(e)}`)
but this seemed like an anti-pattern since seat.fail
exists
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 have gone with
const errorMsg = `IBC Transfer failed ${q(e)}`;
seat.exit(errorMsg);
throw makeError(errorMsg);
!srcSeat.hasExited() || Fail`The seat cannot have exited.`; | ||
const { zcfSeat: tempSeat, userSeat: userSeatP } = zcf.makeEmptySeatKit(); | ||
const userSeat = await userSeatP; | ||
atomicTransfer(zcf, srcSeat, tempSeat, give); | ||
atomicTransfer(zcf, srcSeat, tempSeat, amounts); |
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.
atomicTransfer(zcf, srcSeat, tempSeat, amounts); | |
zcf.atomicTransfer(srcSeat, tempSeat, amounts); |
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.
Please see c5ada2f
/** @typedef {ReturnType<typeof makeZoeTools>} ZoeTools */ | ||
|
||
/** @typedef {ReturnType<typeof makeZoeTools>} ZoeToolsHost */ | ||
/** @typedef {GuestInterface<ZoeToolsHost>} ZoeTools */ |
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.
are we always going to have the unqualified name be the guest? I don't recall if we've made that decision.
Consider not defining ZoeTools
and letting the guest places use GuestInterface<ZoeToolsHost>
export const makeZoeTools = (zcf, vowTools) => { | ||
const { when, allVows, allSettled, asVow } = vowTools; |
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.
why delay the destructuring? It allows for a mixing of call styles in the same closure.
return asVow((
return vowTools.aVow(
await null; | ||
const withdrawToSeat = (localAccount, destSeat, amounts) => | ||
/* | ||
* There are no interchain actions - everything happens on the local chain - |
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.
this explanation is helpful but strikes me as something to document and reference. (It's repeated twice here and surely there are other places equally worth noting it.)
Not a blocker for merge but something we should improve
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.
Agree, moved to the @file
declaration
bb41c47
to
2cad888
Compare
- retriable that withdraws payments (described by an IssuerKeywordRecord) from a LCA to a user seat
2cad888
to
99f89dc
Compare
- `retriable` does not seem to be a good fit for these functions as they are not idempotent
99f89dc
to
ea73ef7
Compare
closes: #9925
refs: #9193
Description
zoeTools
implementation for orchestrated async-flows by:zoeTools.localTransfer
are recovered to the original seat in the event of failure or partial failurezoeTools.withdrawFromSeat
retriable function to facilitate withdrawing funds from a LocalChainAccount to a User seat. Ensures all changes are rolled back in the event of partial failure.asVow
instead ofretriable
since these operations are not idempotentsrc/fixtures/zoe-tools.contract.js
to facilitate testing error pathsvowTools.allVowsSettled
was implemented in@agoric/vow
. See feat:vowTools.allSettled
#10077 which this PR depends onSecurity Considerations
Improves safety around Payment handling in async-flow contracts in
@agoric/orchestration
Scaling Considerations
n/a
Documentation Considerations
Updates jsdoc strings for
localTransfer
andwithdrawToSeat
. As maintainer notes for considerations aroundasVow
and promptness.Testing Considerations
Includes tests with different failure paths previously unaccounted for.
Upgrade Considerations
Unreleased code that will be part of an npm release