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

Remove POC Zoe metering #4222

Closed
wants to merge 1 commit into from
Closed

Remove POC Zoe metering #4222

wants to merge 1 commit into from

Conversation

erights
Copy link
Member

@erights erights commented Dec 28, 2021

A step towards #4225 . You can review that one against master, which is probably more coherent. Or you can review these individually.

@erights erights self-assigned this Dec 28, 2021
@erights erights changed the title WIP Remove Zoe metering Remove Zoe metering Dec 29, 2021
@erights erights marked this pull request as ready for review December 29, 2021 02:51
@dckc
Copy link
Member

dckc commented Dec 29, 2021

Did I miss a memo? Why remove Zoe metering?
Is there an issue I can look at for context?

@erights
Copy link
Member Author

erights commented Dec 29, 2021

Did I miss a memo? Why remove Zoe metering? Is there an issue I can look at for context?

No. Came up first in a discussion I had with @warner. I then discussed it in this morning's eng sync. It's on the agenda for tomorrow's (oops, today's) Zoe meeting, where we'll discuss it. Could you be there? (we'll record in any case)

Summary:

  • We don't need it for mainnet 1,
  • it is a significant source of complexity, and
  • removing it was almost purely subtractive.
  • The removal caused no compensating engineering down a throw-away dead end.

The Least Authority review was pre-metering. We're about to go into a purple teaming effort on Zoe, where I'm supposed to be blue, and I do not know enough to try to defend that code. And it is a distraction from what we should be worried about for mainnet 1.

For mainnet 1, as we discussed at the all-hands, our plans to prevent DOS and runaways is vat-level, not contract-level anyway. Let's get this in place first and get some experience with it.

Finally, we plan to replace Zoe 1 with Zoe 2 after mainnet 1. That seems like the right time to take this on for production. Because Zoe 2 starts so much simpler, we're more likely to be able to add these additional concerns with higher confidence.

Copy link
Contributor

@Chris-Hibbert Chris-Hibbert left a comment

Choose a reason for hiding this comment

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

This looks good.

We should talk about the order of merging between this and my rearranging of the run-protocol contracts.

@dckc
Copy link
Member

dckc commented Dec 29, 2021

  • We don't need it for mainnet 1,

How about for devnet? That is: how do smart contract developers get a feel for how much their contracts will cost to run?

@erights erights changed the title Remove Zoe metering Remove POC Zoe metering Dec 29, 2021
@erights
Copy link
Member Author

erights commented Dec 29, 2021

  • We don't need it for mainnet 1,

How about for devnet? That is: how do smart contract developers get a feel for how much their contracts will cost to run?

We discussed in the kernel meeting. There isn't enough value here to make it worth keeping, given that:

  • We need to have kernel/vat level metering and fees by mainnet 1 anyway.
  • The current zoe API with expiry is wrong anyway (though as Dean points out this could be removed while leaving the rest intact)
  • The important devnet zoe API will be zoe 2.

So we decided to move ahead with this PR together with #4225.

@erights
Copy link
Member Author

erights commented Dec 29, 2021

This looks good.

We should talk about the order of merging between this and my rearranging of the run-protocol contracts.

We talked and decided that if this can go in today, which seems likely, then it should go first.

@erights
Copy link
Member Author

erights commented Dec 29, 2021

#4225 was built on this one and was merged to master, making this PR irrelevant. Closing.

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.

3 participants