Refactor Payment PR 5 #10153
Refactor Payment PR 5 #10153
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ziggie1984, 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 undertakes a significant refactoring effort by introducing a new PaymentDB interface to abstract the underlying payment database operations. This change decouples various system components, such as the RPC server and the control tower, from the concrete Key-Value payment database implementation. The refactoring enhances modularity, improves testability by allowing different database implementations to be swapped in, and lays the groundwork for future architectural improvements.
Highlights
- Introduction of Payment Database Interface: A new
paymentsdb.PaymentDBinterface, along withPaymentReader,PaymentWriter, andPaymentControlsub-interfaces, has been introduced to define standard payment database operations. This provides a clear contract for how payment data is accessed and modified. - Decoupling of Core System Components: Key components of the system, including
config_builder,routing/control_tower,rpcserver, and the mainserver, have been updated to interact with the payment database solely through the newPaymentDBinterface. This significantly reduces coupling and improves architectural flexibility. - Comprehensive Test Reorganization and Refinement: Payment-related tests have been thoroughly reorganized. Generic tests that operate on the
PaymentDBinterface have been moved topayments/db/payment_test.go, promoting interface-driven testing. Tests specific to the Key-Value implementation remain inpayments/db/kv_store_test.go, and test database creation helpers were adjusted to support this separation.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a PaymentDB interface to abstract the payment database implementation, which is a solid refactoring step. The changes are consistently applied across the codebase, and the test files have been reorganized logically. My feedback is focused on ensuring documentation comments adhere to the project's style guide.
ziggie1984
force-pushed
the
refactor-payments-code-05
branch
from
3e885a4 to
ee81e16
Compare
ziggie1984
force-pushed
the
refactor-payments-code-05
branch
from
ee81e16 to
59a2c71
Compare
ziggie1984
force-pushed
the
refactor-payments-code-05
branch
2 times, most recently
from
02f655b to
ed327b9
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the payment database logic. By introducing a paymentsdb.DB interface, it successfully decouples the payment system from the underlying kvdb implementation, paving the way for alternative backends like native SQL. Moving the tests from the kv_store to a backend-agnostic location is a great step towards ensuring correctness across different database implementations.
My review includes a couple of suggestions to improve test code clarity and documentation, but overall the changes are solid and align well with the stated goals.
| // NewKVTestDB is a helper function that creates an BBolt database for testing | ||
| // and there is no need to convert the interface to the KVStore because for | ||
| // some unit tests we still need access to the kvdb interface. |
There was a problem hiding this comment.
The comment for NewKVTestDB has a minor typo ("an BBolt" should be "a bbolt") and could be rephrased for better clarity.
| // NewKVTestDB is a helper function that creates an BBolt database for testing | |
| // and there is no need to convert the interface to the KVStore because for | |
| // some unit tests we still need access to the kvdb interface. | |
| // NewKVTestDB is a helper function that creates a bbolt database for testing. | |
| // It returns the concrete `KVStore` type, which is useful for tests that need | |
| // access to the underlying `kvdb` interface. |
I don't think we can, because the paymentsdb:
whereas the ControlTower:
plus the interface gives us good mocking/testing possibilities, wdyt ? |
There was a problem hiding this comment.
plus the interface gives us good mocking/testing possibilities, wdyt ?
Given that the control tower holds the paymentsdb.DB, it already provides mocking as when testing, we can just create a mock db and attach it to the control tower. I think to make the layer clearer,
- at the bottom we have the db layer
- on top of it we have the control tower layer, which provides notifications
I think the control tower interface can embed the payment control interface, but yeah the changes may be large so some future refactoring PR would do.
Only got one final question re the payment controller interface, otherwise this is good to go.
| // its database operations. This interface represents the control flow of how | ||
| // a payment should be handled in the database. They are not just writing | ||
| // operations but they inherently represent the flow of a payment. The methods | ||
| // are called in the following order. |
There was a problem hiding this comment.
nice - based on the reasoning here, DeleteFailedAttempts should also be part of PaymentControl?
There was a problem hiding this comment.
yes moved the function as well to the PaymentControl interface
This commit starts reusing test cases which are not dependant on the kv db backend. So they can be later used with the native db implementation as well.
we make the index assertion db independant so it is a noop for a future native sql backend. This allows us to reuse even more tests for the different db architectures.
This matches the same naming as used in the graph package.
ziggie1984
force-pushed
the
refactor-payments-code-05
branch
from
d115a29 to
bf6131b
Compare
Depends on #9847
This introduces the paymentDB interfaces and moves all tests from the kv store implementation to the db agnostic place where they can run against both backends (kv and native sql)