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

removeOperator for fast-usdc #10755

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

removeOperator for fast-usdc #10755

wants to merge 2 commits into from

Conversation

turadg
Copy link
Member

@turadg turadg commented Dec 20, 2024

refs: #10754

Description

This has a couple fixes for removing operators,

It shouldn't close that issue until it has a scenario test (in /boot). Reviewers, should that come in another PR so this can land or should this include that test?

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

Copy link

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9f3f27b
Status: ✅  Deploy successful!
Preview URL: https://76174888.agoric-sdk.pages.dev
Branch Preview URL: https://10754-removeoperator.agoric-sdk.pages.dev

View logs

const feedKit = makeFeedKit();
const { op1 } = await makeOperators(feedKit);
const evidence = MockCctpTxEvidences.AGORIC_PLUS_OSMO();

// works before disabling
op1.operator.submitEvidence(evidence);

op1.admin.disable();
await feedKit.creator.removeOperator('op1');
Copy link
Member

Choose a reason for hiding this comment

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

maybe this means makeOperator can lose the awkward admin thing altogether?

Copy link
Member Author

Choose a reason for hiding this comment

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

removeOperator calls operatorKit.admin.disable().

What's awkward about it?

Copy link
Member

Choose a reason for hiding this comment

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

I mean makeOperator in the test file.

// operator only gets `.invitationMakers`
// but for testing, we need `.admin` too. UNTIL #?????

Copy link
Member Author

Choose a reason for hiding this comment

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

that test no longer needs admin but it's still true that OperatorKit has an admin facet and the operator invitation offer result is an OperatorKit. I'm not sure what changes you propose.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest that result of makeOperator in the test file should not include an .admin property; the returned value should represent what an operator can do, and operators don't have admin authority.

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 think the change you're asking for is to app, not just tests.

The current design is that operators do get the admin facet of the operator kit. (My recollection from earlier; I'm on a phone now)

Right now that means they can disable themselves. Not a bad thing but I agree they shouldn't get a facet called admin.

So in this PR I'll attenuate the kit to omit admin in the offer result.

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.

2 participants