8722 provisionPool re-register notifiers #9481

Chris-Hibbert · 2024-06-10T22:35:23Z

fixes: #8722
fixes: #8724
fixes: #8238

Description

Upgrade provisionPool so its notifiers are robust to upgrades.

Security Considerations

This is about soundness of the chain and its services.

Scaling Considerations

N/A

Documentation Considerations

No change to functionality.

Testing Considerations

Includes a test in a3p-integration that demonstrates that provisionPool will survive its own or vat-bank's upgrade.

Upgrade Considerations

See above.

cloudflare-workers-and-pages · 2024-06-10T23:07:10Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`997d72c`
Status:	✅ Deploy successful!
Preview URL:	https://14c87d62.agoric-sdk.pages.dev
Branch Preview URL:	https://8722-provpoolresub.agoric-sdk.pages.dev

View logs

turadg · 2024-06-11T18:29:31Z

packages/inter-protocol/src/provisionPoolKit.js

+                  if (desc.issuerName.match(FIRST_CAP_ASCII)) {
+                    trace(`Saved Issuer ${desc.issuerName}`);
+                    await zcf.saveIssuer(desc.issuer, desc.issuerName);
+
+                    // see https://github.com/Agoric/agoric-sdk/issues/8238
+                  } else if (desc.issuerName.match(FIRST_LOWER_ASCII)) {
+                    await saveIssuerWithLegalKeyword(desc, zcf);
+                  } else {
+                    console.error(
+                      `unable to save issuer with illegal keyword: ${desc.issuerName}`,
+                    );
+                  }


both branch save with a legal keyword. This abstraction makes it look like this branch does something different. I think it would be more clear if you made a valid desc and then saved it consistently.

Suggested change

if (desc.issuerName.match(FIRST_CAP_ASCII)) {

trace(`Saved Issuer ${desc.issuerName}`);

await zcf.saveIssuer(desc.issuer, desc.issuerName);

// see https://github.com/Agoric/agoric-sdk/issues/8238

} else if (desc.issuerName.match(FIRST_LOWER_ASCII)) {

await saveIssuerWithLegalKeyword(desc, zcf);

} else {

console.error(

`unable to save issuer with illegal keyword: ${desc.issuerName}`,

);

}

// let this handle the conditions and log about whether repair was necessary

const goodDesc = repair(desc);

if (goodDesc) {

await zcf.saveIssuer(desc.issuer, desc.issuerName);

} else {

console.error(

`unable to save issuer with illegal keyword: ${desc.issuerName}`,

);

}

Yes, that's better.

turadg · 2024-06-11T18:32:47Z

packages/vats/src/proposals/upgrade-provisionPool-proposal.js

+    instancePrivateArgsP,
+  ]);
+  const { adminFacet, instance } = provisionPoolStartResult;
+  // const privateArgs = await deeplyFulfilled(instancePrivateArgs.get(instance));


turadg · 2024-06-11T18:33:43Z

a3p-integration/proposals/a:upgrade-next/agd-tools.js

@@ -137,3 +137,8 @@ export const bankSend = (addr, wanted) => {

  return agd.tx('bank', 'send', VALIDATORADDR, addr, wanted, ...noise);
 };
+
+export const getProvisionPoolMetrics = async () => {


not worth the abstraction imo

It hides two things: getQuoteBody() is not exported, and the callers would have to share a definition of the path. I'll make the change if you insist, but you'll have to ask a second time. :-)

ah, I didn't notice the export privacy aspect

turadg · 2024-06-11T18:35:06Z

a3p-integration/proposals/a:upgrade-next/provisionPool.test.js

+
+/**
+ * @file
+ * The problems to be addressed are that provisionPool won't correctly handle


minor: "won't correctly handle" is one problem

turadg · 2024-06-11T18:37:45Z

a3p-integration/proposals/a:upgrade-next/provisionPool.test.js

+ * 2a. if we had been able to null-upgrade provisionPool at this point, we
+ * wouldn't have been able to productively add USDC, but the null upgrade fails.


IIUC, this would be more clear:

Suggested change

* 2a. if we had been able to null-upgrade provisionPool at this point, we

* wouldn't have been able to productively add USDC, but the null upgrade fails.

* 2a. verify a provisionPool null upgrade fails (if it passed, we wouldn't have been able to productively add USDC)

Though I don't understand why

I attempted to clarify.

turadg · 2024-06-11T18:38:09Z

a3p-integration/proposals/a:upgrade-next/provisionPool.test.js

+ * 3. Do a full upgrade of provisionPool; then deposit USDC, and see IST
+ * incremented in totalMintedConverted.
+ *
+ * 4. Null upgrade vat-bank again, and then see (in logs) that adding a new


why not make a deposit to verify that ability?

We only have assets for currencies that provisionPool already knows about. We can add new currencies, but I haven't found a reasonable way to mint them to be able to deposit them.

it is trickier since it's a synthetic-chain test. I think you could do it with core-eval that both adds a new currency and with the available powers mints then deposits. But this comment suffices.

turadg · 2024-06-11T18:38:53Z

a3p-integration/proposals/a:upgrade-next/provisionPool.test.js

+    'brands match',
+  );
+  if (expectedToGrow) {
+    // I couldn't import AmountMath. dunno why.


you'd have to add @agoric/ertp as a dependency of this proposal package. Did you try that?

I did try that. Lemme try again.

When I do, I get an error at runtime

ReferenceError: assert is not defined › file://node_modules/@endo/eventual-send/src/E.js:3:40

without anything I can recognize as indicating where the problem manifested.

oh yeah, that durn dependency on SES.

You'll also need import "@endo/init/debug.js" before that import.

I'm not familiar with that one (or I've forgotten). Do you have a pointer to an issue or discussion?

Ah, it turns out that what we get from vstorage isn't actually an Amount. The brand field contains stringified captp representations. We can compare them for equality, but they're not the brands that AmountMath recognizes.

turadg · 2024-06-11T18:40:07Z

a3p-integration/proposals/a:upgrade-next/provisionPool.test.js

+  }
+};
+
+test.serial('add assets before', async t => {


are these meant to run in sequence? test.serial just means not in parallel and the order is technically undefined.

if they're in sequence, just put them all into a single test. You can use t.log to show progress

turadg · 2024-06-11T18:41:31Z

packages/builders/scripts/vats/add-STARS.js

+
+export default async (homeP, endowments) => {
+  const { writeCoreProposal } = await makeHelpers(homeP, endowments);
+  await writeCoreProposal('add-STARS-collateral', starsVaultProposalBuilder);


doesn't this belong under scripts/inter-protocol like the other add-STARS.js? Please distinguish between them by filename and perhaps a @file comment

Thinking about moving them there (good idea!) made it clearer that these two are for testing, and shouldn't be confused with the production scripts. Is it enough to put them under a testing directory and add a top-comment?
I also renamed them from STARS since the re-use of an actual currency name is of no value.

turadg · 2024-06-11T18:43:18Z

packages/inter-protocol/src/provisionPoolKit.js

-          // the contract, so it technically can change between incarnations.
-          // That would be a severe bug.
-          AmountMath.coerce(poolBrand, params.getPerAccountInitialAmount());
+          void observeNotifier(E(exchangePurse).getCurrentAmountNotifier(), {


this is part of a commit titled refactor: but it's changing behavior. It's adding a new feature for upgrade durability so it's a feat:. Though it would have made review easier if the refactor changes were separated.

fixed. I'll try to make better separations.

https://github.com/tummychow/git-absorb helps me. E.g. when you had the your review changes, if you ran git add . && git absorb it would make fixups on the previous. That way master doesn't have the iterations of code review.

turadg · 2024-06-12T12:35:48Z

packages/inter-protocol/src/provisionPoolKit.js

-          // the contract, so it technically can change between incarnations.
-          // That would be a severe bug.
-          AmountMath.coerce(poolBrand, params.getPerAccountInitialAmount());
+          void observeNotifier(E(exchangePurse).getCurrentAmountNotifier(), {


https://github.com/tummychow/git-absorb helps me. E.g. when you had the your review changes, if you ran git add . && git absorb it would make fixups on the previous. That way master doesn't have the iterations of code review.

turadg · 2024-06-12T12:38:07Z

packages/builders/scripts/testing/add-LEMONS.js

+
+/**
+ * @file
+ *     *** TESTING ONLY   ***   NOT FOR PRODUCTION USE  ***


+1 to scripts/testing. Perhaps even scripts/a3p. There might be some other scripts that belong here but nbd.

That said, I think this line is too scary. There's practically zero risk that somebody is going to find scripts/testing/add-LEMON.js and shepherd it into production.

Suggested change

* *** TESTING ONLY *** NOT FOR PRODUCTION USE ***

Yeah, okay. I added the scary line before I renamed them. It's unneeded now.

turadg · 2024-06-12T12:39:22Z

packages/inter-protocol/src/provisionPoolKit.js

+const FIRST_CAP_ASCII = /^[A-Z][a-zA-Z0-9_$]*$/;
+const FIRST_LOWER_ASCII = /^[a-z][a-zA-Z0-9_$]*$/;


these are more restricted than ASCII. consider,

Suggested change

const FIRST_CAP_ASCII = /^[A-Z][a-zA-Z0-9_$]*$/;

const FIRST_LOWER_ASCII = /^[a-z][a-zA-Z0-9_$]*$/;

const FIRST_CAP_KEYWORD = /^[A-Z]/;

const FIRST_LOWER_KEYWORD = /^[a-z]/;

I'm changing the names to FIRST_UPPER_KEYWORD and FIRST_LOWER_NEAR_KEYWORD, but keeping the patterns the same. If it's not a keyword pattern, I'd rather take the else fallback.

repair kyyword names for some issuers convert from console.log() to trace()

mhofman · 2024-06-17T23:30:14Z

a3p-integration/proposals/a:upgrade-next/localchain.test.js

Why is this test being removed?

closes: #9553 ## Description Restores tests deleted in #9481 and fixes a bug in localchain introduced since ### Security Considerations None ### Scaling Considerations None ### Documentation Considerations None ### Testing Considerations Restoring integration tests ### Upgrade Considerations Needed for upgrade 16

Chris-Hibbert added bug Something isn't working test Inter-protocol Overarching Inter Protocol contract-upgrade labels Jun 10, 2024

Chris-Hibbert self-assigned this Jun 10, 2024

Chris-Hibbert changed the title ~~8722 provisionPool re-register norifiers~~ 8722 provisionPool re-register notifiers Jun 10, 2024

Chris-Hibbert force-pushed the 8722-provPoolResub branch from b7a696f to 4c0d25d Compare June 10, 2024 23:07

Chris-Hibbert force-pushed the 8722-provPoolResub branch from 4c0d25d to d665a40 Compare June 10, 2024 23:43

Chris-Hibbert requested a review from turadg June 11, 2024 00:14

turadg reviewed Jun 11, 2024

View reviewed changes

Chris-Hibbert force-pushed the 8722-provPoolResub branch from d665a40 to 04d67a3 Compare June 12, 2024 00:22

turadg mentioned this pull request Jun 12, 2024

migrate to @endo/errors package #5672

Closed

turadg approved these changes Jun 12, 2024

View reviewed changes

Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Jun 12, 2024

Chris-Hibbert added 5 commits June 12, 2024 18:14

feat: notifiers recover when vats upgrade

ada1db8

repair kyyword names for some issuers convert from console.log() to trace()

test: add a test for upgrading provisionPool

f20b9da

test: drop or clean-up old Tests

6189733

chore: move core-evals to testing, and rename them from STARS

7b3eea8

chore: cleanup test etc. based on commnts

997d72c

Chris-Hibbert force-pushed the 8722-provPoolResub branch from 87566b6 to 997d72c Compare June 12, 2024 18:14

mergify bot merged commit 26b0505 into master Jun 12, 2024
63 checks passed

mergify bot deleted the 8722-provPoolResub branch June 12, 2024 18:47

mhofman reviewed Jun 17, 2024

View reviewed changes

a3p-integration/proposals/a:upgrade-next/localchain.test.js

Copy link

Member

mhofman Jun 17, 2024

Choose a reason for hiding this comment

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

Why is this test being removed?

This was referenced Jun 21, 2024

integration tests missing from master #9553

Closed

Restore a3p tests #9557

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8722 provisionPool re-register notifiers #9481

8722 provisionPool re-register notifiers #9481

Chris-Hibbert commented Jun 10, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Jun 10, 2024 •

edited

Loading

turadg Jun 11, 2024

Chris-Hibbert Jun 11, 2024

turadg Jun 11, 2024

turadg Jun 11, 2024

Chris-Hibbert Jun 11, 2024

turadg Jun 12, 2024

turadg Jun 11, 2024

turadg Jun 11, 2024

Chris-Hibbert Jun 11, 2024

turadg Jun 11, 2024

Chris-Hibbert Jun 11, 2024

turadg Jun 12, 2024 •

edited

Loading

turadg Jun 11, 2024

Chris-Hibbert Jun 11, 2024

Chris-Hibbert Jun 11, 2024

turadg Jun 12, 2024

Chris-Hibbert Jun 12, 2024

Chris-Hibbert Jun 12, 2024

turadg Jun 11, 2024

Chris-Hibbert Jun 11, 2024

turadg Jun 11, 2024

Chris-Hibbert Jun 11, 2024

turadg Jun 11, 2024

Chris-Hibbert Jun 12, 2024

turadg Jun 12, 2024

turadg Jun 12, 2024

turadg Jun 12, 2024

Chris-Hibbert Jun 12, 2024

turadg Jun 12, 2024

Chris-Hibbert Jun 12, 2024

mhofman Jun 17, 2024

		* 2a. if we had been able to null-upgrade provisionPool at this point, we
		* wouldn't have been able to productively add USDC, but the null upgrade fails.

	* 2a. if we had been able to null-upgrade provisionPool at this point, we
	* wouldn't have been able to productively add USDC, but the null upgrade fails.
	* 2a. verify a provisionPool null upgrade fails (if it passed, we wouldn't have been able to productively add USDC)

		const FIRST_CAP_ASCII = /^[A-Z][a-zA-Z0-9_$]*$/;
		const FIRST_LOWER_ASCII = /^[a-z][a-zA-Z0-9_$]*$/;

8722 provisionPool re-register notifiers #9481

8722 provisionPool re-register notifiers #9481

Conversation

Chris-Hibbert commented Jun 10, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Jun 10, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Hibbert commented Jun 10, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Jun 10, 2024 •

edited

Loading

turadg Jun 12, 2024 •

edited

Loading