-
Notifications
You must be signed in to change notification settings - Fork 118
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
Call init()
during deploy()
, just once
#543
Conversation
init()
during deploy()
, just once
src/lib/zkapp.ts
Outdated
initUpdate.update.appState.some(({ isSome }) => !isSome.toBoolean()) | ||
) { | ||
console.warn(`WARNING: the \`init()\` method was called without overwriting the entire state. This means that your zkApp will lack | ||
the \`isProved\` status which certifies that the current state was verifiably produced by proofs (and not arbitrarily set by the zkApp developer). |
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 wonder if there's a link we can point to for more information about isProved
. If this resource exists, it would be great to include it here too imo :D
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.
Good idea! Btw I realized its actually called provedState
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.
Well, not sure if there's a resource right now O_o
src/lib/zkapp.ts
Outdated
@@ -178,6 +179,7 @@ function wrapMethod( | |||
accountUpdates: [], | |||
fetchMode: inProver() ? 'cached' : 'test', | |||
isFinalRunOutsideCircuit: false, | |||
numberOfRuns: Infinity, |
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 do we use Infinity
inside a checked computation here?
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.
err.. I introduced this numberOfRuns
field on the transaction because the transaction code is run twice, and I wanted to identify whether I was in the first one, so that I wouldn't display the same warning twice. However, then in prove / compile I had no good idea how to set it; I just wanted it to be more than 0. So I opted for Infinity
. Maybe 2 would also be fine
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 we can introduce a type to have the constraints that it can only be the value 1
, 2
or an ignore value. That way it's easier to tell what numberOfRuns
should be at any point in time. Up to you though!
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.
agreed, a 1 | 2 | 'ignore'
or something like slightly more structured could be nice here
+1 sounds great
But this would change the behavior away from our discussed goals, no? It seems that the solution described earlier in this RFC, as opposed to in this last paragraph, is more ideal and in line with the user experience we discussed. |
The motivation would be to fulfill the original goal for introducing I don't think this would harm any of our other goals, besides
Other than that, @jasongitmail is there anything else you don't like about init being a method by default? (I'm sure we'll still have a way to change that default) |
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 very well-thought out and looks fantastic to me. I wonder if we can rip most of this description directly into our docs when this lands.
src/lib/zkapp.ts
Outdated
@@ -178,6 +179,7 @@ function wrapMethod( | |||
accountUpdates: [], | |||
fetchMode: inProver() ? 'cached' : 'test', | |||
isFinalRunOutsideCircuit: false, | |||
numberOfRuns: Infinity, |
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.
agreed, a 1 | 2 | 'ignore'
or something like slightly more structured could be nice here
@mitschabaude (re: the last paragraph of the RFC) TLDR I'm +1 on the RFC, looks great and includes the goal behavior, but highlighting that the last paragraph would be a totally different DX. So I'm -1 for accepting the last paragraph as something we will plan to do without further discussion, bc it's a totally different DX than we discussed as the goal, and with further discussion we could come up with other alternatives like allowing the dev to sepcify a flag during deployment if they know they want isProved set: |
@jasongitmail gotcha, we will discuss this further, but I want to point out that making init a method actually has no influence on the ability to reinitialize, since usually the smart contract wouldn't allow updating the state without proof anyway, after deployment |
The solution to this that I highlighted above is rolling back the permissions and the state, which means writing a custom transaction anyway. So it turns out this question is kind of orthogonal to the defaults for init |
init()
RFCThis proposes a simple, incremental change which is intended to
(with users I mean zkApp developers)
The logic that I propose is as follows:
init()
. It is, by default, not a@method
provedState
to befalse
.The
init
method can be overridden by the user, to define custom state initialization logic. It's suitable for that because init will only run on the first deploy, and not on subsequent deploys. A typical smart contract could look like this:Note that this contract doesn't have a custom deploy method. There's no need for it, because we can set custom permissions in
init
as well (there's no reason to set their value again in re-deployments). I expect this to be the default pattern used for smart contracts going forward: no deploy, but a custom init method.With the defaults, init is not a
@method
, and doesn't create a proof. Not being a method also has the side effect that init will not create its own account update, but will reuse the deploy account update. This is actually very important! Because, by default, zkApps have theeditState
permission set toproof
. That means that init, without a proof, could not change the state if it were a separate account update after deploy.Another nice side effect of having both in one update is that the user could still have state initialization logic in deploy. This pattern has been used in many examples so we can't expect it to immediately go away:
Note that here, the base init is called during
super.deploy()
. Luckily, the usual pattern when overriding deploy is to put your logic aftersuper.deploy()
. This means that your changes to the state will come after init (which here sets the state to zero; if it came after, it would wipe out your initialization). This is why I don't expect major disruption from this change for users who continue to use the old pattern and don't override init.Some users will want to use
init()
slightly differently and make it a method:Because methods always create their own account update (otherwise they couldn't create a proof about it), the effect of the decorator is that there is now a second account update, separate from deploy, which just exercises the init logic. It will have a proof created for it, and after running this, the
provedState
attribute on the account is true.How do we ensure that malicious end users don't use init to wipe out the state?
If there is no
@method
decorator, normal users won't be able to run init because state updates without proof or signature will not be allowed to them.If there is a method decorator, then the
provedState
attribute will become true after the first deployment. In this state, nobody can run init, because of the precondition it has that provedState = false.Minor qualification: Since
provedState
is not available yet from graphQL, as a temp solution I actually don't set that precondition. (It's commented out.) The temp solution is that init takes an optionalprivateKey
argument, and if this argument is provided, the base init checks that this private key belongs to the zkApp public key. Therefore, a developer can easily ensure, already with this PR, that no-one else can run init, by passing the private key to the base init. (See thesimple_zkapp.ts
example in this PR. It's fine to ignore this paragraph, because there's already a PR up for exposingprovedState
on graphQL.Can a developer change their mind and init the state a second time, with different state?
It depends: If the
editState
permission is set toproof
, then the state can't be changed after deployment. However, with our current defaults, the permission itself can be rolled back, and so can the state. The same is true if a developer is using a@method
for init. In fact, the question of when a developer can change the state later has nothing to do with init.What if we forget to call
super
?If we don't call
super.init()
, then the base logic which sets the whole state to zero doesn't run. This is not ideal, because in the proving case, we won't getprovedState=true
. This is why I implemented a warning: After init was run, deploy will look if the entire state was updated, and if not, print the following:Shouldn't
init()
be a@method
, and make a proof, by default?I think it should eventually, but having it opt-in is fine as an incremental step. Note that, when the base init is a
@method
, then the state is silently wiped afterdeploy()
even for developers who aren't aware of init and are still using the old pattern of initializing state in deploy. Therefore, it feels better to ramp up developers slowly on init so that it becomes wide-spread, before we make such a change which makes the old pattern obsolete.