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

migrate/improve auction functionality tests in z:acceptance #10229

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

anilhelvaci
Copy link
Collaborator

closes: https://github.com/Agoric/BytePitchPartnerEng/issues/8
closes: #10111

Description

Currently acceptance tests do not cover auction functionality. When we look at the proposals in https://github.com/Agoric/agoric-3-proposals, there are no tests that check for depositors or bidders payouts from a given auction. There is code for changing auction params here and there, including z:acceptance.

In this PR, we test an auction from start to end. Here's the test case implemented;

  • In USE phase of n:upgrade-next, we place a bid
  • Start executing TEST phase of z:acceptance
  • We push a price for ATOM
  • gov1 deposits 100 IST to ATOM auction book
  • user1 places a bid
  • gov2 places a bid
  • Auction round ends
  • Depositor gets correct payouts
  • All bidders including the one placed in an earlier proposal get correct payouts

Why is this PR a draft?

This pr depends on tools in #10171, as soon that's approved I'll rebase this one on top of that one.

Security Considerations

None.

Scaling Considerations

Currently this test takes from 4-5 minutes to 12-13 minutes. It can take up to 20 minutes. This is because how the auction params are set. Why not just change the auction params? See Testing Considerations.

Documentation Considerations

None.

Testing Considerations

Testing auctions in a3p have a huge disadvantage since we don't have any tools for controlling how the time advances when running an actual node. I believe Agoric/agoric-3-proposals#179 should be a priority as it will unlock very comprehensive test coverage in a3p testing.

Since time is a variable that we cannot control but it also plays a huge role when running our auction tests, we need to wait for it. Another complication is coming from how a3p images are built. Imagine this scenario;

  • chain stopped at t - 3
  • when the chain stopped, nextStartTime of the auction schedule was t - 1
  • auction tests started the chain at t
  • when chain started at t the waker set for the round t - 1 is triggered before our run-test.sh is run
  • the callback of the waker captured the prices then tried to start the round at t - 1 when it is actually at t
  • scheduler realized this and rescheduled the round at t - 1 to start at t + 1
  • round at t + 1 is now the activeStartTime but there's a +1 delay that we have to wait for
  • our test starts depositing and bidding after the price we set is captured by the auctioneer
  • price for activeStartTime is already captured when the t - 1 waker run during the fast forward when chain restarted
  • we need the auction to run with the price we set so we can make sure the long living bid is ended with correct payouts
  • auctioneer will capture new prices at newStartTime
  • so the soonest we can start running out test is at newStartTime
  • we have to wait for +1 (assuming we are still at t) PLUS a whole auction round

This scenario is common when yarn test -m acceptance uses caching as the latest cached layer will have it's state at when it was built. So if you built your latest cached layer an hour ago and auction start frequency is 10 minutes, you will face this issue.

If the latest cached layer is built less than 15 mins ago (10 mins round time and 5 mins maxLate, auctioneer is okay if round starts late less than 5 mins), it is very unlikely that you will face this issue("unlikely" because my math there might be wrong.) So I assume we will not face this issue when running in CI as I think all build layers are built from scratch in CI.

Why not update auction params?

Changed params take effect AFTER the next scheduled round. We are only interested in the next scheduled round since it is the soonest we can start depositing and bidding. One way to make the changed params effective for the next scheduled round in our test-acceptance build might be to change them in an earlier proposal USE state which means we have to depend on the global state to carry out our tests which is something I'm not a fan of.

Who places the long living bid?

Again, in order not to depend on global state we create a new user called long-living-bidder. Because other tests might mess with the bidder's wallet history and make querying payouts more complex.

Upgrade Considerations

None.

@anilhelvaci anilhelvaci requested a review from a team as a code owner October 5, 2024 03:10
@anilhelvaci anilhelvaci marked this pull request as draft October 5, 2024 03:10
@Chris-Hibbert Chris-Hibbert added the force:integration Force integration tests to run on PR label Oct 7, 2024
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

I'll fully review when it's out of draft

a3p-integration/proposals/n:upgrade-next/package.json Outdated Show resolved Hide resolved
Copy link

cloudflare-workers-and-pages bot commented Oct 8, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9901261
Status: ✅  Deploy successful!
Preview URL: https://8f527361.agoric-sdk.pages.dev
Branch Preview URL: https://anil-auction-tests.agoric-sdk.pages.dev

View logs

Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

I'll fully review when it's out of draft

…st time related complexities

fix: apply change requests from PR review, resolve rebase conflicts, style fixes

* change `performAction.js` name to `submitBid.js`
* remove `Math.round` from `scale6`
* update indexing of bids and other constants in `config` object to improve readability (`auction.test.js`)
* move helper functions in `auction.test.js` to `test-lib/auction-lib.js`
* move `lib/vaults.mts` to `test-lib/vaults.mts`  and remove empty `lib` directory
* let it be known `sync-tools.js` is a stand-in code for #10171

Refs: Agoric/BytePitchPartnerEng#8

fix: style fixes

fix(acceptance-auction): lint fixes
…llision

Refs: Agoric/BytePitchPartnerEng#31

fix(auction-acceptance): formatting fixes

fix(auction-acceptance): formatting fixes
@anilhelvaci anilhelvaci marked this pull request as ready for review October 30, 2024 13:22
a3p-integration/proposals/n:upgrade-next/submitBid.js Outdated Show resolved Hide resolved
a3p-integration/proposals/n:upgrade-next/submitBid.js Outdated Show resolved Hide resolved
a3p-integration/proposals/z:acceptance/auction.test.js Outdated Show resolved Hide resolved
const [gov1Results, longLivingBidResults, user1Results, gov3Results, brands] =
await Promise.all([
agoric
.follow(
Copy link
Member

Choose a reason for hiding this comment

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

I lament that we still have to go out through the CLI to get this functionality that is in JS:

Agoric/agoric-3-proposals#98 is related.

Filed #10369

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Definitely agree

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
force:integration Force integration tests to run on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

acceptance test of bids surviving upgrades
3 participants