Skip to content
This repository has been archived by the owner on May 15, 2021. It is now read-only.

Feat/HashingLogic #10

Merged
merged 21 commits into from Sep 17, 2018
Merged

Feat/HashingLogic #10

merged 21 commits into from Sep 17, 2018

Conversation

edhedges
Copy link
Contributor

@edhedges edhedges commented Sep 14, 2018

Adds a HashingLogic export, which can be used to perform attestation hashes between bloom-web and attestation-kit. Also includes merkletree support.

@edhedges
Copy link
Contributor Author

@ipatka @djvs
I'd like to get some feedback on more types, functions, etc. you'd like to see added in this or some subsequent PRs to better implement the spec.

'0x5c836525f55b64ede01a232fafde74afa7a8a82b47ecfe599294317971b2f4b5',
}

test('HashingLogic matches', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe also a test with an array of more than 1 attestation 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.

Will do


export const hashAttestations = (attestations: IAttestationData[]) => {
const individualAttestationHashes = attestations.map(a =>
soliditySha3({
Copy link
Contributor

Choose a reason for hiding this comment

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

choosing soliditySha3 wasn't a fully educated decision. I just used it because it was available. is there an argument for using a different one? if not, this is fine

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 didn't do any research into using a different one. I guess I just figured no reason to change it at this time. My goal here was to move duplicate hashing logic and types to this middleman library.

Copy link
Contributor

Choose a reason for hiding this comment

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

right, keeping it consistent makes sense

value: string
}

export const getAttestationAgreement = (
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what gets passed to signTypedData right? This will change when we use the merged EIP712 spec. Fine for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate on how?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or point me in a direction where I can educate myself.

Copy link
Contributor

Choose a reason for hiding this comment

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

getFormattedSignTypedDataLegacy vs not legacy in the signingLogic.ts file in bloom-web. Will chat on slack

@ipatka
Copy link
Contributor

ipatka commented Sep 14, 2018

we'll also want to add all those sub-types like 'full-name', 'birth-date', 'gender' once everyone approves of the spec

Copy link
Contributor

@ipatka ipatka left a comment

Choose a reason for hiding this comment

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

Looks great! Reminder for future reference that the EIP712 version of getAttestationAgreement will need to be finalized once metamask is ready

@edhedges edhedges merged commit e263c47 into master Sep 17, 2018
@edhedges edhedges deleted the feat/hashing branch September 17, 2018 22:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants