-
Notifications
You must be signed in to change notification settings - Fork 228
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
6701 ec oracles list #6963
6701 ec oracles list #6963
Conversation
e3590f0
to
c463cf7
Compare
// FIXME different facet that peeks under governance | ||
const aggregatorFacet = E(aggregator.creatorFacet).getCreatorFacet(); |
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.
IIRC, this gets the limited creator facet, so it's right. no FIXME needed.
const aggregator = await E(zoe).startInstance( | ||
priceAggregator, | ||
/** @type {Installation<import('@agoric/governance/src/contractGovernor.js').start>} */ | ||
(contractGovernor), |
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 instance that comes back here is the contract governor instance.
To get the governed instance and such, follow the PSM example:
agoric-sdk/packages/inter-protocol/src/proposals/startPSM.js
Lines 159 to 163 in c463cf7
const [psm, psmCreatorFacet, psmAdminFacet] = await Promise.all([ | |
E(governorFacets.creatorFacet).getInstance(), | |
E(governorFacets.creatorFacet).getCreatorFacet(), | |
E(governorFacets.creatorFacet).getAdminFacet(), | |
]); |
@@ -12,6 +13,8 @@ import { ParamChangesQuestionDetailsShape } from './typeGuards.js'; | |||
|
|||
const { Fail } = assert; | |||
|
|||
const trace = makeTracer('CGov'); |
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.
const trace = makeTracer('CGov'); | |
const trace = makeTracer('CGov', false); |
@@ -301,6 +317,8 @@ const start = async (zcf, privateArgs) => { | |||
}); | |||
|
|||
const publicFacet = Far('contract governor public', { | |||
// BEFOREMERGE is this okay? why not? |
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 it's not okay.
It's not a POLA violation, but it does violate separation of concerns. Code that wants to call public methods on the governed contract should be able to get that public facet. What does it simplify to mix those methods in here?
'@agoric/zoe/src/contracts/automaticRefund.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.
??
@@ -57,7 +57,7 @@ export const defaultProposalBuilder = async ( | |||
brandOutRef: brandOut && publishRef(brandOut), | |||
priceAggregatorRef: publishRef( | |||
install( | |||
'@agoric/inter-protocol/src/price/fluxAggregator.js', | |||
'@agoric/inter-protocol/src/price/fluxAggregator.contract.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.
Was there a decision I missed on renaming contracts?
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.
no this is new. I'm finding it common to have a contract have a singleton and it seems silly to have separate names for them. E.g. vaultFactory has vaultDirector, but it's just one more name to keep track of. We often talk about their behavior with one name. So I think it better to keep the same common name for both but qualify one as the 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.
okay. What's the argument for fluxAggregator.contract.js rather than fluxAggregatorContract.js? I think the latter is more in keeping with our standard style. It seems odd to call some contracts out with .contract of we're not doing that in general.
I guessed that this was the start at a plan to rename all contracts, which sounds somewhat plausible, but I hadn't heard the arguments either way.
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.
What's the argument for fluxAggregator.contract.js rather than fluxAggregatorContract.js?
A contract file is not just any JS file because it has to have an export of start
or prepare
. I expect to also eventually autogenerate documentation about contracts from the contract file. I'd like to be able to autogenerate a report of contracts within a package.
The above could be done by looking for a start
or prepare
export, but that doesn't necessarily mean it's a contract. There should be some way to declare that some code is a contract that can be read cheaply. (e.g. no type analysis to see whether it implements the API)
console.log('DEBUG searching invitations', invitations, 'for', instance); | ||
|
||
const matches = invitations.filter( | ||
details => | ||
description === details.description && instance === details.instance, | ||
details => description === details.description, | ||
// FIXME DONOTMERGE instance isn't matching | ||
// && instance === details.instance, |
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.
Not for merge?
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.
Yep that's what I mean when I write DONOTMERGE :)
This is a draft. Thanks for looking it over.
* storageNode: ERef<StorageNode>, | ||
* }} privateArgs | ||
* @param {Baggage} baggage | ||
* @param {TimerService} timerPresence |
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.
Why timerPresence? We don't usually call out presences as a distinct type. Would it be sufficient to use timerP to indicate that it's remote? Is there some other timer
facet to distinguish it from?
*/ | ||
|
||
/** | ||
* PriceAuthority for their median. Unlike the simpler `priceAggregator.js`, this approximates |
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.
* PriceAuthority for their median. Unlike the simpler `priceAggregator.js`, this approximates | |
* PriceAuthority that produces a median. Unlike the simpler `priceAggregator.js`, this approximates |
import { E } from '@endo/eventual-send'; | ||
import { provideFluxAggregator } from './fluxAggregator.js'; | ||
|
||
const trace = makeTracer('FluxAgg'); |
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.
const trace = makeTracer('FluxAgg'); | |
const trace = makeTracer('FluxAgg', false); |
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 going to leave the debugging in until this is R4R
const oracleTimer = aggregator.manualTimer; | ||
|
||
const pricePushAdminA = await E(aggregator.creatorFacet).initOracle( | ||
'agorice1priceOracleA', |
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 having trouble reading these strings.I parse them as "agoric", "e1", "priceOracle", "A". What's the "e1" about? Why isn't the "E" capitalized? It's just a doc-string, but my eyes stumble over it every time.
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.
e1
is a typo. agoric1
is how all addresses start. priceOracleA
is meant to be the unique identifier. The reason to include agoric1
is to convey that it is a (mock) BEC32 address.
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 was hoping it would be something like that. thanks.
c463cf7
to
c024a4a
Compare
This reverts commit 24f23f0.
c024a4a
to
be64930
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.
test looks good so far
addOracle: oracleId => fa.creatorFacet.initOracle(oracleId), | ||
/** | ||
* | ||
* @param {string} oracleId | ||
*/ | ||
removeOracle: oracleId => fa.creatorFacet.deleteOracle(oracleId), |
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 inconsistency between addOracle
and initOracle
(and likewise removeOracle
and deleteOracle
) seems like a little sand in the gears.
Tolerable, though, I suppose.
// eslint-disable-next-line no-unused-vars -- used by TS | ||
import { coalesceUpdates } from '@agoric/smart-wallet/src/utils.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.
something like @typedef { Awaited<ReturnType<import('@agoric/smart-wallet/src/utils.js').coalesceUpdates>> } WalletState
might avoid increasing the bundle size... oh... this is a test, so there's no relevant bundle. NVM.
* @param {Awaited<ReturnType<typeof coalesceUpdates>>} state | ||
* @param {Brand<'nat'>} brand | ||
*/ | ||
export const purseBalance = (state, brand) => { |
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.
Call this invitationPurseBalance
? Since #6807 , won't work on any other brand, right?
* @param {import('@agoric/smart-wallet/src/smartWallet.js').CurrentWalletRecord} record | ||
* @param {Brand<'nat'>} brand | ||
*/ | ||
export const currentPurseBalance = (record, brand) => { |
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.
likewise currentInvitationPurseBalance
?
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 you really do need the balance of some other brand, use the relevant bank directly, as in:
agoric-sdk/packages/inter-protocol/test/smartWallet/contexts.js
Lines 69 to 70 in adc0836
const getBalanceFor = brand => | |
E(E(bank).getPurse(brand)).getCurrentAmount(); |
/** @type {import('@agoric/smart-wallet/src/invitations').ContinuingInvitationSpec} */ | ||
const proposeInvitationSpec = { | ||
source: 'continuing', | ||
previousOffer: 44, |
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.
a number works here? offer ids are strings these days, 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.
ah...
OfferSpec: M.splitRecord(
{
id: M.or(M.number(), M.string()),
source: 'continuing', | ||
previousOffer: 44, | ||
invitationMakerName: 'VoteOnApiCall', | ||
invitationArgs: harden([feed, 'addOracle', [newOracle], 2n]), |
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.
maybe const deadline = 2n
? It took me a while to realize that's what 2n is doing here.
await offersFacet.executeOffer(proposalOfferSpec); | ||
await eventLoopIteration(); | ||
|
||
// vote ///////////////////////// |
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 using t.log
in place of most comments of this sort...
// vote ///////////////////////// | |
t.log('now the EC member(s) vote') |
).getDetails(); | ||
t.is(electionType, 'api_invocation'); | ||
const yesPosition = harden([positions[0]]); | ||
t.deepEqual(issue, { apiMethodName: 'addOracle', methodArgs: [newOracle] }); |
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.
Displaying this legibly in the EC UI seems like an interesting challenge. I guess identicons / hashicons can help.
|
||
const voteInvitationDetails = await getInvitationFor( | ||
'Voter0', | ||
1, |
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 yesPosition
? maybe add a comment to that effect?
const voteOffer = { | ||
id: 47, | ||
invitationSpec: getVoteSpec, | ||
proposal: {}, |
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 makes me wonder if proposal
should be optional
closes: #6701
refs: #XXXX
Description
Security Considerations
Scaling Considerations
Documentation Considerations
Testing Considerations