PR for CI test #2
Open
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
anilhelvaci
force-pushed
the
anil/auction-tests
branch
2 times, most recently
from
65bfdc0 to
b29d36e
Compare
anilhelvaci
force-pushed
the
anil/auction-tests
branch
2 times, most recently
from
f1dad49 to
a89e8e2
Compare
…0241) closes: Agoric#10138 closes: Agoric#10185 closes: Agoric#10258 ## Description A3P tests for the replace electorate core eval. also adds the replace-electorate-core.js script to upgrade.go since it will now be a part of the chain halting upgrade new tests are as follows: - adds new `n:replace-electorate` which tests for acceptance of new invitations - adds params governance proposal and voting tests to `z:acceptance` ### Security Considerations ### Scaling Considerations ### Documentation Considerations ### Testing Considerations ### Upgrade Considerations
refs: Agoric#10267 ## Description We'll be upgrading Zoe in the next SW upgrade. We need to reset the KREAd subscribers when we do that. ### Security Considerations No broader security implications ### Scaling Considerations Not a scaling concern ### Documentation Considerations I should have remembered this when adding the Zoe upgrade. ### Testing Considerations None ### Upgrade Considerations There's a constraint that we need to enforce when we upgrade Zoe. So far, we've been remembering it, but it would be better to automate.
…ic#10306) closes: Agoric#10274 ## Description Half the EC were able to pass a motion, when a majority should have been required. ### Security Considerations Ordinary vote counting issue. ### Scaling Considerations N/A ### Documentation Considerations None ### Testing Considerations Added tests for committee. ### Upgrade Considerations The change is in the committee code. This needs to be merged before the proposal in Agoric#10164 runs. It [already installs new committee code](https://github.com/Agoric/agoric-sdk/blob/4a33accaeeba27044ab07dd04f64226de1b77759/packages/builders/scripts/inter-protocol/replace-electorate-core.js#L28).
… in PR descriptions
…0318) ## Description Updates inline comments in GitHub actions files to explain CI override directives (`#endo-branch`, `#getting-started-branch`, etc.) and adds references to them in the PR template to reduce our dependence upon oral tradition. Also, although out of scope for this PR and probably not needed, we might consider a future change to pull out a helper for consumption of such directives: <details><summary>.github/**/*.yml</summary> ```patch - name: Get the appropriate dapp branch id: get-branch - uses: actions/github-script@v7 + uses: ./.github/actions/read-pr-directive with: - result-encoding: string - script: | - let branch = 'main'; - if (context.payload.pull_request) { - const { body } = context.payload.pull_request; - const regex = /^\#getting-started-branch:\s+(\S+)/m; - const result = regex.exec(body); - if (result) { - branch = result[1]; - } - } - console.log('getting-started dapp branch: ' + branch); - return branch; + tag: '#getting-started-branch' + default-value: main ``` </details> <details><summary>.github/actions/read-pr-directive</summary> ```yaml name: Read PR Directive description: 'Read the value of a `#`-prefixed directive from a PR description' inputs: tag: description: 'The directive tag to read (must start with `#`)' required: true default-value: description: 'The value to return in the absence of any overriding directive' required: true outputs: result: description: >- The value of the last overriding directive, or the default value if the context is not a pull request or there is no such directive value: ${{ steps.script.outputs.result }} runs: using: composite steps: - id: script uses: actions/github-script@v7 env: TAG: ${{ inputs.tag }} DEFAULT_VALUE: ${{ inputs.default-value }} with: result-encoding: string script: |- const q = JSON.stringify; const { TAG, DEFAULT_VALUE } = process.env; if (!TAG.startsWith('#')) { throw Error(`malformed directive tag ${q(TAG)}`); } if (!context.payload.pull_request) return DEFAULT_VALUE; const { body } = context.payload.pull_request; const directives = body.matchAll(/^(\#[\w-]+):\s+(\S+)/gm); const selections = directives.filter(m => m[1] === TAG).map(m => m[2]); const selection = selections.pop(); for (const ignored of selections) { console.warn(`ignoring ${q(`${TAG}: ${ignored}`)}`); } console.log(`${TAG}: ${selection}`); return selection || DEFAULT_VALUE; ``` </details> ### Security Considerations n/a ### Scaling Considerations n/a ### Documentation Considerations :ballot_box_with_check: ### Testing Considerations n/a ### Upgrade Considerations n/a
extracted getVariantFromUpgradeName
The vaults upgrade consumed instance.auctioneer and compared it to the new instance, passed from the new auctioneer proposal in a single-use promise, and failed if they weren't different. In this software upgrade, the instance.auctioneer value might be the new value, so this test can fail. Since the single-use promise comes directly from the new auction proposal, if it has a value, it's the new one.
The kernel is responsible for "disconnecting" (rejecting) promises decided by a vat being upgraded, because Promise settlement functions (resolve/reject) are ephemeral, so they are lost during the upgrade, so the new incarnation will have no way to ever settle them. When the vat resolves a promise with syscall.resolve(), that vat's c-list entry is removed by the syscall handler. But when the kernel did the rejection, the c-list entry was left in place, causing the promise to be pinned forever. This changes the kernel to remove the c-list entry as well, except in the case that the vat is also subscribed to its own promise (which happens when a PromiseWatcher is used on a locally-decided promise: unusual but we needed to tolerate that case). Self-subscribed promises are left in the c-list, because the subsequent dispatch.notify() will finish the job and remove the entry. We also add remediation code, run during `upgradeSwingset`, and bump the state version from 2 to 3. Remediation looks for all settled promises that are still present in vat c-lists, and schedules dispatch.notify deliveries to allow the usual cleanup code to remove the entries, decrement the refcounts, and GC anything thus freed. This results in harmless deliveries that will be ignored by the vat's new incarnation. fixes Agoric#9039
Two tests are updated to exercise the cleanup of promise c-list entries during vat termination. `terminate.test.js` adds some promises to the c-list and then checks their refcounts after termination, to demonstrate that bug Agoric#10261 is leaking a refcount when it deletes the dead vat's c-list entry without also decrementing the refcount. `slow-termination.test.js` adds a number of promises to the c-list, and expects the budget-limited cleanup to spend a phase on promises. Both tests are marked as failing until the code fix is landed in the next commit.
Previously, when a vat was terminated, and we delete the promise c-list entries from its old state, the cleanup code was failing to decrement the kpid's refcount properly. This resulted in a leak: those promises could never be retired. This commit updates the vat cleanup code to add a new phase, named `promises`. This executes after `exports` and `imports`, but before `kv`, and is responsible for both deleting the c-list entries and also decrementing the refcounts of the corresponding promises. We do this slowly, like we do exports and imports, because we don't know how many there might be, and because those promise records might hold references to other objects (in the resolution data), which could trigger additional work. However, this work is unlikely to be significant: the run-queue is usually empty, so these outstanding promises are probably unresolved, and thus cannot beholding resolution data. All promises *decided* by the dead vat are rejected by the kernel immediately during vat termination, because those rejections are visible to userspace in other vats. In contrast, freeing the promise records is *not* visible to userspace, just like how freeing imports or exports are not visible to userspace, so this cleanup is safe to do at a leisurely pace, rate-limited by `runPolicy.allowCleanup`. The docs are updated to reflect the new `runPolicy` API: * `budget.promises` is new, and respected by slow cleanup * `work.promises` is reported to `runPolicy.didCleanup()` The 'test.failing' marker was removed from the previously updated tests. I don't intend to add any remediation code: it requires a full refcount audit to find such promises, and the mainnet kernel has only ever terminated one vat so far, so I believe there cannot be very many leaked promises, if any. Once this fix is applied, no new leaks will occur. fixes Agoric#10261
…goric#10268) Previously, when a vat was terminated, and we delete the promise c-list entries from its old state, the cleanup code was failing to decrement the kpid's refcount properly. This resulted in a leak: those promises could never be retired. This commit updates the vat cleanup code to add a new phase, named `promises`. This executes after `exports` and `imports`, but before `kv`, and is responsible for both deleting the c-list entries and also decrementing the refcounts of the corresponding promises. We do this slowly, like we do exports and imports, because we don't know how many there might be, and because those promise records might hold references to other objects (in the resolution data), which could trigger additional work. However, this work is unlikely to be significant: the run-queue is usually empty, so these outstanding promises are probably unresolved, and thus cannot beholding resolution data. All promises *decided* by the dead vat are rejected by the kernel immediately during vat termination, because those rejections are visible to userspace in other vats. In contrast, freeing the promise records is *not* visible to userspace, just like how freeing imports or exports are not visible to userspace, so this cleanup is safe to do at a leisurely pace, rate-limited by `runPolicy.allowCleanup`. The docs are updated to reflect the new `runPolicy` API: * `budget.promises` is new, and respected by slow cleanup * `work.promises` is reported to `runPolicy.didCleanup()` I don't intend to add any remediation code: it requires a full refcount audit to find such promises, and the mainnet kernel has only ever terminated one vat so far, so I believe there cannot be very many leaked promises, if any. Once this fix is applied, no new leaks will occur. fixes Agoric#10261
All vats have `vatParameters`, a mostly-arbitrary data record, delivered as the second argument to their `buildRootObject()` function. Dynamic vats get their vatParameters from the options bag given to `E(vatAdminSvc).createVat()`. Static vats get them from the kernel config record. When a vat is upgraded, the new incarnation gets new vatParameters; these come from the options bag on `E(adminNode).upgrade()`. When received via `createVat()` or `upgrade()`, the vatParameters can contain object and device references. VatParameters cannot include promises. Previously, the kernel delivered vatParameters to the vat, but did not keep a copy. With this commit, the kernel retains a copy of vatParameters (including a refcount on any kernel objects therein). Internally, `vatKeeper.getVatParameters()` can be used to retrieve this copy. Only vats created or upgraded after this commit lands will get retained vatParameters: for older vats this will return `undefined`. Retained vat parameters should make it easier to implement "upgrade all vats", where the kernel perform a unilateral `upgrade()` on all vats without userspace asking for it. When this is implemented, the new incarnations will receive the same vatParameters as their predecessors. The slow-termination test was updated: it counts kvStore entries precisely as we delete them all, so it requires an update each time we add one. fixes Agoric#8947
…ric#10270) fix(swingset): retain vatParameters for vat creation and upgrade All vats have `vatParameters`, a mostly-arbitrary data record, delivered as the second argument to their `buildRootObject()` function. Dynamic vats get their vatParameters from the options bag given to `E(vatAdminSvc).createVat()`. Static vats get them from the kernel config record. When a vat is upgraded, the new incarnation gets new vatParameters; these come from the options bag on `E(adminNode).upgrade()`. When received via `createVat()` or `upgrade()`, the vatParameters can contain object and device references. VatParameters cannot include promises. Previously, the kernel delivered vatParameters to the vat, but did not keep a copy. With this commit, the kernel retains a copy of vatParameters (including a refcount on any kernel objects therein). Internally, `vatKeeper.getVatParameters()` can be used to retrieve this copy. Only vats created or upgraded after this commit lands will get retained vatParameters: for older vats this will return `undefined`. Retained vat parameters should make it easier to implement "upgrade all vats", where the kernel perform a unilateral `upgrade()` on all vats without userspace asking for it. When this is implemented, the new incarnations will receive the same vatParameters as their predecessors. fixes Agoric#8947
Refs: Agoric/BytePitchPartnerEng#23 fix(acceptance-psm): address change requests
closes: https://github.com/Agoric/BytePitchPartnerEng/issues/23 ## Description Current version of `z:acceptance` tests do not cover PSM at all. The goal of this PR is to make sure we cover test cases in https://github.com/Agoric/BytePitchPartnerEng/issues/23 prepared by @otoole-brendan ### Security Considerations None. ### Scaling Considerations None. ### Documentation Considerations None. ### Testing Considerations The tests assume that `anchorPerMinted` ratio is 1. I think this is a safe assumption because; 1. Unit tests and `boot` tests for the PSM has the same assumption 2. The value is read from terms. Therefore, it can only change by "replacing" the `USDC_axl` instance which is something we shouldn't worry about anytime soon. ### Upgrade Considerations None.
They'll be reported on flush
refs: Agoric#10300 Incidental Best reviewed commit-by-commit ## Description While verifying Agoric#10300 I ran into some errors and lack of stdout streaming features. This is what I arrived at to let me process some slog files manually. ### Security Considerations None ### Scaling Considerations None production impacting This adds a new block throttle mechanism to the ingest-slog tool, while relaxing the line based throttle. ### Documentation Considerations None ### Testing Considerations Manually tested with the slog sender detailed in Agoric#10300 (review). ### Upgrade Considerations Affects chain software, but only the optional telemetry side. Not consensus affecting.
closes: Agoric#10269 ## Description Adds a slog sender which will build various contexts along the way and report them along with the slogs for better logs querying and identification ### Security Considerations None ### Scaling Considerations This uses a json file storage ### Documentation Considerations This is a new slogger which can be opted into ### Testing Considerations This will be deployed on testnets (already deployed on one of the testnets and log link is added in a comment below) ### Upgrade Considerations This can be configured on existing deployments by bumping the telemetry package
…se` phase of `f:replace-price-feeds` (Agoric#10296) closes: https://github.com/Agoric/BytePitchPartnerEng/issues/26 refs: https://github.com/Agoric/BytePitchPartnerEng/issues/22 ## Description This PR introduces a new script, `verifyPushedPrice`, which is executed during the `use` phase of the `n:upgrade-next` proposal. The purpose of this script is to set up the oracle and push an initial price for key brands such as Atom and stAtom. This functionality ensures that a price quote is available for the subsequent proposals and acceptance tests. ### Security Considerations ### Scaling Considerations ### Documentation Considerations ### Testing Considerations The existing test file, `priceFeedUpdate.test.js`, has been updated to account for the new price feed calls made during the use phase, as well as take advantage of the new helper functions built. ### Upgrade Considerations
anilhelvaci
force-pushed
the
anil/auction-tests
branch
from
a89e8e2 to
6fc0e4d
Compare
…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 Agoric#10171 Refs: Agoric/BytePitchPartnerEng#8 fix: style fixes fix(acceptance-auction): lint fixes
anilhelvaci
force-pushed
the
anil/auction-tests
branch
from
b714d15 to
20ca789
Compare
…llision Refs: Agoric/BytePitchPartnerEng#31 fix(auction-acceptance): formatting fixes fix(auction-acceptance): formatting fixes
anilhelvaci
force-pushed
the
anil/auction-tests
branch
from
20ca789 to
bf05414
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.
closes: https://github.com/Agoric/BytePitchPartnerEng/issues/8
closes: Agoric#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;
USEphase ofn:upgrade-next, we place a bidTESTphase ofz:acceptancegov1deposits 100 IST to ATOM auction bookuser1places a bidgov2places a bidWhy is this PR a draft?
This pr depends on tools in Agoric#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. I 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
auctionsin 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 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;
t - 3nextStartTimeof the auction schedule wast - 1ttthe waker set for the roundt - 1is triggered before ourrun-test.shis runt - 1when it is actually attt - 1to start att + 1t + 1is now theactiveStartTimebut there's a+1delay that we have to wait foractiveStartTimeis already captured when thet - 1waker run during the fast forward when chain restartednewStartTimenewStartTimet) PLUS a whole auction roundThis scenario is common when
yarn test -m acceptanceuses caching as the latest cached layer will have it's state at when it was built. So if you built your 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 less than 5 mins), it 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-acceptancebuild might be to change them in an earlier proposalUSEstate 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.