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

Sequencing events #274

Merged
merged 27 commits into from
Jul 15, 2022
Merged

Sequencing events #274

merged 27 commits into from
Jul 15, 2022

Conversation

mitschabaude
Copy link
Member

@mitschabaude mitschabaude commented Jul 6, 2022

@mitschabaude mitschabaude mentioned this pull request Jul 7, 2022
@@ -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>>(
Copy link
Member Author

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
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 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 = [
Copy link
Member Author

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>(
Copy link
Member Author

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!

Copy link
Member

@bkase bkase left a 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.

@@ -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>>(
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 should be select or switch or case? pickOne doesn't feel right to me

Copy link
Member Author

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

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

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

this.stateUpdate.emit(increment);
}

@method rollupStateUpdate() {
Copy link
Member

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

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?

Copy link
Member Author

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

Copy link
Member

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

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

Copy link
Member Author

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

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

Choose a reason for hiding this comment

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

this is nice

Copy link
Member

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

@mitschabaude mitschabaude merged commit 8e5adf1 into main Jul 15, 2022
@mitschabaude mitschabaude deleted the feature/sequence-events branch July 15, 2022 06:57
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.

Sequencing support (a.k.a. sequence events)
2 participants