test(inter-protocol): snapshot storage/RPC API for auction, vaultFactory, reserve, psm, committees #7368

dckc · 2023-04-10T19:20:28Z

Description

Flesh out the "Reading data off-chain" section of the README with snapshots of chainStorage data produced by the inter protocol contracts.

NOTE: The README says auction.books.book0, but what shows up in the snapshot is just auction.books0. Is that by design?

If feedback on this draft shows that we do want this sort of test...

DRAFT TODO:

address copy-pasta from on-exit test

Security / Scaling Considerations

none, AFAICT.

Documentation Considerations

adds reliable docs.

rendered: Reading data off-chain

for example:

snapshots/test-auctionContract.js.md

I think consumers look for "RPC API". I wonder if we should tweak the "Reading data off-chain" section heading to include that.

Testing Considerations

This adds a bit of test maintenance. As usual with ava snapshot tests: yarn test test/auction/test-auctionContract.js -m '*snap*' --update-snapshots

turadg

Great idea.

NOTE: The README says auction.books.book0, but what shows up in the snapshot is just auction.books0. Is that by design?

I missed that in review. Good thing for snapshot tests! @Chris-Hibbert could you push a commit to correct that here?

Chris-Hibbert · 2023-04-10T20:02:44Z

The README says auction.books.book0, but what shows up in the snapshot is just auction.books0. Is that by design?

I was guessing, I'll admit. I'm definitely okay with it the way it is. If there's a reason for it to be otherwise, it should be simple to fix.

could you push a commit to correct that here?

Done 3cc073a

Chris-Hibbert

seems fine to me. @turadg says this will be valuable, so I'm for it.

Chris-Hibbert · 2023-04-10T20:14:06Z

packages/inter-protocol/test/auction/test-auctionContract.js

@@ -1250,3 +1253,102 @@ test('bid zero', async t => {
    },
  );
 });
+
+test('auctioneer chainStorage RPC API snapshot', async t => {
+  //#region copied from on-exit test


I'm not a fan of #region comments.

agreed that this one, at least, doesn't carry its weight. As noted above, I aim to "DRY out copy-pasta from on-exit test".

I didn't find a straightforward way to DRY out the common stuff; I changed the #region comments into a function.

it occurs to me that, rather than repeating a bunch of stuff from another test, I can just add the t.snapshot() into that test.

dckc · 2023-04-11T03:57:14Z

packages/inter-protocol/README.md

-        - `books`
-          - `book0`
-            - `data`
+        - `book0`


so we want book0, book1 etc. under auction after all?

I'm standing by to see if by "correct this" @turadg meant to change the published structure to include a book or books node rather than tweaking this README.

Sorry @Chris-Hibbert my earlier comment wasn't clear on the request:. I meant to fix the code so it matches the README, and the enumeration principle sketched in #6111

If keys are an index of high cardinality, they should be under their own parent key so that they don't make siblings of other types hard to find. E.g. if manager102 sibling to metrics is bad because the latter will be easy to miss. managers sibling to metrics is good.

Managers and Books are high enough cardinality to merit an grouping key.

That doesn't seem to me like a change that should be made by adding a commit to a PR on another subject.
@dckc would you excise the commit I inserted?
@turadg, would you mind creating an issue that can be prioritized?

each commit ends up separately in master so a bugfix commit is reasonable to me to include in any PR that has the appropriate reviewers. if you want to do the work in a different PR up to you.

as an issue to prioritize, #6111 encompasses the requirement. it's prioritized as a blocker for Vaults Release.

I tried integrating the change with the existing commit stack, but the changes to the README are in a different commit than where I'd put the code changes. When I tried it, it turned into a mess, because several later commits also edit the same section of the README. It'll be far simpler as a follow-on.

dckc · 2023-04-11T17:27:12Z

packages/governance/test/unitTests/snapshots/test-committee.js.md

+          },
+          electionType: 'survey',
+          issue: {
+            text: 'why3',


This lacks detail on parameter change issues. I think developers of dapp-gov, i.e. us, are the most interested consumers of that API, and we already figured it out.

dckc · 2023-04-11T17:27:50Z

packages/inter-protocol/README.md

@@ -30,6 +30,8 @@ To maintain that the keys of vaults to liquidate are stable requires that its ke

 VaultFactory publishes data using StoredPublishKit which tees writes to off-chain storage. These can then be followed off-chain like so,
 ```js
+import { makeFollower } from '@agoric/casting';


Where does makeDefaultLeader come from? I don't see it in @agoric/casting.

dckc · 2023-04-11T17:29:42Z

packages/inter-protocol/README.md

+      - `governance`
+      - `metrics`
+    - `priceFeed` - [snapshot of details](./test/price/snapshots/test-fluxAggregator.js.md)
+      - `${inputBrand}-${outputBrand}_price_feed`


this formerly had a _ as in ATOM_USD when snapshotting shows it's ATOM-USD.

dckc · 2023-04-11T17:30:45Z

packages/inter-protocol/README.md

+      - `metrics`
+    - `priceFeed` - [snapshot of details](./test/price/snapshots/test-fluxAggregator.js.md)
+      - `${inputBrand}-${outputBrand}_price_feed`
+      - `${inputBrand}-${outputBrand}_price_feed.latestRound`


latestRound was missing.

Is it worth trying to maintain this tree of all the keys? Now that I have snapshots for all but stakeFactory, maybe it's better to just list the top level?

latestOutcome below was missing as well.

dckc · 2023-04-11T17:35:07Z

packages/inter-protocol/test/price/snapshots/test-fluxAggregator.js.md

+        'published.priceAggregator.LINK-USD_price_feed.latestRound',
+        {
+          roundId: 3n,
+          startedAt: 5n,


i suppose this will change to a time record in #5799 so that we can keep from exposing the whole timer below

dckc · 2023-04-11T17:44:41Z

The technique I eventually found for the auction contract was so straightforward that I applied it to the other contracts as well (except stakeFactory).

PTAL.

dckc · 2023-04-11T21:41:06Z

Basis points vs. Ratios

We seem to publish basis points in some cases. I wonder if the econ-gov UI is set up for that.

        LowestRate: {
          type: 'nat',
          value: 4500n,
        },

Chris-Hibbert

I'm not an expert on chainStorage, but this looks fine to me.

turadg · 2023-04-11T23:43:31Z

packages/governance/test/unitTests/test-committee.js

@@ -134,7 +134,28 @@ test('committee-open question:one', async t => {
  );
 });

-test('committee-open question:mixed', async t => {
+const snapshotStorage = async (t, storage) => {


types please

Suggested change

const snapshotStorage = async (t, storage) => {

const snapshotStorage = async (

/** @type {import('ava').ExecutionContext<unknown>} */ t,

/** @type {import('@agoric/internal/src/storage-test-utils.js').MockChainStorageRoot} */ storage,

) => {

turadg · 2023-04-11T23:46:19Z

packages/inter-protocol/test/auction/test-auctionContract.js

@@ -906,6 +924,8 @@ test.serial('onDeadline exit', async t => {
    currentPriceLevel: { numerator: { value: 9_350_000_000_000n } },
  });

+  await snapshotStorage(t, driver.mockChainStorage);


each test is defining its own snapshotStorage but they follow the same pattern. please DRY it out. I think something like,

Suggested change

await snapshotStorage(t, driver.mockChainStorage);

await snapshotStorage(t, driver.mockChainStorage, {'mockChainStorageRoot.thisAuction': 'published.auction'}, `Under "published", the "auction" node is delegated to

the auctioneer contract, which publishes data as follows.

See also board marshalling conventions (_to appear_).

` );

or maybe more to the point: documentStorageKeys since the note and the rewriting to published: is not really about testing.

turadg · 2023-04-11T23:50:10Z

packages/inter-protocol/test/psm/test-psm.js

+      storageNode: mockChainStorage
+        .makeChildNode('psm')
+        .makeChildNode('IST')
+        .makeChildNode('AUSD'),


this looks like logic that is meant to be under test but is part of the driver.

consider a function to make a storage path for a PSM which can be re-used in startPSM:

agoric-sdk/packages/inter-protocol/src/proposals/startPSM.js

Lines 129 to 133 in 50c8e26

const psmStorageNode = await makeStorageNodeChild(chainStorage, 'psm');

const storageNode = E(

E(psmStorageNode).makeChildNode(Stable.symbol),

).makeChildNode(keyword);

turadg · 2023-04-11T23:52:59Z

packages/governance/test/unitTests/test-committee.js

+  const note = `Under "published", the "committees.Economic_Committee" node is delegated to
+a committee contract, which publishes data as follows.


clever way to mix docs into tests into docs!

"which publishes data as follows" isn't clear to me. The following is an instance of the possible data. A plausible instance? Just conformant? What can the reader count on it for?

Whatever language is chosen, I think it should be consistent across contract instances (see comment below on DRY)

It's supposed to be illustrative. Ideally, it would illustrate all branches of any union types. I'm not sure how to be precise about what the reader can count without putting so much text in the way that it's distracting..

A plausible instance?

That's accurate and concise, if not complete. I suppose I can go with that.

turadg

Nicely done

turadg · 2023-04-13T14:41:44Z

packages/governance/test/unitTests/test-committee.js

+    pattern: 'mockChainStorageRoot.thisElectorate.',
+    replacement: 'published.committees.Economic_Committee.',


these would read more easily as a map, even if it has just one item.

Suggested change

pattern: 'mockChainStorageRoot.thisElectorate.',

replacement: 'published.committees.Economic_Committee.',

rekey: {'mockChainStorageRoot.thisElectorate': published.committees.Economic_Committee`},

dckc · 2023-04-14T17:40:59Z

A couple storage keys in the vaults snapshot flip-flopped. I'm not sure how stable they were. I sorted them all an regenerated the snapshots.

Also, I squashed all the inter-protocol snapshot commits together. And moved all the governance commits before inter-protocol.

- link auction snapshot details - update docs for actual published keys

where does makeDefaultLeader come from?

dckc requested review from turadg, Chris-Hibbert and otoole-brendan April 10, 2023 19:20

turadg reviewed Apr 10, 2023

View reviewed changes

Chris-Hibbert reviewed Apr 10, 2023

View reviewed changes

dckc commented Apr 11, 2023

View reviewed changes

dckc force-pushed the dc-auction-rpc-snapshot branch from d12900d to 50c8e26 Compare April 11, 2023 17:40

dckc changed the title ~~test(inter-protocol): snapshot auction/auctioneer storage/RPC API~~ test(inter-protocol): snapshot storage/RPC API for auction, vaultFactory, reserve, psm, committees Apr 11, 2023

dckc marked this pull request as ready for review April 11, 2023 17:43

dckc requested review from turadg, Chris-Hibbert and ivanlei April 11, 2023 17:44

Chris-Hibbert approved these changes Apr 11, 2023

View reviewed changes

turadg reviewed Apr 11, 2023

View reviewed changes

turadg mentioned this pull request Apr 12, 2023

7400 governance docs #7402

Merged

dckc added automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR labels Apr 13, 2023

turadg approved these changes Apr 13, 2023

View reviewed changes

dckc force-pushed the dc-auction-rpc-snapshot branch 2 times, most recently from f40093b to 899c566 Compare April 14, 2023 17:39

test: governance committee chainStorage RPC API snapshot

b7eef9e

dckc added 9 commits April 14, 2023 18:08

refactor(governance): factor out documentStorageSchema

89b0e13

chore: generated governance RPC API snapshots

01d6e20

test(inter-protocol): snapshot auctioneer storage/RPC API

be2f176

- link auction snapshot details - update docs for actual published keys

test: snapshot test for reserve RPC API

c5ad3d2

test: snapshot of vaultFactory RPC API

22fb320

test: snapshot psm chainStorage RPC API

aa7932d

test: snapshot price aggregator chainStorage RPC API

53fed71

docs: show where makeFollower() comes from

5e6b84d

where does makeDefaultLeader come from?

chore: generated inter-protocol chainStorage API snapshots

8a4d75a

dckc force-pushed the dc-auction-rpc-snapshot branch from 899c566 to 8a4d75a Compare April 14, 2023 18:08

mergify bot merged commit 2b027a4 into master Apr 14, 2023

mergify bot deleted the dc-auction-rpc-snapshot branch April 14, 2023 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(inter-protocol): snapshot storage/RPC API for auction, vaultFactory, reserve, psm, committees #7368

test(inter-protocol): snapshot storage/RPC API for auction, vaultFactory, reserve, psm, committees #7368

dckc commented Apr 10, 2023 •

edited

Loading

turadg left a comment

Chris-Hibbert commented Apr 10, 2023

Chris-Hibbert left a comment

Chris-Hibbert Apr 10, 2023

dckc Apr 11, 2023

dckc Apr 11, 2023

dckc Apr 11, 2023

dckc Apr 11, 2023

turadg Apr 11, 2023 •

edited

Loading

Chris-Hibbert Apr 11, 2023

turadg Apr 11, 2023

Chris-Hibbert Apr 11, 2023

dckc Apr 11, 2023

dckc Apr 11, 2023

dckc Apr 11, 2023

dckc Apr 11, 2023

dckc Apr 11, 2023

dckc Apr 11, 2023

dckc commented Apr 11, 2023

dckc commented Apr 11, 2023

Chris-Hibbert left a comment

turadg Apr 11, 2023

turadg Apr 11, 2023

turadg Apr 11, 2023

turadg Apr 11, 2023

dckc Apr 13, 2023

turadg left a comment

turadg Apr 13, 2023

dckc commented Apr 14, 2023

-  await snapshotStorage(t, driver.mockChainStorage);
+  await snapshotStorage(t, driver.mockChainStorage, {'mockChainStorageRoot.thisAuction': 'published.auction'}, `Under "published", the "auction" node is delegated to
+the auctioneer contract, which publishes data as follows.
+See also board marshalling conventions (_to appear_).
+` );

	const psmStorageNode = await makeStorageNodeChild(chainStorage, 'psm');
	const storageNode = E(
	E(psmStorageNode).makeChildNode(Stable.symbol),
	).makeChildNode(keyword);

		const note = `Under "published", the "committees.Economic_Committee" node is delegated to
		a committee contract, which publishes data as follows.

		pattern: 'mockChainStorageRoot.thisElectorate.',
		replacement: 'published.committees.Economic_Committee.',

	pattern: 'mockChainStorageRoot.thisElectorate.',
	replacement: 'published.committees.Economic_Committee.',
	rekey: {'mockChainStorageRoot.thisElectorate': published.committees.Economic_Committee`},

test(inter-protocol): snapshot storage/RPC API for auction, vaultFactory, reserve, psm, committees #7368

test(inter-protocol): snapshot storage/RPC API for auction, vaultFactory, reserve, psm, committees #7368

Conversation

dckc commented Apr 10, 2023 • edited Loading

Description

Security / Scaling Considerations

Documentation Considerations

Testing Considerations

turadg left a comment

Choose a reason for hiding this comment

Chris-Hibbert commented Apr 10, 2023

Chris-Hibbert 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

turadg Apr 11, 2023 • 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

dckc commented Apr 11, 2023

dckc commented Apr 11, 2023

Basis points vs. Ratios

Chris-Hibbert 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

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc commented Apr 14, 2023

dckc commented Apr 10, 2023 •

edited

Loading

turadg Apr 11, 2023 •

edited

Loading