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

Preconditions #207

Merged
merged 50 commits into from
Jun 9, 2022
Merged

Conversation

mitschabaude
Copy link
Contributor

@mitschabaude mitschabaude commented May 26, 2022

TODOs:

  • change integration test so that it tests fetching from the graphql URI, and uses preconditions, and ideally is broken up into zkapps that test different features
  • investigate broken prover and decide if it should be fixed as part of this issue
  • some precondition fields aren't implemented yet because the logic to parse them from an encoded string is missing. this could be done here or in a follow-up PR

@mitschabaude mitschabaude marked this pull request as ready for review June 3, 2022 10:16
Copy link
Contributor Author

@mitschabaude mitschabaude left a comment

Choose a reason for hiding this comment

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

Some comments to make review easier:

@@ -218,34 +245,6 @@ function stringifyAccount(account: FlexibleAccount): FetchedAccount {
};
}

async function checkResponseStatus(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this code block was just moved to the bottom of the file. I often move unimportant / helper-type code to the bottom, and the exported API / most important parts to the top, because I think this makes code more readable

@@ -317,6 +347,163 @@ function addCachedAccountInternal(
};
}

async function fetchLastBlock(graphqlEndpoint = defaultGraphqlEndpoint) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this provides the logic to get the network precondition. we fetch the bestChain, which returns a list of blocks, and take the network state from the last of them. is this OK @bkase?

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 it in one graphql call — bestChain optionally takes a parameter for the number to pull and goes through a happy path if you put 1 there (executes much faster), so we should add that! Then you can skip the query for the block at a specific height.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

very nice, I did that and it became much faster!

},
}: FetchedBlock): NetworkValue {
return {
snarkedLedgerHash: Field.zero, // TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

some of the network preconditions are not implemented yet, because we lack the logic for parsing these hashes into Fields

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 make accessing the unimplemented parts of network/account preconditions throw an error instead of silently using the wrong value? This will certainly confuse people I think if we don’t do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👍🏻

@@ -380,3 +567,31 @@ async function makeGraphqlRequest(
return [undefined, inferError(error)] as [undefined, FetchError];
}
}

async function checkResponseStatus(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

code moved here from the top of the file

inProver,
inCompile,
inCheckedComputation,
};

// per-smart-contract context for transaction construction
type ExecutionState = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this code was moved here from zkapp.ts. this continues two themes:

  • breaking apart zkapp.ts because it was too big
  • doing book-keeping with WeakMap instead of secret properties on public-facing objects

}

function assertPreconditionInvariants(party: Party) {
let context = getPreconditionContextExn(party);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the function that throws an error if the user used .get() on a account / network field, without explicitly adding a precondition. calling get would have added an entry to context.read. adding a precondition would have added an entry to context.constrained.

/**
* The constant [[`Bool`]] that is `true`.
*/
static true: Bool;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added Bool.true and Bool.false, because in the end it won the community vote :D

Copy link
Member

Choose a reason for hiding this comment

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

Nice

src/lib/int.ts Outdated
@prop value: Field;
static NUM_BITS = 64;
type = 'UInt64' as const;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this field to UInt64, to make the type conform to the interface { value: Field; type: "UInt64" }, which is used to signify UInt64s in the autogenerated party types.
the intention is to make the UInt64 interface distinct from the UInt32 one, to get advanced types in precondition.tsto work. this also fixes a couple of bug sources, where UInt32 could be passed into functions that expected UIn64 and vice versa

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

Can we use a phantom type here instead of actually polluting the object at runtime? https://dev.to/busypeoples/notes-on-typescript-phantom-types-kg9 You can put the type parameter on the interface to avoid people having to see it when they use Uint64 .

Copy link
Member

Choose a reason for hiding this comment

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

See below, I think I solved it nicely

src/lib/int.ts Outdated

constructor(value: Field) {
super();
this.value = value;
// TODO type breaks Circuit.if for some reason
delete (this as any).type;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we actually can't keep the .type property on the class instance.. it's just for typescript

this.isSelf = isSelf;
}

static clone(party: Party) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before adding this, Partys were just cloned with cloneCircuitValue directly. I changed it because the new account and network fields aren't safely clone-able, due to how they use closures / are attached to a party instance. This new cloning will lose precondition-related metadata attached to a party (i.e. which account / network fields were accessed / constrained). this seems appropriate, because cloning a party also doesn't magically add it to the current transaction

Copy link
Member

Choose a reason for hiding this comment

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

👍

@mitschabaude mitschabaude requested a review from bkase June 7, 2022 09:50
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.

I don’t know if I believe in the WeakMap over storing secret things on the object.
The benefit is you can easily see everything associated with that entity by looking at the class. When there are WeakMap’s floating around, you have to know where to look to find the extra associated data.

Also, popular and well-thought-out libraries do this (like React), so I don’t think it’s controversial.

What is your reason for reaching for weakmaps here?

If we do end up sticking with WeakMaps, having just one for all the secret things at least mitigates the “hard to find” associated data problem a bit. Rather than one for each kind of associated datum.

@@ -317,6 +347,163 @@ function addCachedAccountInternal(
};
}

async function fetchLastBlock(graphqlEndpoint = defaultGraphqlEndpoint) {
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 it in one graphql call — bestChain optionally takes a parameter for the number to pull and goes through a happy path if you put 1 there (executes much faster), so we should add that! Then you can skip the query for the block at a specific height.

}
}`;

type FetchedBlock = {
Copy link
Member

Choose a reason for hiding this comment

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

I look forward to the day when we autogenerate this like we did with transaction. Unfortunately, today is not that day. This is good for now.

},
}: FetchedBlock): NetworkValue {
return {
snarkedLedgerHash: Field.zero, // TODO
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 make accessing the unimplemented parts of network/account preconditions throw an error instead of silently using the wrong value? This will certainly confuse people I think if we don’t do this.

src/lib/int.ts Outdated
@prop value: Field;
static NUM_BITS = 64;
type = 'UInt64' as const;
Copy link
Member

Choose a reason for hiding this comment

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

👍

src/lib/int.ts Outdated
@prop value: Field;
static NUM_BITS = 64;
type = 'UInt64' as const;
Copy link
Member

Choose a reason for hiding this comment

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

Can we use a phantom type here instead of actually polluting the object at runtime? https://dev.to/busypeoples/notes-on-typescript-phantom-types-kg9 You can put the type parameter on the interface to avoid people having to see it when they use Uint64 .

| OptionLayout<BaseLayout>
| BaseLayout;

// TS magic for computing flattened precondition types
Copy link
Member

Choose a reason for hiding this comment

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

These types are awesome haha, reminds me of doing this kind of stuff when I was doing web things at Pinterest

}

// helper. getPath({a: {b: 'x'}}, 'a.b') === 'x'
// TODO: would be awesome to type this
Copy link
Member

Choose a reason for hiding this comment

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

microsoft/TypeScript#12290 😉 (no need to do this now unless you feel an itch for cleanliness)

/**
* The constant [[`Bool`]] that is `true`.
*/
static true: Bool;
Copy link
Member

Choose a reason for hiding this comment

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

Nice

@@ -12,8 +12,8 @@ export {

export { toJson, toJsonLeafTypes, toFields, toFieldsLeafTypes };

type UInt64 = { value: Field };
type UInt32 = { value: Field };
type UInt64 = { value: Field; type: 'UInt64' };
Copy link
Member

Choose a reason for hiding this comment

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

Ah I think I solved it with phantom types cleanly:

type FieldLike<N> = { value: Field }
type UInt64 = FieldLike<{type: 'UInt64'}>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately doesn't seem to solve my use case.. I need UInt64 to not extend UInt32 and vice versa.

I tried the following snippet:

type FieldLike<N> = { value: Field };
type UInt64 = FieldLike<{ type: 'UInt64' }>;
type UInt32 = FieldLike<{ type: 'UInt32' }>;
type X = UInt32 extends UInt64 ? 'not working' : 'it works!';

For me, X is of type "not working"..

Copy link
Contributor Author

@mitschabaude mitschabaude Jun 8, 2022

Choose a reason for hiding this comment

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

I think the original solution can be improved by making the type prop optional:

type UInt64 = { value: Field; type?: 'UInt64' };
type UInt32 = { value: Field; type?: 'UInt32' };

that way, they don't extend each other and we can just instantiate them without the type field at runtime.

it's still not perfect because the type field shows up in intellisense

Copy link
Member

Choose a reason for hiding this comment

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

That's weird that they extend each other, shows that I do not understand typescript's type system enough.

I think this is good with them optional -- since it appears in intellisense can you use some kind of prefix so people know to avoid it: like _type or $type (I'm not sure if we have a standard for this yet)

src/lib/int.ts Outdated
@prop value: Field;
static NUM_BITS = 64;
type = 'UInt64' as const;
Copy link
Member

Choose a reason for hiding this comment

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

See below, I think I solved it nicely

@mitschabaude
Copy link
Contributor Author

mitschabaude commented Jun 8, 2022

I don’t know if I believe in the WeakMap over storing secret things on the object. (...) Also, popular and well-thought-out libraries do this (like React), so I don’t think it’s controversial.

Ok, this is good feedback. I might have overreached with the intention to remove all secret props. Specifically, I'm not sure if I can defend the move of SmartContract._executionState to a weakmap, because that prop mainly gets used internally by the SmartContract class. In that case, I guess it's more natural to make it a private class field, so it's removed from the TS type / doesn't pollute intelli-sense, but is accessible to the class that uses it.

What is your reason for reaching for weakmaps here?

If we do end up sticking with WeakMaps, having just one for all the secret things at least mitigates the “hard to find” associated data problem a bit. Rather than one for each kind of associated datum.

I do think there are cases where weakmaps are better than extra class props, and it's also better to not have them in one place.

For example, the precondition.ts module stores metadata associated with each Party instance in a weakmap. I like the weakmap approach for that because it makes the metadata book-keeping be localized to the module that is concerned with it - precondition.ts. Keeping modules encapsulated helps with understanding and maintaining the code. In this case, seeing a map of precondition-related metadata being initialized on the Party class in party.ts would be more confusing, because there's no other code in party.ts related to that metadata; and when trying to understand the precondition.ts code, it would be confusing that we don't see where that metadata is initialized. It would mix concerns of the Party class with concerns of the precondition module, and the downside is increased code complexity.

An alternative would be to secretly set a property (party as any)._preconditionData = ... inside precondition.ts, but I think this is just not as clean and not as well-typed as maintaining a weakmap for that data. It breaks the "contract" that Party defines, and forces us to use as any whenever we deal with the secret property.

The same reasoning applies for the @state logic that's now encapsulated in state.ts. We used to store _states and _layout() fields on SmartContract subclasses from inside the @state logic. I think it's strictly less confusing and more maintainable now that we have a state-related weakmap inside state.ts, and not mess with the SmartContracts from that module.

Since only precondition.ts / state.ts ever need to be concerned with their respective metadata, I don't see the value of having a central weakmap which combines their stuff. Also, the keys and values of that centralized weakmap would be a mix of completely different things if we do that, which would make accurate typing much harder.

TL;DR, I think both of those weakmaps help with encapsulation, code clarity and strict typing.

@bkase, what do you think of a middle-ground approach where we don't try to avoid private props in principle, but we use weakmaps if a module wants to associate data with objects that originate in a different module? I would revert the _executionState weakmap in that case.

@bkase
Copy link
Member

bkase commented Jun 8, 2022

Renaming the type? to _type? (or something) + refactoring that WeakMap for executionState and then I'll approve 💯

@mitschabaude mitschabaude merged commit e992c87 into feature/preconditions-refactor Jun 9, 2022
@mitschabaude mitschabaude mentioned this pull request Jun 9, 2022
@mitschabaude mitschabaude deleted the feature/preconditions branch June 30, 2022 07:52
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.

Account / network precondition RFC
2 participants