multiaddr[1/N]: lift single address creation restrictions #1002
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 @hieblmi, 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 implements a foundational refactoring of the static address management system. It introduces a new database column and a migration to correctly associate historical deposit records with their static addresses. Concurrently, the process for generating and managing new static addresses has been overhauled to be more robust and scalable, preparing the system for advanced features like supporting multiple static addresses per client in the future.
Highlights
- Database Schema Update and Migration: Introduced a new static_address_id column in the deposits table, linking deposits to their respective static addresses. A one-time migration is added to backfill this field for existing deposits, assuming a single static address for historical data.
- Static Address Management Refactor: The address.Manager has been refactored to use an asynchronous worker model for creating new static addresses, improving responsiveness and setting the stage for future multi-address support. Address generation logic has been moved to the address.Parameters struct.
- API and Internal Logic Adjustments: Updated RPC calls (NewStaticAddress, ListUnspentDeposits, GetStaticAddressSummary) to directly handle string representations of static addresses. The StaticAddressSummaryResponse protobuf message has been updated to include a per_address_summaries field, deprecating older single-address fields.
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. ↩
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant step towards supporting multiple static addresses. It refactors the address manager to handle multiple addresses, introduces a database migration to link deposits to static addresses, and updates the RPC API to return summaries for multiple addresses.
My review has identified a critical bug in the new address creation logic that could lead to a panic, and an incomplete implementation of the updated GetStaticAddressSummary RPC endpoint. The rest of the changes look solid and well-aligned with the goal of supporting multiple static addresses.
hieblmi
force-pushed
the
multi-1
branch
4 times, most recently
from
6b33c18 to
35f4809
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant changes to support multiple static addresses, moving away from the previous single-address limitation. The refactoring in staticaddr/address/manager.go to handle address creation asynchronously is a key part of this. The changes also include database migrations to associate deposits with their respective static addresses. While the overall direction is good, I've identified a critical issue in the database migration that could impact existing users, along with a couple of high-severity issues where the multi-address support is either incomplete or contains a bug. Addressing these points will be crucial for a stable release.
hieblmi
force-pushed
the
multi-1
branch
2 times, most recently
from
6d6455d to
599c09f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant architectural change to lift the restriction of having only a single static address. The changes are extensive, including database migrations, refactoring the address manager to be asynchronous, and updating various components to handle multiple addresses. Overall, the implementation is solid, but I've identified a critical issue where some parts of the code still operate under the assumption of a single static address, which can lead to incorrect information being presented to the user. Addressing this will be crucial for the correctness of the multi-address feature.
hieblmi
force-pushed
the
multi-1
branch
11 times, most recently
from
7ef6018 to
a285715
Compare
The QuoteRequest adds a field auto_select_deposits to signal that the specified quote amount should be coin-selected from the client's deposits to determine the number of deposits to quote for. The StaticAddressLoopInRequest gets a new amount field that either indicates that the swap amount should be allocated from the passed deposits or coin-selected from all available deposits.
In this commit we add a new function SelectDeposits to the loop-in manager. It coin-selects deposits that meet an arbitrary swap amount provided by the client. We have to ensure that the server creates the correct change outputs for the htlc- and sweepless sweep transactions.
If a quote request contains an amount and flag SelectDeposits set to true the quoting coin- selects the required deposits to meet the swap amount in order to quote for the number of deposits.
The selected_amount column of all previous swaps is filled with the total value of deposits that partook in these swaps.
� Conflicts: � staticaddr/withdraw/interface.go
No description provided.