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

bug: types for nested objects are not generated correctly #1

Open
martines3000 opened this issue Mar 20, 2024 · 4 comments
Open

bug: types for nested objects are not generated correctly #1

martines3000 opened this issue Mar 20, 2024 · 4 comments

Comments

@martines3000
Copy link

The library is used inside Veramo.

My code:

const verifiableCredential = await agent.createVerifiableCredential({
  credential: {
    issuer: { id: identifier.did },
    '@context': ['https://www.w3.org/2018/credentials/v1', 'https://example.com/1/2/3'],
    type: ['VerifiableCredential', 'Custom'],
    issuanceDate: new Date().toISOString(),
    credentialSubject: {
      id: 'did:web:example.com',
      you: 'Rock',
      power: {
        "over": "9000"
      }
    },
  },
  proofFormat: 'EthereumEip712Signature2021',
});

Error:
image

I already did a basic rewrite of the getEthTypesFromInputDocHelper function which works with nested objects and with arrays.
The last part is also missing in the specifications.

  1. Do you think there will be some extra work done there ?
  2. Would it make sense to rewrite the library or restructure (newer versions of packages, improved test, etc.. ?)
@mirceanis
Copy link

Thanks for posting!

This repository definitely needs some more attention.
Any PR here is appreciated.

@martines3000
Copy link
Author

martines3000 commented Mar 20, 2024

Hi. I created a PR #2 that refactors the project with improved tooling, updated libs, adds github workflows.

It also removes the requirement to have a mandatory proof field as I think this is not necessary. If I missed something, feel free to add feedback.

I am happy to contribute more. I am planning to add the logic of the function refactor I did and add a lot of tests.

For the tests I am also planning to add either MetaMasks or ethers library to dev-dependencies to test actual signing and verification with the generated types and if things really work as expected.

@martines3000
Copy link
Author

martines3000 commented Mar 20, 2024

The library is quite crucial for creating EIP712 signed verifiable credentials, so I think it would also be nice if it could be moved to a more important place like Veramo or VeramoLabs organization on GitHub.

It would also make sense to add workflows for publishing on NPM and something for generating changelogs (like changesets).

I am happy to provide both the publishing workflows and setup changelog generation with changesets. Just let me know if this is something that you think would improve the repository and think it is necessary.

@martines3000
Copy link
Author

Current work in progress of the function rewrite can be found here.

For now I added an optional parameter useHashing test if implementation passes the old tests. If it is not set, it uses the old naming convention for types (type names come from key names, with first character uppercased). For the final version I would use hashes as type names to avoid the problem of collisions.

Feedback is welcome 😄

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

No branches or pull requests

2 participants