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

feat: update auction in agoricNames, test that the boardId changed #9970

Merged
merged 2 commits into from
Aug 27, 2024

Conversation

Chris-Hibbert
Copy link
Contributor

closes: #9963

Description

The upgrade of auctions neglected to replace the agoricNames binding for the auction instance. This fixes that and tests that the boardId is different before and after the coreEval.

Security Considerations

scary that we got that far through validation without noticing that the auction hadn't been updated in agoricNames.

Scaling Considerations

None.

Documentation Considerations

None.

@Chris-Hibbert Chris-Hibbert added contract-upgrade auction next-release about next agoric-sdk or endo release labels Aug 27, 2024
@Chris-Hibbert Chris-Hibbert requested review from turadg and dckc August 27, 2024 02:58
@Chris-Hibbert Chris-Hibbert self-assigned this Aug 27, 2024
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

a couple notes for future reference...

Comment on lines +120 to +121
t.true(
newAuctionInstance !== oldInstance,
Copy link
Member

Choose a reason for hiding this comment

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

Probably not worth a re-spin for this alone, but for future ref:

If you use t.not, ava will show the 2 values when the assertion fails.

Suggested change
t.true(
newAuctionInstance !== oldInstance,
t.not(newAuctionInstance,
oldInstance,

auctionInstance.reset();
await auctionInstance.resolve(governedInstance);
// belt and suspenders; the above is supposed to also do this
await E(E(agoricNamesAdmin).lookupAdmin('instance')).update(
Copy link
Member

Choose a reason for hiding this comment

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

agoricNamesAdmin is a big hammer; it's unfortunate that resolve() alone didn't do the trick.

here's hoping for some later convenient time to figure out why not

Copy link
Member

Choose a reason for hiding this comment

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

the comment reads "belt-and-suspenders" as if the belt sometimes works. Does it not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't found the code that "is supposed to", so I don't know what it's trying to save, or what the corner cases might be. What code in promiseSpace is supposed to treat instances specially? Or what else updates agoricNames for instances?

@dckc dckc added the force:integration Force integration tests to run on PR label Aug 27, 2024
@Chris-Hibbert
Copy link
Contributor Author

The docker test failed wtih Error: key (an object) not found in collection "instanceToGovernor". That sounds like a flake to me, so I'll re-run it.

@dckc
Copy link
Member

dckc commented Aug 27, 2024

I don't think it's a flake. I think the econCommitteeCharter has to be introduced to the new auctioneer.

2024-08-27T03:55:06.783Z SwingSet: ls: v15: Error#1: key Object [Alleged: InstanceHandle] {} not found in collection instanceToGovernor
2024-08-27T03:55:06.783Z SwingSet: ls: v15: Error: key (an object) not found in collection "instanceToGovernor"
 at construct ()
 at Error (/bundled-source/.../node_modules/ses/src/error/tame-error-constructor.js:56)
 at makeError (/bundled-source/.../node_modules/ses/src/error/assert.js:243)
 at fail (/bundled-source/.../node_modules/ses/src/error/assert.js:357)
 at get (/bundled-source/.../packages/swingset-liveslots/src/collectionManager.js:351)
 at voteOnParamChanges (.../inter-protocol/src/econCommitteeCharter.js:79)

Copy link

cloudflare-workers-and-pages bot commented Aug 27, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 87b945d
Status: ✅  Deploy successful!
Preview URL: https://a03c723d.agoric-sdk.pages.dev
Branch Preview URL: https://9963-registernewauction.agoric-sdk.pages.dev

View logs

@Chris-Hibbert Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Aug 27, 2024
@Chris-Hibbert
Copy link
Contributor Author

Oops. It looks like you added your commit, and then I merge it again. I hope that was idempotent. I marked it for automerge:rebase.

@Chris-Hibbert Chris-Hibbert force-pushed the 9963-registerNewAuction branch from ed7f1e6 to 87b945d Compare August 27, 2024 05:27
@mergify mergify bot merged commit 43345a5 into master Aug 27, 2024
83 checks passed
@mergify mergify bot deleted the 9963-registerNewAuction branch August 27, 2024 14:11
@@ -201,3 +201,18 @@ export const getProvisionPoolMetrics = async () => {
const path = `published.provisionPool.metrics`;
return getQuoteBody(path);
};

export const getAuctionInstance = async price => {
Copy link
Member

Choose a reason for hiding this comment

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

not really an "agd tool". Also would be more general with a little tweak:

Suggested change
export const getAuctionInstance = async price => {
export const getInstanceBoardId = async instanceName => {

(price isn't used)

Not a blocker for this repo but in synthetic-chain package it should be designed for re-use.

Comment on lines +1 to +4
# we have an eval.sh so we can run prepare.sh before the rest

echo "[$PROPOSAL] Running prepare.sh"
./prepare.sh
Copy link
Member

Choose a reason for hiding this comment

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

prepare is for chain-halting upgrade proposals. There should be no prepare.sh in a CoreEvalProposal.

Please remove the indirection:

Suggested change
# we have an eval.sh so we can run prepare.sh before the rest
echo "[$PROPOSAL] Running prepare.sh"
./prepare.sh
./saveAuctionInstance.js

@@ -4,3 +4,5 @@
# actions are executed in the upgraded chain software and the effects are
# persisted in the generated image for the upgrade, so they can be used in
# later steps, such as the "test" step, or further proposal layers.

Copy link
Member

Choose a reason for hiding this comment

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

another reason to remove this file is it says it's for after the proposal is executed but you're running it as part of the eval

console.log('old auction instance ', oldAuctionInstance, env.HOME);

await writeFile(
`${env.HOME}/.agoric/previousInstance.json`,
Copy link
Member

Choose a reason for hiding this comment

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

this file is part of the permanent history a3p. please give it a more informative name for someone who comes across it without this context. or delete it after it's read.

also in general I think f a3p is writing to this directory it should be under a sub-path

auctionInstance.reset();
await auctionInstance.resolve(governedInstance);
// belt and suspenders; the above is supposed to also do this
await E(E(agoricNamesAdmin).lookupAdmin('instance')).update(
Copy link
Member

Choose a reason for hiding this comment

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

the comment reads "belt-and-suspenders" as if the belt sometimes works. Does it not?

mergify bot added a commit that referenced this pull request Sep 4, 2024
refs: #9970

## Description
Follow up on #9970 that had merged while I was reviewing

### Security Considerations
none
### Scaling Considerations
none

### Documentation Considerations
none

### Testing Considerations
CI

### Upgrade Considerations
no additional
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auction automerge:rebase Automatically rebase updates, then merge contract-upgrade force:integration Force integration tests to run on PR next-release about next agoric-sdk or endo release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vaults not being liquidated and therefore collateral not being sent to auctioneer for auction (Xnet)
3 participants