DO NOT MERGE - just commentary and questions as I read #4553
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,29 +1,67 @@ | ||
| // @ts-check | ||
|
|
||
| import { deeplyFulfilled } from '@endo/marshal'; | ||
|
|
||
| // const { entries, fromEntries, keys, values } = Object; | ||
| const { entries, fromEntries } = Object; | ||
|
|
||
| // Why the `Collect` object? In any case, it was missing a `harden` or `Far`. | ||
| // But why not just export the useful functions directly? | ||
|
|
||
| // Huh. I didn't know you could put the @template at the bottom. | ||
| // See `objectMap` in objArrayConversion.js | ||
| // Read the qualifiers in the comment! | ||
| /** | ||
| * @param {Record<string, V>} obj | ||
| * @param {(v: V) => U} f | ||
| * @returns {Record<string, U>} | ||
| * @template V | ||
| * @template U | ||
| */ | ||
| export const mapValues = (obj, f) => | ||
| harden(fromEntries(entries(obj).map(([p, v]) => [p, f(v)]))); | ||
| harden(mapValues); | ||
|
|
||
| /** | ||
| * @param {X[]} xs | ||
| * @param {Y[]} ys | ||
| * @returns {[X, Y][]} | ||
| * @template X | ||
| * @template Y | ||
| */ | ||
| export const zip = (xs, ys) => harden(xs.map((x, i) => [x, ys[i]])); | ||
| harden(zip); | ||
|
|
||
| // Are you sure you only want one level? | ||
| // See deeplyFulfilled in deeplyFulfilled.js | ||
| // /** | ||
| // * @param {Record<string, ERef<V>>} obj | ||
| // * @returns {Promise<Record<string, V>>} | ||
| // * @template V | ||
| // */ | ||
| // export const allValues = async obj => { | ||
| // // Get both keys and values before the `await`, so they're consistent | ||
| // // (assuming that `obj` is not a proxy, which we don't validate) | ||
| // const ks = keys(obj); | ||
| // const vs = await Promise.all(values(obj)); | ||
| // return harden(fromEntries(zip(ks, vs))); | ||
| // }; | ||
| // harden(allValues); | ||
|
|
||
| // This should have worked, but was passed something non-hardened | ||
| // export const allValues = deeplyFulfilled; | ||
| // So I tried the following instead | ||
| export const allValues = obj => deeplyFulfilled(harden(obj)); | ||
|
There was a problem hiding this comment. I left this uncommented for now, and lines 37-49 commented above, so the resulting failures will appear under CI. See comment below for the bug causing these failures. There was a problem hiding this comment. Note that this There was a problem hiding this comment. deeplyFulfilled loses the types of its args. So when I know I'm dealing with a I can work on the |
||
| harden(allValues); | ||
|
|
||
| // This didn't work either. Investigating why led to a | ||
| // missing `harden` in endo, in the last line of `bundleZipBase64`. | ||
| // ```js | ||
| // return { endoZipBase64, moduleFormat: 'endoZipBase64' }; | ||
| // ``` | ||
| // should be | ||
| // ```js | ||
| // return harden({ endoZipBase64, moduleFormat: 'endoZipBase64' }); | ||
| // ``` | ||
| // With this fixed, the second replacement of `allValues` above does | ||
| // work. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,7 @@ import '../exported.js'; | |
|
|
||
| import { PROTOCOL_FEE_KEY, POOL_FEE_KEY } from './vpool-xyk-amm/params.js'; | ||
|
|
||
| import { allValues, mapValues } from './collect.js'; | ||
|
|
||
| const { entries, keys } = Object; | ||
|
|
||
|
|
@@ -50,8 +50,8 @@ export const startEconomicCommittee = async ( | |
| const bundles = await governanceBundles; | ||
| keys(bundles).forEach(key => assert(shared.contract[key])); | ||
|
|
||
| const installations = await allValues( | ||
| mapValues(bundles, bundle => E(zoe).install(bundle)), | ||
| ); | ||
|
|
||
| const [installAdmin, instanceAdmin] = await collectNameAdmins( | ||
|
|
@@ -167,6 +167,7 @@ export const setupAmm = async ({ | |
| E(instanceAdmin).update('ammGovernor', g.instance), | ||
| ]); | ||
| }; | ||
| harden(setupAmm); | ||
|
There was a problem hiding this comment. I'm not surprised to see lots of functions exported but not hardened. Fortunately it is not very dangerous. But it is a hazard we need to avoid. It leaves mutable top-level state, such that two parties importing the same function from the same module instance can now communicate without calling the function. This isn't POLA and breaks reasoning by separation. There was a problem hiding this comment.
Could missing There was a problem hiding this comment. Yes, there are consistent rules:
Automatic detection is an open research problem, IIUC. agoric-labs/jessica#35 There was a problem hiding this comment.
On #b, We have tried several times to write down static rules that would enforce these abstract rules. I think @michaelfig has grammatical restrictions for pure Jessie modules somewhere. @michaelfig ? On #a, a conservative rule that works almost all the time is to just put a Occasionally, we do have valid code where we locally mutate the object before hardening it, but still harden it before it might escape. To enforce that statically would involve treating all variables holding the object as "owning" variables in the reference-capability sense. Or as linear or affine variables or something. The key is that we statically know that the object hasn't escaped via an alias before it's been hardened. We have sketched static rules for this but never got very far. Fortunately, the need for such delayed hardening happens even more rarely than I expected. We can almost always simply follow the conservative harden-the-literal rule above. There was a problem hiding this comment. On #b, a conservative rule is to only have top level const declarations of hardened things. Again, this conservative rule works almost always. For exported functions, we follow a pattern of a slightly delayed harden: export const foo = x => x * x;
harden(foo);This still hardens There was a problem hiding this comment. Thanks @dckc . I answered before I saw your response. There was a problem hiding this comment. Would an imperfect but worthwhile rule be that named exports from a module have a corresponding E..g, export const sayHi = () => {
console.log("hi");
};PASS export const sayHi = () => {
console.log("hi");
};
harden(sayHi);
// eslint-disable-next-line endo/hardened-exports
export const foo = {};EDIT: I also was typing while @erights was. If I'm reading "this conservative rule works almost always" right then the answer is yes that we could have a lint rule for that. When it doesn't work how obvious is it? Would someone writing the module know to be able to work around it or suppress the lint? Or would might they ship something overly hardened and break downstream. (With or without the lint we need developers to know when to harden, so I don't suppose linting could be any worse and it should help both educate devs and help catch things they missed.) There was a problem hiding this comment. You may need to accept |
||
|
|
||
| /** | ||
| * @param {BootstrapPowers & { | ||
|
|
@@ -222,7 +223,7 @@ export const startVaultFactory = async ( | |
| ); | ||
|
|
||
| const bundles = await vaultBundles; | ||
| const installations = await allValues({ | ||
| VaultFactory: E(zoe).install(bundles.VaultFactory), | ||
| liquidate: E(zoe).install(bundles.liquidate), | ||
| }); | ||
|
|
@@ -307,12 +308,13 @@ export const startVaultFactory = async ( | |
| ), | ||
| ]); | ||
| }; | ||
| harden(startVaultFactory); | ||
|
|
||
| /** @param { BootstrapPowers } powers */ | ||
| export const configureVaultFactoryUI = async ({ | ||
| consume: { agoricNames, nameAdmins, board, zoe }, | ||
| }) => { | ||
| const installs = await allValues({ | ||
| amm: E(agoricNames).lookup('installation', 'amm'), | ||
| vaultFactory: E(agoricNames).lookup('installation', 'VaultFactory'), | ||
| contractGovernor: E(agoricNames).lookup('installation', 'contractGovernor'), | ||
|
|
@@ -325,11 +327,11 @@ export const configureVaultFactoryUI = async ({ | |
| 'binaryVoteCounter', | ||
| ), | ||
| }); | ||
| const instances = await allValues({ | ||
| amm: E(agoricNames).lookup('instance', 'amm'), | ||
| vaultFactory: E(agoricNames).lookup('instance', 'VaultFactory'), | ||
| }); | ||
| const central = await allValues({ | ||
| brand: E(agoricNames).lookup('brand', CENTRAL_ISSUER_NAME), | ||
| issuer: E(agoricNames).lookup('issuer', CENTRAL_ISSUER_NAME), | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,12 @@ const makeResult = (latestInterestUpdate, interest, newDebt) => ({ | |
| newDebt, | ||
| }); | ||
|
|
||
| // What about leap years? | ||
| // https://www.grc.nasa.gov/www/k-12/Numbers/Math/Mathematical_Thinking/calendar_calculations.htm | ||
| // says the average Gregorian year is 365.2425 | ||
| // whereas the astronomical figure (tropical year) is 365.2422 | ||
| // No mention made of the occasional leap second! | ||
| // Whether we care depends on what we do with it. | ||
|
There was a problem hiding this comment. I'll admit this is overly pedantic and probably doesn't matter at all. But I am curious if, for our use, there is an unambiguous right answer. There was a problem hiding this comment. I had wondered the same thing. It's a negligible difference and for loans the regulations allow treating interest in a leap year as any other.
|
||
| export const SECONDS_PER_YEAR = 60n * 60n * 24n * 365n; | ||
| const BASIS_POINTS = 10000; | ||
| // single digit APR is less than a basis point per day. | ||
|
|
||
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 reformatted only because I tried an experiment of adding `"bundles/install-on-chain.js" to this list, so it would no longer be exempt from linting.