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

Hash inputs in JS #310

Merged
merged 22 commits into from
Aug 1, 2022
Merged

Hash inputs in JS #310

merged 22 commits into from
Aug 1, 2022

Conversation

mitschabaude
Copy link
Collaborator

@mitschabaude mitschabaude commented Jul 30, 2022

Copy link
Collaborator 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 guide review:

@@ -0,0 +1,179 @@
import {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is a sort of unit test, which is also run in Mina CI. I left it here just because for the Mina version, I had to remove the TS types, and it's easier to read & modify with the types in place

@@ -139,7 +139,7 @@ function toPermission(p: AuthRequired): Permission {
type FetchedAccount = {
publicKey: string;
nonce: string;
tokenId: string;
token: string;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this file has some unrelated fixes which turned up when debugging snarkyjs transactions against a local Mina node, while debugging timestamp preconditions

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch on figuring out these fixes!


type Input = { fields?: Field[]; packed?: [Field, number][] };

type TokenSymbol = { symbol: string; field: Field };
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

new TokenSymbol type!

* Convert the {fields, packed} hash input representation to a list of field elements
* Random_oracle_input.Chunked.pack_to_fields
*/
function packToFields({ fields = [], packed = [] }: Input) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the last step that's necessary to convert the result of toInput to something which can be hashed

let fields = Types.Party.toFields(this);
return Ledger.hashPartyFromFields(fields);
let input = Types.Party.toInput(this);
return hashWithPrefix(prefixes.body, packToFields(input));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is the main achievement of this PR!

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome work!

@@ -57,8 +57,6 @@ let unimplementedPreconditions: LongKey[] = [
'account.isNew',
// this is partially unimplemented because the field is not returned by the local blockchain
'account.delegate',
// this is unimplemented because setting this precondition made the integration test fail
'network.timestamp',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed the timestamp precondition, so removed it from the list of unimplemented preconditions!

@@ -257,7 +257,7 @@ To write a correct circuit, you must avoid any dependency on the concrete value
throw Error(
'fetch can only be called when the State is assigned to a SmartContract @state.'
);
if (Mina.currentTransaction !== undefined)
if (Mina.currentTransaction.has())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unrelated fix

let party = Party.createSigned(feePayerKey);
let feePayerAddress = feePayerKey.toPublicKey();
let party = Party.defaultParty(feePayerAddress);
party.body.useFullCommitment = Bool(true);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unrelated fix, necessary for intg test

@@ -52,183 +56,115 @@ function asFieldsAndAux<T, TJson>(typeData: Layout, customTypes: CustomTypes) {
};
}

function toJson(typeData: Layout, value: any, customTypes: CustomTypes): any {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lots of refactoring here -- see the new mapReduce function at the bottom of the file, which abstracts what most of the functions here were doing!

Base automatically changed from feature/composability-rebased to main August 1, 2022 09:12
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.

Looks good! Just one naming nit

);
}

type MapReduceSpec<T, R> = {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is semantically more of a fold ala Fields.fold in the ocaml code that’s generated from deriving fields. I think something like FoldSpec and genericFold or layoutFold?

let delegate = PrivateKey.random().toPublicKey();
update.delegate.value = delegate;

party.tokenSymbol.set('BLABLA');
Copy link
Contributor

Choose a reason for hiding this comment

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

Hahaha! This token symbol is funny.

jsLayout.Party.entries.body.entries.preconditions.entries.network
);
let network = party.body.preconditions.network;
party.network.stakingEpochData.ledger.hash.assertEquals(Field.random());
Copy link
Contributor

Choose a reason for hiding this comment

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

What does invoking assertEquals on a random Field element do here?

Copy link
Collaborator Author

@mitschabaude mitschabaude Aug 1, 2022

Choose a reason for hiding this comment

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

It changes a precondition on the Party. I'm trying to add as much changes to the party here as possible, to not miss a part of it where the hashing logic doesn't agree between JS and OCaml (if I were just using the default party that I start out with, the hash input would be mostly zeros -- so two of those zeros could be switched and we wouldn't notice)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok that makes sense.

@mitschabaude mitschabaude merged commit d180ddb into main Aug 1, 2022
@mitschabaude mitschabaude deleted the feature/compatible-hashing branch August 1, 2022 21:12
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.

Fix setting timestamp precondition
4 participants