Skip to content

store review records without publishing them #36

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

Merged
merged 3 commits into from
Feb 7, 2018
Merged

Conversation

fazo96
Copy link
Collaborator

@fazo96 fazo96 commented Feb 6, 2018

This PR contains the ground work for making #24 work:

storeReviewRecord has a new publish boolean option that is true by default but when set to false it stores the review record in IPFS without doing the following steps (actually advertising/publishing the review)

Originally we wanted to use IPFS to calculate the resulting multihash without writing the content to IPFS but it looks like this might not be possible using js-ipfs yet. I opened an issue about this: ipfs/js-ipfs#1205 We will be able to quickly revisit this part when there is a way to do that

Another new option to storeReviewRecord is expectedMultihash. If this is provided, then the library will halt publishing if the storeReviewRecord operation results in a multihash different than expected. This is a failsafe measure to avoid accidentally publishing the wrong review after having written its multihash to the blockchain

So to complete the job we just need to update chlu-demo to making reviews using this process:

  • calling storeReviewRecord with publish: false. This will error out if there are any problems in the review record and returns the multihash if there aren't
  • creating the blockchain transaction with the multihash just provided
  • calling storeReviewRecord with expectedMultihash so that this time it gets published. This will error out if something changed and the review record multihash is different now

Internally, stuff about publishing has been taken out to a different function making storeReviewRecord a shorter method. There are also new tests for all the new use cases

@kulpreet if this is ok I'll update the chlu-demo

@fazo96 fazo96 self-assigned this Feb 6, 2018
@fazo96 fazo96 requested a review from kulpreet February 6, 2018 12:54
@fazo96 fazo96 changed the title store review reocords without publishing them store review records without publishing them Feb 6, 2018
@kulpreet kulpreet merged commit ad3d022 into master Feb 7, 2018
@fazo96 fazo96 deleted the compute-hash branch March 16, 2018 10:03
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.

2 participants