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

feat: add revocation store fragment #94

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nebulis
Copy link
Contributor

@Nebulis Nebulis commented Jun 3, 2020

UNEXPECTED_ERROR = 0,
DOCUMENT_REVOKED = 1,
CONTRACT_ADDRESS_INVALID = 2,
ETHERS_UNHANDLED_ERROR = 3,
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 this mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iirc there are some ethers error correctly handled some not, this is in the case it's an unknown error from ethers

DOCUMENT_REVOKED = 1,
CONTRACT_ADDRESS_INVALID = 2,
ETHERS_UNHANDLED_ERROR = 3,
SKIPPED = 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is skipped in the revocation code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why not ? I dont understand ?

},
]);
});
it("should return a invalid fragment when revocation store is used and the document is not revoked", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it("should return a invalid fragment when revocation store is used and the document is not revoked", async () => {
it("should return a invalid fragment when revocation store is used and the document is revoked", async () => {

code: 1,
codeString: "DOCUMENT_REVOKED",
message:
"Certificate 0x856924fa2cf3374bf64697eb0dcf38d0251ff18aedae2bbc193398e8bb11fbd1 has been revoked under contract 0x8Fc57204c35fb9317D91285eF52D6b892EC08cD3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Certificate 0x856924fa2cf3374bf64697eb0dcf38d0251ff18aedae2bbc193398e8bb11fbd1 has been revoked under contract 0x8Fc57204c35fb9317D91285eF52D6b892EC08cD3",
"Document 0x856924fa2cf3374bf64697eb0dcf38d0251ff18aedae2bbc193398e8bb11fbd1 has been revoked under contract 0x8Fc57204c35fb9317D91285eF52D6b892EC08cD3",

Can we change reference of certificate to document

const type: VerificationFragmentType = "DOCUMENT_STATUS";
export const openAttestationRevocationStore: Verifier<WrappedDocument<v2.OpenAttestationDocument>> = {
skip: () => {
return Promise.resolve({
Copy link
Contributor

Choose a reason for hiding this comment

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

wont this resolve be redundant? or can just add async to the function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could, just coding style :p (both are equivalent)

test: (document) => {
if (utils.isWrappedV2Document(document)) {
const documentData = getData(document);
return documentData.issuers.every((issuer) => "revocationStore" in issuer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt this be any? Any failure should fail it?

Copy link
Contributor Author

@Nebulis Nebulis Jun 4, 2020

Choose a reason for hiding this comment

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

hum indeed used .some everywhere but for revoke 🤔

will change both 🙈

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.

None yet

2 participants