Skip to content
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

test(fast-usdc): bootstrap test for advancement #10606

Merged
merged 4 commits into from
Dec 4, 2024
Merged

Conversation

samsiegart
Copy link
Contributor

@samsiegart samsiegart commented Dec 2, 2024

refs #10511

Does not count computrons yet.

This makes 5 oracles submit evidence to provide a realistic scenario for measurement.

@samsiegart samsiegart requested a review from a team as a code owner December 2, 2024 23:21
Copy link

cloudflare-workers-and-pages bot commented Dec 2, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: e0d5e94
Status: ✅  Deploy successful!
Preview URL: https://78f79b41.agoric-sdk.pages.dev
Branch Preview URL: https://srs-advance-test.agoric-sdk.pages.dev

View logs

Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather see 3 oracle operators and no makeTestPushInvitation. What do you think?

Comment on lines 159 to 164
const oracles = await Promise.all([
wd.provideSmartWallet('agoric144rrhh4m09mh7aaffhm6xy223ym76gve2x7y78'),
wd.provideSmartWallet('agoric19d6gnr9fyp6hev4tlrg87zjrzsd5gzr5qlfq2p'),
wd.provideSmartWallet('agoric19uscwxdac6cf6z7d5e26e0jm0lgwstc47cpll8'),
wd.provideSmartWallet('agoric1krunjcqfrf7la48zrvdfeeqtls5r00ep68mzkr'),
wd.provideSmartWallet('agoric1n4fcxsnkxe4gj6e24naec99hzmc4pjfdccy5nj'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I gather we're more likely to have 3 in prod.

Is now a good time to update the builder script?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well... the builder script still has a TODO to determine the real addresses but I just arbitrarily removed two of them for now.

source: 'agoricContract',
instancePath: ['fastUsdc'],
callPipe: [
['makeTestPushInvitation', [MockCctpTxEvidences.AGORIC_PLUS_DYDX()]],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to get rid of makeTestPushInvitation soon. Is it important to use it here? What are your thoughts on when to stop using it?

I see you're using the real continuing invitation API below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this test altogether since we check vstorage in the advancement test too now

);
await eventLoopIteration();

const usdc = agoricNamesRemotes.vbankAsset.USDC.brand;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const usdc = agoricNamesRemotes.vbankAsset.USDC.brand;
/** @type {Brand<'nat'>} */
const usdc = agoricNamesRemotes.vbankAsset.USDC.brand;

if necessary, add a @ts-expect-error here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done with as (since it's a ts file)

},
proposal: {
give: {
// @ts-expect-error it doesnt recognize usdc as a Brand type
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to fix the type of usdc where it's defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@samsiegart samsiegart requested a review from dckc December 3, 2024 00:30
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good.

@samsiegart samsiegart added the automerge:squash Automatically squash merge label Dec 3, 2024
}),
),
);
await eventLoopIteration();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth simulating the acknowledgement from the vtransfer bridge?

 await runInbound(
    BridgeId.VTRANSFER,
    buildVTransferEvent({
    ...
    }),
  )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm good point, I think this would happen in a different block, so maybe measure it separately, but should be counted nonetheless. I can follow up with this.

mergify bot added a commit that referenced this pull request Dec 3, 2024
closes: #10388
refs: #10511

## Description

Prune `testBorrow`, `testRepay` methods along with contract tests that depend on them.

 - Several tests are largely covered by share-pool-math tests
   - 1 was not, so I made a pool-share-math test to cover _repay succeeds with no Pool or Contract Fee_.
 - Several tests basically checked the interface guards of `lp.borrower.borrow` / `lp.repayer.repay`.
    These are internal APIs with static types.
   - testing consistency between interface guards and static types
     might have some value, but, I suggest, not enough for the cost

### Scaling / Documentation / Upgrade Considerations

none

### Security / Testing Considerations

small loss in test coverage - mostly in test redundancy

`makeTestPushInvitation` remains on the public facet. Getting rid of it in due course remains critical.
I expect / hope we can get rid of it in #10606 (cc @samsiegart ) .

Ideally, the liquidity-pool exo would have unit test coverage. I looked into that but found that I would have to build substantial ZCF / Zoe test tooling. Since `liquidity-pool.js` is just 360 lines of straightforward Zoe API usage, I suggest we postpone that under...

 - #10558
@mergify mergify bot merged commit ce195ab into master Dec 4, 2024
81 checks passed
@mergify mergify bot deleted the srs-advance-test branch December 4, 2024 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants