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

Use regex match for merkle root comparison #45

Closed
wants to merge 2 commits into from

Conversation

unlimit
Copy link

@unlimit unlimit commented Jul 16, 2018

fixes issue #26

@raiseandfall
Copy link
Member

@unlimit Thanks for the contribution! Can you add some tests here to cover that change? https://github.com/blockchain-certificates/cert-verifier-js/blob/master/tests/testChecks.js

@unlimit
Copy link
Author

unlimit commented Jul 18, 2018

@raiseandfall done!

@ghost
Copy link

ghost commented Jul 18, 2018

Hey @unlimit, thanks for adding the test.

One thing I would like to see, if it's okay, is a test on a sample certificate. We don't have any cases for us to test out smart contract transactions, and our cert-issuer doesn't currently issue these types of transactions. I'd want to make sure your certificate and any others sent to smart contracts verify with any refactoring coming up in the near future.

@unlimit
Copy link
Author

unlimit commented Jul 19, 2018

@AnthonyRonning yes, here is our sample certificate.
And Ethereum TX for it

@lemoustachiste
Copy link
Member

@unlimit I think what @AnthonyRonning meant was that he would like to have a test covering your use case (something a bit larger than a unit test), so that we could have confidence we would not break your case when refactoring (which we are doing since we are already working on the v2).

@unlimit
Copy link
Author

unlimit commented Jul 25, 2018

@lemoustachiste thank you for the details. Honestly I don't see any sense in larger test covering. (It is very simple change and unit test should completely cover it)
Merkle root comparison is a question(not an issue) to Blockcerts protocol(specially to v2):

  1. Should it be strict?
    In this case, js and python verifier must use the same rules. Also issuer will not be able to issue certificate via ethereum smart contract methods.
  2. Should it be soft?
    This can be very handy, but maybe you have some reasons to block this? Any security concerns?

@ghost
Copy link

ghost commented Jul 25, 2018

Yes, @lemoustachiste, thanks for clarifying, that's what I meant.

@unlimit, I'm asking for the benefit of both of us here, not necessarily for the code change involved. Thank for for providing that unit test, and I agree it should cover it.

Our lm-cert-issuer open source does not issue to smart contracts, so we do not have any coverage for this use case. It would be nice to have it, but I guess since our open source doesn't provide this functionality, we shouldn't worry about covering others' specific implementations of blockcerts? Thoughts @kimdhamilton?

@raiseandfall
Copy link
Member

Digging down in old opened PRs, what should we do here @AnthonyRonning @kimdhamilton ?

@lemoustachiste
Copy link
Member

closing as this has been open forever with no action from op

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.

3 participants