-
Notifications
You must be signed in to change notification settings - Fork 142
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
Sequencing events #274
Sequencing events #274
Conversation
mitschabaude
commented
Jul 6, 2022
•
edited
Loading
edited
- Closes Sequencing support (a.k.a. sequence events) #88
- RFC: Sequence Events RFC #265
src/lib/circuit_value.ts
Outdated
@@ -467,3 +469,39 @@ function circuitValueEquals<T>(a: T, b: T): boolean { | |||
([key, value]) => key in b && circuitValueEquals((b as any)[key], value) | |||
); | |||
} | |||
|
|||
function pickOne<T, A extends AsFieldElements<T>>( |
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.
handy function that I needed for the dynamic length stuff, and exported as Circuit.pickOne
. A less general version was previously in string.ts
@@ -254,7 +256,21 @@ function LocalBlockchain({ | |||
return { wait: async () => {} }; | |||
}, | |||
async transaction(sender: SenderSpec, f: () => void) { | |||
return createTransaction(sender, f); | |||
// bad hack: run transaction just to see whether it creates proofs | |||
// if it doesn't, this is the last chance to run SmartContract.runOutsideCircuit, which is supposed to run only once |
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 introduced SmartContract.runOutsideCircuit
for an earlier version of the state rollup example, where I needed to take values from the @method
run and store them outside. It was pretty hacky to get that working -- e.g. it's important that this runs only once (in the "user expectation" sense of "once"). Left it in the API since it may be useful another time and could get much less hacky with the some planned changes (parties-returning and async-method-running)
src/lib/zkapp.ts
Outdated
); | ||
} | ||
let methodData = contract.analyzeMethods(); | ||
let possibleUpdatesPerTransaction = [ |
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.
one of the magical things this function does is it knows how many events each of the smart contract methods emits, so it can perform computations for each of those numbers-of-events and then pick the actual one.
to enable that, I added SmartContract.analyzeMethods
in this file and synthesizeMethodArguments
in proof_systems.ts
src/lib/zkapp.ts
Outdated
}; | ||
}; | ||
|
||
function getStateUpdate<S, U>( |
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.
here and below is where the stateUpdate
API is actually implemented!
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 still needs updating to the latest version of the RFC before we should land it even under the "experimental" flag -- and don't forget to change the names of things everywhere! I stopped mentioning it after I realized you haven't updated this yet.
src/lib/circuit_value.ts
Outdated
@@ -467,3 +469,39 @@ function circuitValueEquals<T>(a: T, b: T): boolean { | |||
([key, value]) => key in b && circuitValueEquals((b as any)[key], value) | |||
); | |||
} | |||
|
|||
function pickOne<T, A extends AsFieldElements<T>>( |
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 this should be select
or switch
or case
? pickOne
doesn't feel right to me
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.
switch is great -- analogous to Circuit.if
src/examples/state_update_rollup.ts
Outdated
// version that's implicitly represented by the list of updates | ||
@state(Field) counter = State<Field>(); | ||
// helper field to store the point in the update history that our on-chain state is at | ||
@state(Field) stateHash = State<Field>(); |
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.
let's change this to whatever you pick on the RFC-side like actionsHash
or actionsCommitment
src/examples/state_update_rollup.ts
Outdated
this.stateUpdate.emit(increment); | ||
} | ||
|
||
@method rollupStateUpdate() { |
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.
-> reduce
@@ -0,0 +1,15 @@ | |||
import { Bool, Circuit, isReady, shutdown, Int64 } from '../../dist/server'; | |||
|
|||
describe('circuit', () => { |
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.
can you add tests expecting failures if there are more than one true and zero true?
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.
done. for some reason returning 0 for zero true seemed natural to me. but I can also throw in that case if you think that's better
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 point yeah 0 is fine, but can you add a test for the zero case succeeding too?
if (doProofs) await tx.prove(); | ||
tx.send(); | ||
// update internal state | ||
pendingActions.push([INCREMENT]); |
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.
We can do this later when we revisit this API to move it out of experimental, but shouldn't we have some way of reaching into the reducer inside the zkapp to pull the pendingActions out for testing? It's brittle to need to remember to push the actions.
I really like how this library handles testing reducers https://pointfreeco.github.io/swift-composable-architecture/main/documentation/composablearchitecture/teststore
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.
yeah I agree. I think this would be achieved if reducer.getActions
-- which will fetch actions in the real network interaction -- gets them from memory in the LocalBlockchain case
@@ -0,0 +1,15 @@ | |||
import { Bool, Circuit, isReady, shutdown, Int64 } from '../../dist/server'; | |||
|
|||
describe('circuit', () => { |
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 point yeah 0 is fine, but can you add a test for the zero case succeeding too?
src/lib/zkapp.ts
Outdated
stateType: AsFieldElements<State>, | ||
reduce: (state: State, action: Action) => State, | ||
initial: { state: State; actionsHash: Field }, | ||
advancedOptions?: { maxTransactionsWithActions?: number } |
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 nice
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.
probably just options?
is fine