Skip to content
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

Closed
wants to merge 33 commits into from
Closed

6701 ec oracles list #6963

wants to merge 33 commits into from

Conversation

turadg
Copy link
Member

@turadg turadg commented Feb 10, 2023

closes: #6701
refs: #XXXX

Description

  • refactor(fluxAggregator): split singleton / contract
  • chore(types): BundleCap
  • feat: puppetGovernance setup tools
  • feat(price): fluxAggregator governance

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

@turadg turadg requested review from dckc and Chris-Hibbert February 10, 2023 00:46
@turadg turadg force-pushed the 6701-ec-oracles-list branch from e3590f0 to c463cf7 Compare February 10, 2023 06:09
Comment on lines 233 to 234
// FIXME different facet that peeks under governance
const aggregatorFacet = E(aggregator.creatorFacet).getCreatorFacet();
Copy link
Member

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.

Comment on lines 193 to 195
const aggregator = await E(zoe).startInstance(
priceAggregator,
/** @type {Installation<import('@agoric/governance/src/contractGovernor.js').start>} */
(contractGovernor),
Copy link
Member

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:

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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?
Copy link
Contributor

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',
);

/** */
Copy link
Contributor

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',
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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)

Comment on lines 75 to 80
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for merge?

Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const trace = makeTracer('FluxAgg');
const trace = makeTracer('FluxAgg', false);

Copy link
Member Author

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',
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

@turadg turadg force-pushed the 6701-ec-oracles-list branch from c463cf7 to c024a4a Compare February 10, 2023 21:02
@turadg turadg force-pushed the 6701-ec-oracles-list branch from c024a4a to be64930 Compare February 10, 2023 23:31
Copy link
Member

@dckc dckc left a 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

Comment on lines 85 to 90
addOracle: oracleId => fa.creatorFacet.initOracle(oracleId),
/**
*
* @param {string} oracleId
*/
removeOracle: oracleId => fa.creatorFacet.deleteOracle(oracleId),
Copy link
Member

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.

Comment on lines +3 to +4
// eslint-disable-next-line no-unused-vars -- used by TS
import { coalesceUpdates } from '@agoric/smart-wallet/src/utils.js';
Copy link
Member

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) => {
Copy link
Member

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) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likewise currentInvitationPurseBalance?

Copy link
Member

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:

const getBalanceFor = brand =>
E(E(bank).getPurse(brand)).getCurrentAmount();

/** @type {import('@agoric/smart-wallet/src/invitations').ContinuingInvitationSpec} */
const proposeInvitationSpec = {
source: 'continuing',
previousOffer: 44,
Copy link
Member

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?

Copy link
Member

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]),
Copy link
Member

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 /////////////////////////
Copy link
Member

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...

Suggested change
// 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] });
Copy link
Member

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,
Copy link
Member

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: {},
Copy link
Member

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

@turadg
Copy link
Member Author

turadg commented Feb 12, 2023

Superceded by #6975 and #6977.
I should have closed this to convey that. I left it open because I have some other commits I want to salvage into other PRs, but I can do that with it closed.

@turadg turadg closed this Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EC can propose and vote on changes to oracle operators list
3 participants