support changes to governed APIs #10822

Chris-Hibbert · 2025-01-08T23:28:08Z

closes: #9990

Description

When a contract is upgraded, we'll read the governed APIs afresh.

Security Considerations

Nothing special. The point of governed APIs is that contracts can make access to calling specific APIs legible in the sense used by governance.

Scaling Considerations

Not an issue.

Documentation Considerations

If we had detailed documentation on governance, this would be worth adding to the docs.

Testing Considerations

Added a bootstrap test showing that a contract can be upgraded and the new APIs will be available after upgrade.

Upgrade Considerations

AssetReserve, fluxAggregator, and VaultFactory use apiGovernance and might need this support if any governed APIs were added.

cloudflare-workers-and-pages · 2025-01-09T00:16:43Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`ffe2dd6`
Status:	✅ Deploy successful!
Preview URL:	https://90740884.agoric-sdk.pages.dev
Branch Preview URL:	https://9990-upgradeapis.agoric-sdk.pages.dev

View logs

dckc

Yes, I agree we should merge this.

How to validate it on a testnet in an upcoming release is sort of... interesting. It will have no impact until some governed contract gets upgraded. That might be in a later release. At that point, there would be a testing burden to verify that this fix works. But this issue won't be listed in the things we fixed for that later release. Seems like an acceptable risk.

The rest of my comments are sort of thinking out loud. Maybe I should keep them to myself... but I suppose I'll share anyway.

dckc · 2025-01-15T06:03:39Z

packages/boot/test/bootstrapTests/upgradeAPI.test.ts

+];
+
+const setUpGovernedContract = async (zoe, timer, EV, controller) => {
+  const installBundle = contractBundle => EV(zoe).install(contractBundle);


Is it important to use .install(...) rather than .installBundleID(...)?

Nope. They're equally useful AFAICT.

In order to understand, I looked at the docs, and realized they were out of date, so I created Agoric/documentation#1265.

dckc · 2025-01-15T06:09:00Z

packages/boot/test/bootstrapTests/upgradeAPI.test.ts

+  add2: { outgoing: 'add2' },
+};
+
+let governedContract;


module level mutable state. hm. consider using a promise(kit) in t.context something like t.context.governedContractKit.promise and .resolver

I didn't mind module mutable state in a test. What javascript is missing here is write-once variables, but you're right that promises provide that ability. Done.

dckc · 2025-01-15T06:10:25Z

packages/boot/test/bootstrapTests/upgradeAPI.test.ts

+  const agoricNamesAdmin =
+    await EV.vat('bootstrap').consumeItem('agoricNamesAdmin');
+  const instanceAdmin = await EV(agoricNamesAdmin).lookupAdmin('instance');
+  await EV(instanceAdmin).update('governedContract', instance);


I wonder why we're adding governedContract to agoricNames.instance

[I don't have good terminology for this. I welcome cleaner solutions.]

I needed an object with getKref() to pass to governanceDriver.proposeApiCall(). Storing the object in agoricNames, and retrieving it withEV was the only path I found that worked.

If it had been a substantive contract, storing the instance in agoricNames would have been a reasonable thing to do, and it would have had a more plausible name. j

dckc · 2025-01-15T06:25:12Z

packages/governance/src/contractHelper.js

+    // Far side-effects the object, and can only be applied once
+    const farGovernedApis = Far('governedAPIs', governedApis);


after thinking about this a bit, it's probably OK as is. But I'll leave some breadcrumbs...

can we make it the caller's responsibility to mark it Far? Making the record in one place and marking it Far in another is odd.

One possibility is to make a new record...

Suggested change

// Far side-effects the object, and can only be applied once

const farGovernedApis = Far('governedAPIs', governedApis);

// Far side-effects the object, and can only be applied once

const farGovernedApis = Far('governedAPIs', { ... governedApis });

One possibility is to make a new record...

Thanks. That's a much better answer.

dckc · 2025-01-15T06:29:32Z

packages/zoe/src/contractFacet/zcfZygote.js

+          } catch (e) {
+            Fail`restartContract failed: original contract facets were not durable`;
+          }


Is it really the case that the only exception possible is that the facets were not durable? I suppose so... zcfBaggage.get(...) is the only thing in the try.

But if get(...) had a bug, we might lose diagnostic info here. It feels weird to throw away e completely.
log it?

Do we support something like this?

Suggested change

} catch (e) {

Fail`restartContract failed: original contract facets were not durable`;

}

} catch (e) {

throw assert.error(`restartContract failed: original contract facets were not durable`, { cause: e });

}

In what way would that be better?

I think Fail gets us an error, a stack, and messages visible in logs. I don't know what throw assert.error() could add to that. Whatever assert.error() does, I wouldn't expect it to return something that would be useful to throw, but I certainly don't know for sure.

In what way would that be better?

It doesn't throw away the details of e. Note especially { cause: e }

dckc · 2025-01-15T06:36:20Z

packages/boot/test/bootstrapTests/upgradeAPI.test.ts

@@ -0,0 +1,311 @@
+import { test as anyTest } from '@agoric/swingset-vat/tools/prepare-test-env-ava.js';


it's a little hard to see the forest for the trees.

Part of me would like the uninteresting stuff moved to a separate file, but then I'd have to jump back and forth.

Ah, of course.
(rhetorical) Why didn't yarn lint warn me about an unused variable?
I can't easily add that to my pre-check script, since it's just a grep pattern.

mergify · 2025-01-22T19:34:01Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

❌ integration-test-result
☑️ linear-history

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

Chris-Hibbert added Governance Governance contract-upgrade labels Jan 8, 2025

Chris-Hibbert self-assigned this Jan 8, 2025

Chris-Hibbert requested a review from a team as a code owner January 8, 2025 23:28

Chris-Hibbert requested a review from AgoricTriage January 8, 2025 23:28

Chris-Hibbert force-pushed the 9990-upgradeAPIs branch from cc5512a to 4d3a323 Compare January 9, 2025 00:16

Chris-Hibbert force-pushed the 9990-upgradeAPIs branch 3 times, most recently from 0edc4ac to d1ec616 Compare January 15, 2025 00:24

Chris-Hibbert requested a review from dckc January 15, 2025 00:49

dckc approved these changes Jan 15, 2025

View reviewed changes

Chris-Hibbert added the automerge:rebase Automatically rebase updates, then merge label Jan 22, 2025

Chris-Hibbert added 5 commits January 22, 2025 18:41

refactor: support updating governedAPIs at upgrade

9329e8a

chore: improve message when original contract facets weren't durable

53b79de

test: Verify that new APIs are recognized on upgrade

2d53503

chore: improvements from review

7c58e75

chore: include error with failure message

ffe2dd6

Chris-Hibbert force-pushed the 9990-upgradeAPIs branch from 7f7eb0c to ffe2dd6 Compare January 22, 2025 18:41

mergify bot merged commit f5bf658 into master Jan 22, 2025
83 checks passed

mergify bot deleted the 9990-upgradeAPIs branch January 22, 2025 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support changes to governed APIs #10822

support changes to governed APIs #10822

Chris-Hibbert commented Jan 8, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Jan 9, 2025 •

edited

Loading

dckc left a comment

dckc Jan 15, 2025

Chris-Hibbert Jan 21, 2025

dckc Jan 15, 2025

Chris-Hibbert Jan 21, 2025

dckc Jan 15, 2025

Chris-Hibbert Jan 21, 2025

dckc Jan 15, 2025

Chris-Hibbert Jan 21, 2025

dckc Jan 15, 2025

dckc Jan 15, 2025

Chris-Hibbert Jan 22, 2025

dckc Jan 22, 2025

dckc Jan 15, 2025

Chris-Hibbert Jan 21, 2025

Chris-Hibbert Jan 22, 2025

mergify bot commented Jan 22, 2025

		// Far side-effects the object, and can only be applied once
		const farGovernedApis = Far('governedAPIs', governedApis);

		@@ -0,0 +1,311 @@
		import { test as anyTest } from '@agoric/swingset-vat/tools/prepare-test-env-ava.js';

support changes to governed APIs #10822

support changes to governed APIs #10822

Conversation

Chris-Hibbert commented Jan 8, 2025 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Jan 9, 2025 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

dckc left a 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

mergify bot commented Jan 22, 2025

Chris-Hibbert commented Jan 8, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Jan 9, 2025 •

edited

Loading