perf(pollux): Improve CredentialService's API ATL-3225

[FabioPinheiro]

Overview

perf(pollux): Improve CredentialService's API ATL-3225

Checklist

My PR contains...

• No code changes (changes to documentation, CI, metadata, etc.)
• Bug fixes (non-breaking change which fixes an issue)
• Improvements (misc. changes to existing features)
• Features (non-breaking change which adds functionality)

My changes...

• are breaking changes
• are not breaking changes
• If yes to above: I have updated the documentation accordingly

Documentation

• My changes do not require a change to the project documentation
• My changes require a change to the project documentation
• If yes to above: I have updated the documentation accordingly

Tests

• My changes can not or do not need to be tested
• My changes can and should be tested by unit and/or integration tests
• If yes to above: I have added tests to cover my changes
• If yes to above: I have taken care to cover edge cases in my tests

[atala-dev]

🦙 MegaLinter status: ✅ SUCCESS

Descriptor	Linter	Files	Fixed	Errors
✅ REPOSITORY	dustilock	yes		no
✅ REPOSITORY	git_diff	yes		no
✅ REPOSITORY	syft	yes		no

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by

[github-actions]

Pollux Lib Test Results

  7 files    7 suites   1m 7s ⏱️
84 tests 84 ✔️ 0 💤 0 ❌
85 runs  85 ✔️ 0 💤 0 ❌

Results for commit a113401.

♻️ This comment has been updated with latest results.

[ghost]

Changes look good - I understand you are removing the use of Option and switching to an error class which is a factory pattern - is this what's increasing performance? In previous projects, I saw the use of Option logically correct as it's not an error if a record doesn't exist [or rather, it's possible that this is a case and it may exist in the future or not at all], so we modelled it as a positive path and didn't use an error but mapped an Option of None to a 404 in the HTTP layer - does something similar happen here?

[bvoiturier]

It should be considered an error IMO. This one is about telling your prism agent to accept a credential offer you are supposed to have received previously, while that offer doesn't exist in your system. Option is a good fit for methods returning a record based on a parameter which is not the primary key => typically findXXX() methods as opposed to getXXX() methods. Using Option as a return type forces you to handle it on the caller side while you can't really do something with it.

[yshyn-iohk]

I like these changes. The business logic is more concise without Option[]

[FabioPinheiro]

@davidpoltorak-io But the E (error) is also a value. In reality we are even returning more information than before.

Interns of semantics you can not accept a offer acceptCredentialOffer if the offer does not exist.

The method getIssueCredentialRecord still returns you an Option because this is a possibility for the method. But yes I agree with you that need scala docs on the API interfaces.

[patlo-iog]

The changes look good. Suspect there are some conflicts with the recently merged pollux PR.

[bvoiturier]

👍