-
Notifications
You must be signed in to change notification settings - Fork 204
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
No fake baggage #7636
No fake baggage #7636
Conversation
@@ -9,10 +9,15 @@ import { resolve as importMetaResolve } from 'import-meta-resolve'; | |||
|
|||
import '../../exported.js'; | |||
|
|||
// eslint-disable-next-line import/no-extraneous-dependencies |
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.
if it's a dep please include it in package.json. but I don't think it needs to be a dep…
const { zoeService: zoe } = makeZoeKit( | ||
makeScalarBigMapStore('zoe baggage', { durable: true }), | ||
fakeVatAdmin, | ||
); |
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 common enough that there should be a single function to do it. perhaps makeZoeKitForTest
in a tools
export. Then most of these changes would be four chars at the call site (and an extra import, but on a package that's already a dependency).
Moreover, Zoe's baggage should always be durable, so the caller shouldn't be allowed to pass in any MapStore.
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 introduced a makeZoeForTest
, makeZoeKitForTest
and common setupZoeForTest
, and refactored all the tests to use these new tools. Its was a big refactor, but I think it paid out!
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.
Looks it to me!
*/ | ||
const start = async (zcf, privateArgs) => { | ||
const start = async (zcf, privateArgs, baggage) => { |
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 leave this contract alone because it's just an example, not part of production
agoric-sdk/packages/zoe/src/contracts/priceAggregator.js
Lines 47 to 52 in bb30947
* @deprecated use fluxAggregator | |
* | |
* This contract aggregates price values from a set of oracles and provides a | |
* PriceAuthority for their median. This naive method is game-able and so this module | |
* is a stub until we complete what is now in `fluxAggregatorKit.js`. | |
* |
It is confusing that zoe/src/contracts
has examples and production contracts. We have deferred cleaning up package factoring #4645
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.
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.
Reverted, and annotated contract to be notForProductionUse
@@ -34,9 +33,11 @@ import { makePriceAuthorityTransform } from '../contractSupport/priceAuthorityTr | |||
*/ | |||
export const prepare = async ( | |||
zcf, | |||
{ quoteMint = makeIssuerKit('quote', AssetKind.SET).mint } = {}, | |||
{ quoteMint: providedQuoteMint } = {}, |
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 don't see anywhere that this privateArg is provided. I think we're better off excising the privateArgs param. Please confirm @dckc .
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.
yes, if it were important we would/should have had a test.
@michaelfig if you know something we haven't considered, please let us know.
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.
Removed
* @param {import('@agoric/vat-data').Baggage} baggage | ||
* @param {ERef<Mint<'set'>>} [providedQuoteMint] | ||
*/ | ||
export const prepareQuoteMint = (baggage, providedQuoteMint) => { |
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.
when we drop the privateArg (see below) I think this will be better factored as a provideQuoteMint
.
@@ -460,6 +460,7 @@ export const publishAgoricNames = async ( | |||
const marshaller = E(board).getPublishingMarshaller(); | |||
|
|||
// XXX will fail upon restart, but so would the makeStoredPublishKit this is replacing | |||
// XXX is that ok? |
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.
yes, there's only one production use of makeStoredPublishKit
which is vote outcomes and we've elected to defer durability of governance.
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.
Updated with comment explaining why this is ok
@@ -29,6 +29,7 @@ import { assert } from '@agoric/assert'; | |||
const start = zcf => { | |||
const { tokenName = 'token' } = zcf.getTerms(); | |||
// Create the internal token mint | |||
// makeIssuerKit fails upgrade, safe because demo only? |
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.
in this case yes
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.
marked contract as notForProductionUse
packages/vats/src/vat-mints.js
Outdated
@@ -39,6 +39,7 @@ export function buildRootObject() { | |||
* @param {[AssetKind?, DisplayInfo?]} issuerArgs | |||
*/ | |||
makeMintAndIssuer: (issuerNameSingular, ...issuerArgs) => { | |||
// makeIssuerKit fails upgrade, safe because demo only? |
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'm not sure. I see this in demo and devnet configs but also
agoric-sdk/packages/vats/decentral-core-config.json
Lines 35 to 38 in bb30947
"mints": { | |
"sourceSpec": "@agoric/vats/src/vat-mints.js" | |
}, | |
"priceAuthority": { |
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.
decentral-core-config.json
is also non-production. See the $comment
at the top.
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.
vat-mints is very much demo-only. It has the poison pill that is enforced by static analysis in a test:
import { notForProductionUse } from '@agoric/internal/src/magic-cookie-test-only.js';
test('no test-only code is in production configs', async t => { |
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 notForProductionUse
is pretty cool. I added it to the other zoe contracts with makeIssuerKit
that are supposed to not end up in production.
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.
These look like nice improvements!
}), | ||
makeZoeKit( | ||
makeScalarBigMapStore('zoe baggage', { durable: true }), | ||
makeFakeVatAdmin(() => {}).admin, |
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.
Nit: we could probably clarify the purpose of this empty function by naming it something like ignoreTestContext
(and likewise in packages/smart-wallet/test/supports.js).
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 empty function was not needed in the first place. Gone in the refactor
Once this is green, I will rebase and fix up the commits, and mark as ready for review. |
bdb380c
to
caa9e77
Compare
@turadg it looks like since I removed the optional
|
It's there to test that the types are defined so we can just give a different invalid argument. E(zoe).startInstance(
scaledPriceInstallation,
validIssuers,
// @ts-expect-error includes an extra term
{ ...validTerms, extra: 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.
LGTM. Thanks for the clean commits.
I left a separate comment on how to get types passing.
@@ -460,6 +460,8 @@ export const publishAgoricNames = async ( | |||
const marshaller = E(board).getPublishingMarshaller(); | |||
|
|||
// XXX will fail upon restart, but so would the makeStoredPublishKit this is replacing | |||
// Since we expect the bootstrap vat to be replaced instead of upgraded this should be |
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.
✏️
export const provideQuoteMint = baggage => { | ||
/** @type {ERef<Mint<'set'>>} */ | ||
let baggageQuoteMint; | ||
if (baggage.has(`quoteMintIssuerBaggage`)) { |
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 a const
That won't do because |
842dde2
to
4cd1c0e
Compare
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.
LGTM
Reveals a non-upgradability issue of Zoe
4cd1c0e
to
ff38968
Compare
closes: #7578
closes: #7627
closes: #7651
Best reviewed commit-by-commit
Description
This is the result of a manual audit to track down and remove places where a fake baggage is created, or at least to explicitly contain it to tests, tools and demo contracts
In particular this PR does the following:
makeBoard
tomakeFakeBoard
. I realized after the fact this was the same as reduce psuedo baggage #7627, but is slightly more complete and passes testsmakeDurableZoeKit
which required a baggage, and deprecatemakeZoeKit
(with the removed options to specify a baggage). I also introduced a newmakeZoeForTest
tool and wrote a sharedsetupZoeForTest
tool that creates a fake baggage, reducing the uses ofmakeDurableZoeKit
to this test helper and the productionvat-zoe
vat-zoe
implementations used in tests and use the production one instead.buildZoe
params in the baggage. Fixes Zoe is not null-upgradable #7651quoteMint
from priceAuthority'sprivateArgs
, and unconditionally create a durable issuer kit for it.makeIssueKit
as not for productionSecurity Considerations
None
Scaling Considerations
None
Documentation Considerations
None
Testing Considerations
There is a new test tool for Zoe, which should be used instead of
makeZoeKit
, which is now deprecated.Because of the
makeFakeBoard
rename, the following PR is required to land in documentation repo: