-
Notifications
You must be signed in to change notification settings - Fork 3
Refactor AssetProofComposer #333
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
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @jnmt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on refactoring the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request effectively refactors AssetProofComposer to decouple it from ScalarDB, which is a good architectural improvement. The logic for handling ScalarDB-specific Put objects is now correctly encapsulated within ScalarTamperEvidentAssetLedger. Additionally, consolidating duplicated test constants into a new TestConstants class is a great move for maintainability and successfully addresses the SpotBugs warning. The changes are clean and well-implemented. I have one minor suggestion to further improve the new constants class.
ledger/src/test/java/com/scalar/dl/ledger/test/TestConstants.java
Outdated
Show resolved
Hide resolved
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.
Pull request overview
This PR refactors AssetProofComposer to remove its ScalarDB dependency by moving it from the scalardb package to the generic database package, and consolidates duplicate certificate constants across test files to address SpotBugs HSC_HUGE_SHARED_STRING_CONSTANT warnings.
- Removed ScalarDB-specific
create(Put, String)method fromAssetProofComposerand added a helper method inScalarTamperEvidentAssetLedgerto extract data from ScalarDB'sPutobjects - Moved
AssetProofComposerfromcom.scalar.dl.ledger.database.scalardbtocom.scalar.dl.ledger.databasepackage - Created
TestConstantsclass to centralize certificate and private key constants, eliminating duplication across 4 test classes and 1 integration test constants class
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
ledger/src/main/java/com/scalar/dl/ledger/database/AssetProofComposer.java |
Moved from scalardb package to database package; removed ScalarDB-specific create(Put, String) method |
ledger/src/main/java/com/scalar/dl/ledger/database/scalardb/ScalarTamperEvidentAssetLedger.java |
Added createProofFrom helper method to extract data from ScalarDB Put objects before calling AssetProofComposer |
ledger/src/test/java/com/scalar/dl/ledger/test/TestConstants.java |
New class centralizing certificate and private key constants used across test files |
ledger/src/test/java/com/scalar/dl/ledger/validation/ContractValidatorTest.java |
Removed duplicate constants, added imports for TestConstants |
ledger/src/test/java/com/scalar/dl/ledger/service/LedgerValidationServiceTest.java |
Removed duplicate constants, added imports for TestConstants and updated AssetProofComposer import |
ledger/src/test/java/com/scalar/dl/ledger/contract/ContractManagerTest.java |
Removed duplicate constants, added imports for TestConstants |
ledger/src/test/java/com/scalar/dl/ledger/database/scalardb/ScalarTransactionManagerTest.java |
Updated AssetProofComposer import to new package |
ledger/src/test/java/com/scalar/dl/ledger/database/scalardb/ScalarTamperEvidentAssetLedgerTest.java |
Updated AssetProofComposer import to new package |
ledger/src/main/java/com/scalar/dl/ledger/service/ValidationService.java |
Updated AssetProofComposer import to new package |
ledger/src/main/java/com/scalar/dl/ledger/service/LedgerValidationService.java |
Updated AssetProofComposer import to new package |
ledger/src/main/java/com/scalar/dl/ledger/database/scalardb/ScalarTransactionManager.java |
Updated AssetProofComposer import to new package |
ledger/src/integration-test/java/com/scalar/dl/ledger/service/LedgerServiceIntegrationTest.java |
Removed constant imports from integration test Constants, added imports for TestConstants |
ledger/src/integration-test/java/com/scalar/dl/ledger/service/LedgerServiceEndToEndTest.java |
Removed constant imports from integration test Constants, added imports for TestConstants |
ledger/src/integration-test/java/com/scalar/dl/ledger/service/Constants.java |
Removed duplicate certificate and private key constants |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Description
This PR removes the ScalarDB dependency from
AssetProofComposer. I think it's better to refactor it before changing it for the namespace feature.Additionally, this PR consolidates duplicate certificate constants, eliminating duplicate long string constants across multiple test classes to fix the SpotBugs
HSC_HUGE_SHARED_STRING_CONSTANTwarning. This is essentially not related to the refactoring ofAssetProofComposer, but somehow it started to occur.Related issues and/or PRs
N/A
Changes made
create(Put, String)method inAssetProofComposerand add a helper method inScalarTamperEvidentAssetLedgerto extract data fromPut.scalardbpackage to thedatabasepackage.TestConstants.Checklist
Additional notes (optional)
N/A
Release notes
Refactored AssetProofComposer.