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 all 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.
💡 Codebase verification
Use a stable rippled version (2.3.0) instead of a release candidate
The workflow is currently using a release candidate version (2.3.0-rc1). It's recommended to use the stable 2.3.0 release to ensure stability in CI.
🔗 Analysis chain
Verify the rippled version requirement
The workflow now uses a release candidate version (2.3.0-rc1). While this might be necessary for the new Verifiable Credentials feature, using release candidates in CI can potentially introduce instability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 600
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.
Why all the changes 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.
develop
branchentrypoint.sh
bash file in the docker image. In consultation with the @micheal legleux, I've updated thedocker run
command accordingly.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.
lastly, I've updated the mount point of the docker container. We don't need to explicitly specify the conf file anymore, its already placed into the default location that is searched by rippled
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 the changelog entry with more details about XLS-70d implementation.
The current entry could be more detailed given the significance of this feature. Consider expanding it to include:
📝 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.
line 12 needs to begin with a
-
character to represent a bullet point in the markdown fileThere 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, thank you for catching that. You're absolutely right—the line should start with a
-
to represent a bullet point in the markdown file. Here's the updated changelog entry:Let me know if there's anything else I can assist with.
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.
@rabbit can you present a "Commitable Suggestion" with this updated change?
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.
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 Docker command robustness and security
The current Docker command could benefit from additional security and reliability improvements:
--rm
flagConsider this improved version:
📝 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.
This no longer matches above.
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.
yes, this is an intentional change. please check my comment on the integration_test.yml CI/CD file
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
Strengthen credential object parameter testing
The test should be more comprehensive to ensure robust credential validation:
Suggested implementation:
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.
💡 Codebase verification
Additional credential test scenarios needed
The suggested credential scenarios are not fully covered in existing tests. Consider adding the following tests in
tests/unit/models/requests/test_ledger_entry.py
:🔗 Analysis chain
Consider additional test scenarios
While the basic credential functionality is tested, consider adding these scenarios:
Let's check if these scenarios are covered elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 33
Script:
Length of output: 12591
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