Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Verifiable Credentials #759
base: main
Are you sure you want to change the base?
Verifiable Credentials #759
Changes from 12 commits
c7e1a15
9d549c1
8375c42
8d810fc
0a33121
fd8157d
1d85bbb
8ea12c4
e4293be
60b7663
509ecd1
c0151c2
48b8abc
f476a51
8b82c3d
a24ca5b
4402f6d
22de7a3
f848bbd
cb63c6d
ffcf1bd
e498721
e5a0779
ad4331a
1a974fb
880a670
af67d78
c50d611
2c887aa
8468fcd
bba054d
6c0b958
7f68440
1485ae9
27f8719
485d392
562aa49
d83f0d7
a188c1a
b97d6f3
379620a
e63b49d
5425b37
3e8b825
4106236
6e42798
0719107
f1d2b4e
bf5114a
6d8f1be
8e496e1
5656080
b43aa12
6392351
4ad028b
1f25b01
a870f2d
a6ac0f4
111e12a
8593111
7fa47e1
3d66a36
2d86ea2
f58dc2c
0f7f745
b02f902
b70ac84
7f79596
3814a94
c721a87
c76b755
9b7d17c
5068266
c7a6e2e
344bb49
ee01fc9
09b7d18
87d2af7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance type safety and error handling.
The utility function could benefit from type hints and better error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement the suggested verification tests.
The comment indicates missing verification tests. These should be implemented to ensure the credential object exists in both issuer's and subject's directory pages immediately after creation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add cleanup for test resilience.
The test should clean up any remaining credential objects in case of test failures. Consider using
setUp
andtearDown
methods.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the setup and teardown is performed by the CredentialCreate and CredentialDelete transactions. I don't want to abstract these transactions in separate util functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ckeshava, understood. I'll keep in mind that the setup and teardown are performed using the
CredentialCreate
andCredentialDelete
transactions, and that you prefer not to abstract these transactions into separate utility functions.(_/)
( •_•)
✏️ Learnings added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is unnecessary. We're testing the library, not rippled. Checking just one is more than enough. You could even just check for whether the object exists with
ledger_entry
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The choice between
ledger_entry
andaccount_objects
is inconsequential, because both of them refer to the same ledger object.There is no harm in additional check for the existence/deletion of ledger objects. It serves to explain the behavior of credentials better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add validation for credential ID format.
The test verifies basic functionality but should also validate the credential ID format:
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding boundary test cases.
The current tests cover important invalid scenarios, but consider adding these additional test cases: