-
Notifications
You must be signed in to change notification settings - Fork 221
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: start PSM contracts with metrics, governance from previous published state #7480
Conversation
I just realized: it's probably more straightforward to mix the |
68269dc
to
cf2f84e
Compare
fixes: #6645 - compute total fees - style: keep to jessie-check in startPSM - use vatPowers.chainStorageEntries in restorePSM
cf2f84e
to
d65b3bd
Compare
back to draft... I think I can avoid minting IST in bootstrap |
mint the fees within the PSM contract
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lookin good!
|
||
export { inviteCommitteeMembers, startEconCharter, inviteToEconCharter }; | ||
|
||
/** | ||
* @param {EconomyBootstrapPowers & WellKnownSpaces} powers | ||
* Decode vstorage value to CapData | ||
* XXX already written somewhere? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like it must be. agoric/casting does some of this, but I don't see a piece matching this interface.
If it's really new, I think there's a better home for it than startPSM. maybe in agoric/internal?
Longer term I think @endo/marshal should have something like assertCapData
, which is the bulk of this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split decodeToCapData
into assertCapData
and parsedValuesFromStreamCellText
and put them in board-utils.js
with makeHistoryReviver
.
* | ||
* @param {unknown} value | ||
*/ | ||
const decodeToCapData = value => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is deserializing not decoding and the function name should specify what it operates on (the vstorage value.)
consider deserializeVstorageValue
. That it produces CapData is loosely implied by that being what went into vstorage. would help to have that as the @returns
as well and simply return data
at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to generalize isStreamCell
.
/**
* @param {string} cellText
* @returns {any[]}
*/
const parsedValuesFromStreamCellText = cellText => {
assert.typeof(cellText, 'string');
const cell = /** @type {{blockHeight: string, values: string[]}} */ (
JSON.parse(cellText)
);
assert(isObject(cell));
const { values } = cell;
assert(Array.isArray(values));
const parsedValues = values.map(value => JSON.parse(value));
return harden(parsedValues);
};
And then separately assert that length of the returned array is 1 and that its sole element is a capdata object, either manually (e.g., const maybeCapData = obj => matches(obj, M.splitRecord(harden({body: M.string(), slots: M.array()})));
) or slower but future-proofed like const isCapData = obj => { defaultMarshaller.fromCapData(obj); return true; };
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is deserializing not decoding
I'm curious where the boundary is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TMK,
Serialization is converting objects to a stream of data.
Encoding is converting one stream of data to another stream of data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've been informally reserving encode/decode for processes in which input and output are both data structures, even though serialize/deserialize (in which the output or input, respectively, is a sequence of primitive values such as bytes or characters) are technically a subset of those more general transformations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted above, I split decodeToCapData
into assertCapData
and parsedValuesFromStreamCellText
and put them in board-utils.js
with makeHistoryReviver
.
* @param {Array<[string, string]>} entries | ||
* @param {(slot: string, iface?: string) => any} [slotToVal] | ||
*/ | ||
export const makeHistoryReviver = (entries, slotToVal = undefined) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider putting this in vat/tools/board-utils.js
// In this reviver, object references are revived as boardIDs | ||
// from the pre-bulldozer board. | ||
const toSlotReviver = makeHistoryReviver(chainStorageEntries); | ||
if (!toSlotReviver.has(`published.psm.${Stable.symbol}.${keyword}.metrics`)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this vstorage path is repeated several times, and in the last time it has IST
hard-coded. please pull out consts for the path strings to name them
* @param {Array<[key: string, value: string]>} chainStorageEntries | ||
* @param {string} keyword | ||
* @param {{ minted: Brand<'nat'>, anchor: Brand<'nat'> }} brands | ||
* @returns {{ metrics?: MetricsNotification, governance?: Record<string, *> }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
source of confusion that the return value doesn't match the contents of what was published. please move the .current
lookup to the caller and type this:
* @returns {{ metrics?: MetricsNotification, governance?: Record<string, *> }} | |
* @returns {{ metrics?: MetricsNotification, governance?: GovernanceSubscriptionState }} |
give: { Anchor }, | ||
} = seat.getProposal(); | ||
stableMint.mintGains({ Minted: target.feePoolBalance }, feePool); | ||
atomicRearrange(zcf, harden([[seat, anchorPool, { Anchor }, { Anchor }]])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something like this is clearer:
atomicRearrange(zcf, harden([[seat, anchorPool, { Anchor }, { Anchor }]])); | |
atomicTransfer(zcf, seat, anchorPool, { Anchor }); |
* | ||
* Taken from mainnet; for example, the 1st value comes from... | ||
* | ||
* `agd --node https://main.rpc.agoric.net:443 query vstorage data published.agoricNames.brand -o json | jq .value` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
* | ||
* @type {Array<[key: string, value: string]>} | ||
*/ | ||
const chainStorageEntries = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please pull this and anchorAssets
out into a fixtures module
test('restore PSM: decode metrics, governance with old board IDs', async t => { | ||
const toSlotReviver = makeHistoryReviver(chainStorageEntries); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is is testing makeHistoryReviver
, not anything about PSM per se. (it's testing the shape of what was published to vstorage, but only assuming the fixture is correct.)
if you move makeHistoryReviver remember to move this test with it.
|
||
for (const [name, installation] of Object.entries({ | ||
...installs, | ||
psm: installs.psmInstall, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please just name the key psm
so this isn't a special case
...makeSet( | ||
entries | ||
.map(([k, _]) => k) | ||
.filter(k => k.length > prefix.length && k.startsWith(prefix)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.filter(k => k.length > prefix.length && k.startsWith(prefix)) | |
.filter(k => k.startsWith(prefix)) |
const capData = decodeToCapData(raw); | ||
return harden(board.fromCapData(capData)); | ||
}; | ||
const children = prefix => [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this missing validation that prefix
ends with "." to avoid e.g. children("foo")
not filtering out a path like "foolish.donkey"?
The history has gotten a little choppy; I suppose I'll use automerge:squash. |
I think I responded to everything. PTAL, @turadg . @gibson042 too, if you like. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The history has gotten a little choppy; I suppose I'll use automerge:squash.
fwiw automerge:rebase recently learned to squash fixups. assuming they all squash cleanly.
assert(data); | ||
assert.typeof(data.body, 'string'); | ||
assert(Array.isArray(data.slots)); | ||
// XXX check that the .slots array elements are actually strings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would suffice to check just the first one. though there might not be any.
alternately you could be honest about what this function is asserting ;-)
* @returns {asserts data is import('@endo/marshal').CapData}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CapData
without any <>
parsed as any
somehow. I tried being honest with CapData<unknown>
but that clashed with a type elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would suffice to check just the first one.
between friends, I suppose
harden(deserializeVstorageValue); | ||
|
||
/** | ||
* Provide access to object graphs serialized in vstorage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good doc
); | ||
await E(seat).getPayouts(); | ||
}; | ||
await (oldState.metrics && restoreMetrics(oldState.metrics)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oic. normally I'd expect,
if (oldState.metrics) {
await restoreMetrics(oldState.metrics));
}
but I suppose that trips on the jessie awaits rule
yeah; that's really handy. But I didn't keep good |
closes: #6645
Description
Given metrics and governance state published from previous VM (
chainStorageEntries
from #7156), start matching PSM contracts.Refine
startPSM
andmakeAnchorAsset
to consultchainStorageEntries
.Security Considerations
anchorPoolBalance
E(creatorFacet).makeRestoreMetricsInvitation()
method that allows the caller to assign{ mintedPoolBalance, totalAnchorProvided, totalMintedProvided, feePoolBalance }
to arbitrary (pattern-matched) values, minting IST forfeePoolBalance
.Scaling Considerations
none
Documentation Considerations
None, I think; that is: I think this is consistent with what BLD stakers would expect without further documentation.
Testing Considerations
Light unit testing seems sufficient, given that
startPSM
will only be called in very constrained circumstances.