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
Credentials #759
Credentials #759
Changes from 14 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
084bc04
5828d8e
c27ab41
1195417
1a5fc8b
7d1781a
ae55a83
bc71c4c
c1adc90
cf6e618
a6d4f1e
cbfb27b
ed6f6ef
36e2fd6
a039075
eb4e176
7bf9a37
657af63
5669d4b
63336b3
54d894d
d754702
397d104
1093164
46861f4
2593446
98e0f79
70f9301
14893ee
24dcb32
383a325
42e8e71
e168dcb
387f72b
8e209a1
8a93ed7
a059003
d823911
aeabe63
26e1b13
1e09926
cd7f860
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.
The account prefix is unnecessary here
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.
Outside the context of credentials, it is not immediately obvious if
Subject
refers to an "account". I prefer to retain the Account prefix.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.
what do you think of
_ISSUER_ACCT
and_SUBJECT_ACCT
? Is that more informative?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.
I agree with @mvadari , it's sufficient to just have
_ISSUER
and_SUBJECT
since the context is obvious being inside a Credential test.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.
54d894d
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: