invoke ymax resolver #12182
invoke ymax resolver #12182
Conversation
dckc
left a comment
dckc
left a comment
There was a problem hiding this comment.
To demonstrate that this is a compatible change, I'd rather keep all old tests in place.
| import { TransactionSettlementOfferArgsShape } from '../src/resolver/types.ts'; | ||
| import { deploy } from './contract-setup.ts'; | ||
|
|
||
| test('CCTP settlement invitation - no pending transaction found', async t => { |
There was a problem hiding this comment.
To demonstrate that this is a compatible change, I'd rather leave the old test in place and add a test that uses invoke.
There was a problem hiding this comment.
All of this test was recapitulated by the CCTP confirmation invitation exits seat properly test, which does extra checks, and was kept in place.
| : undefined; | ||
|
|
||
| const resolverMakers = await getResolverMakers(zoe, started.creatorFacet); | ||
| const resolverService = await getResolverService(started.creatorFacet); |
There was a problem hiding this comment.
it's fine to use only the new interface here
|
|
||
| const settleId = Date.now().toString(); | ||
| t.log('Executing CCTP settlement offer'); | ||
| await wallet.executeOffer({ |
There was a problem hiding this comment.
again, I'd prefer to keep the old test in place and only add a new one. would that be a huge hassle?
There was a problem hiding this comment.
These tests are skipped and broken: the offer args don't match shape and there is no transaction set to resolve. Keeping the old test doesn't do anything.
There was a problem hiding this comment.
I refactored the existing tests to exercise as much as possible the non-broken functionality in non-skipped test, showing that switching doesn't affect the outcome of tests.
| }); | ||
|
|
||
| test.serial( | ||
| 'CCTP settlement with old invitation doesnt work with new contract instance', |
There was a problem hiding this comment.
deleting this test suggests a backwards-incompatible change
There was a problem hiding this comment.
This test is being deleted because its throwsAsync assertion is satisfied for unrelated reasons.
I can try to fix it if you prefer, but I'd have to mark it as skipped because it cannot run if the resolver isn't delivered (either to the wallet store, or as a continuing invitation), because of a previously skipped test.
There was a problem hiding this comment.
I restored and fixed the previously deleted test showing exactly the nature of the expected error.
mhofman
left a comment
mhofman
left a comment
There was a problem hiding this comment.
To demonstrate that this is a compatible change, I'd rather keep all old tests in place.
The runtime change itself is purely additive. There is no working test of the resolver standalone, it's all tested indirectly through the resolver-helpers.ts used in a few of the portfolio contract's tests. I don't think there is a way to test both paths besides duplicating all these tests.
| import { TransactionSettlementOfferArgsShape } from '../src/resolver/types.ts'; | ||
| import { deploy } from './contract-setup.ts'; | ||
|
|
||
| test('CCTP settlement invitation - no pending transaction found', async t => { |
There was a problem hiding this comment.
All of this test was recapitulated by the CCTP confirmation invitation exits seat properly test, which does extra checks, and was kept in place.
|
|
||
| const settleId = Date.now().toString(); | ||
| t.log('Executing CCTP settlement offer'); | ||
| await wallet.executeOffer({ |
There was a problem hiding this comment.
These tests are skipped and broken: the offer args don't match shape and there is no transaction set to resolve. Keeping the old test doesn't do anything.
| }); | ||
|
|
||
| test.serial( | ||
| 'CCTP settlement with old invitation doesnt work with new contract instance', |
There was a problem hiding this comment.
This test is being deleted because its throwsAsync assertion is satisfied for unrelated reasons.
I can try to fix it if you prefer, but I'd have to mark it as skipped because it cannot run if the resolver isn't delivered (either to the wallet store, or as a continuing invitation), because of a previously skipped test.
mhofman
force-pushed
the
mhofman/invoke-resolver
branch
from
f66687f to
d794090
Compare
mhofman
left a comment
mhofman
left a comment
There was a problem hiding this comment.
I refactored the portfolio.test.ts file to better show that this change doesn't fundamentally affect how the resolver interactions work.
I also reworked the resolver helpers to support both continuous invitations and wallet invocations. The former is used in the ymax-scenario.test.ts tests, and the latter is used in the other tests that rely on contract-setup.ts. That way both approaches remain under test, and it'll be easy to switch once we deprecate / remove the continuous invitations for the resolver.
I force pushed to make the initial refactor more clear.
I recommend reviewing commit-by-commit and with whitespace differences ignored.
|
|
||
| const settleId = Date.now().toString(); | ||
| t.log('Executing CCTP settlement offer'); | ||
| await wallet.executeOffer({ |
There was a problem hiding this comment.
I refactored the existing tests to exercise as much as possible the non-broken functionality in non-skipped test, showing that switching doesn't affect the outcome of tests.
| }); | ||
|
|
||
| test.serial( | ||
| 'CCTP settlement with old invitation doesnt work with new contract instance', |
There was a problem hiding this comment.
I restored and fixed the previously deleted test showing exactly the nature of the expected error.
dckc
left a comment
dckc
left a comment
There was a problem hiding this comment.
my eyes are far from fresh on this PR
@gibson042 if you have a moment to look it over, that might be nice
| }: ResolveTxParams) => { | ||
| !proposal || Fail`Unexpected proposal ${q(proposal)}`; | ||
|
|
||
| const offerArgs = { |
There was a problem hiding this comment.
offerArgs seems like an odd name. There's no offer. might as well inline this record.
There was a problem hiding this comment.
Yeah the type is still TransactionSettlementOfferArgs for this, which is awkward, but I didn't change that part of the "code".
Happy to rename.
closes: #11966
refs: #11746, #10741
Description
Using the continuing invitation mechanism to settle cctp transactions is expensive (see #10741 for the oracle analysis). This PR introduces a mechanism to be able to transition the resolver from continuing invitations to smart-wallet based invocations as introduced in #11746.
Security Considerations
None, this doesn't change the model that the ability to submit settlement info is tied to capabilities held by a smart wallet.
Scaling Considerations
Reduce overhead of submitting settlement info
Documentation Considerations
None
Testing Considerations
Switched the existing tests to direct invocation. Some of the tests are skipped (and outdated), but I locally verified that the changes work as far these tests did before with the invitation based mechanism.
Upgrade Considerations
There is not a good cheap way to detect whether there is a capability saved in the smart wallet store, so while the planner includes the implementation to use the invocation mechanism, it doesn't switch to it. Switching will require a follow-on change once all deployed contracts have switched to the supporting direct resolver invocations.