multi: clean up unnecessary stuff and use instance logging in favor of global logger

[bhandras]

No description provided.

[gemini-code-assist]

Summary of Changes

Hello @bhandras, I'm Gemini Code Assist¹! 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 performs a substantial refactoring by removing the previous darepoclient application and its related RPC services, client, and daemon. The project's build process is now streamlined to focus on a single merge-sql-schemas binary. Concurrently, the logging system has been modernized from a global approach to an instance-based one, improving the overall architecture and making components more independent and easier to test.

Highlights

• Application Removal: The entire darepoclient application, including its adminrpc and client-facing rpc services, arkcli client, arkd daemon, and associated configuration, has been removed. This indicates a significant refactoring or refocusing of the project.
• Instance-Based Logging: The logging infrastructure has been overhauled to use instance-based btclog.Logger instances instead of a global logger. Loggers are now explicitly passed to constructors and methods of database components, enhancing modularity and testability.
• Build Target Refocus: The Makefile has been updated to reflect that the project now builds a single binary named merge-sql-schemas, moving away from a generic darepo client/server build target.

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 by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

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 pull request 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

1. 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. ↩

[gemini-code-assist]

Code Review

This pull request performs a significant cleanup by removing the arkd daemon, arkcli client, and associated RPC servers, focusing the repository on the merge-sql-schemas tool. Additionally, it refactors the database package to use instance-based logging instead of a global logger. The changes are well-executed and consistent with the stated goals. The logging refactoring improves modularity by removing global state, and the code removals simplify the project's scope. Overall, this is a solid set of improvements.

[claude]

Code Review - PR #7: Clean up unnecessary stuff and use instance logging

Summary

This PR performs a significant cleanup by removing unused server infrastructure (admin RPC server, client CLI, main daemon) and transitioning from global to instance-based logging in the database layer. Overall, this is a well-executed refactoring that improves code maintainability.

---

✅ Positive Aspects

1. Good refactoring direction: Removing unused code (1128 deletions vs 75 additions) and moving to instance-based logging are both solid improvements.

2. Consistent logger propagation: The changes properly thread the btclog.Logger through all database constructors and the TransactionExecutor.

3. Test compatibility: Tests correctly use btclog.Disabled for test fixtures, which is appropriate.

4. Backward compatibility: The changes maintain the same interfaces while adding logger parameters.

---

🔍 Issues & Concerns

1. Critical: Missing Logger Nil Check (db/interfaces.go:239)

t.log.Tracef("Retrying transaction due to tx serialization or "+
    "deadlock error, attempt_number=%v, delay=%v",
    attemptNumber, retryDelay)

Issue: If t.log is nil, this will panic at runtime. While the constructors currently pass loggers, there's no guarantee.

Fix: Add a nil check or enforce non-nil loggers in the constructor with proper documentation.

---

2. Structured Logging Compliance (Per CLAUDE.md)

According to the project guidelines, all logging should use structured log methods ending in S:

Current (db/postgres.go:94):

log.Infof("Using SQL database '%s'", cfg.DSN(true))

Should be:

log.InfoS(ctx, "Using SQL database",
    slog.String("dsn", cfg.DSN(true)))

Affected locations:

• db/postgres.go:94
• db/sqlite.go:181-182, 210-213, 221-222, 230-231, 234
• db/interfaces.go:242-244 (Tracef is fine for trace level)

Note: Per the guidelines, structured logging is required for new code. Since this is refactoring existing logging calls, you may want to address this in a follow-up PR.

---

3. Makefile Changes: Build Target Narrowing

The Makefile changes restrict building to only cmd/merge-sql-schemas:

-$(GOBUILD) ... -o . ./cmd/...
+$(GOBUILD) ... -o . ./cmd/merge-sql-schemas

Question: Is this intentional? This makes sense if arkd and arkcli are being removed permanently, but please confirm:

• Are these commands truly obsolete?
• Is this repo now a library-only package?
• Should the Makefile comments be updated to reflect this?

---

4. Test Coverage Verification Needed

While the migration tests were updated to pass loggers, I couldn't verify:

1. Do all existing tests still pass?
2. Is the linter happy with the changes?

Recommendation: Run before merging:

make lint
make unit pkg=db timeout=5m

---

🎯 Minor Suggestions

1. db/interfaces.go:207: Consider adding a comment explaining why the logger is needed:

   // log is used for trace-level retry diagnostics during transaction execution
   log btclog.Logger

2. Function comments: The updated function signatures should have their comments updated to document the new log parameter:

   // NewPostgresStore creates a new store that is backed by a Postgres database
   // backend. The provided logger is used for all database-related logging.
   func NewPostgresStore(cfg *PostgresConfig, log btclog.Logger) (*PostgresStore, error)

3. Consistency: In test helpers, consistently document why btclog.Disabled is used:

   // For tests, use a disabled logger to reduce noise
   log := btclog.Disabled

---

🔒 Security & Performance

• No security concerns identified
• Performance: Minimal impact - just passing an additional parameter
• Database changes: None - purely code organization

---

✅ Action Items Before Merge

1. Verify the code compiles and all tests pass
2. Run make lint and ensure it passes
3. Run make unit pkg=db timeout=5m and verify all tests pass
4. Add nil check for t.log in TransactionExecutor.ExecTx or document that logger must not be nil
5. Consider updating to structured logging (or file as follow-up issue)
6. Update function comments to document the new log parameters

---

📊 Overall Assessment

Quality: Good refactoring with a clear purpose
Risk: Low - mostly deletions and straightforward parameter additions
Recommendation: Approve with minor fixes

The direction of this PR is excellent - removing dead code and improving logging architecture. The main concerns are ensuring proper nil checks and potentially updating to structured logging per project guidelines. Once those are addressed, this should be good to merge.

Nice cleanup! 🎉

[claude]

Code Review for PR #7

Summary

This PR performs a significant cleanup by removing unused server scaffolding (RPC servers, CLI tools, config) and migrating the db package from a global logger to instance-based logging with structured logging. Overall, this is a positive change that simplifies the codebase and improves logging practices.

Net Impact: +124 additions, -1162 deletions across 24 files

---

Strengths

1. Proper Structured Logging Migration

The migration to structured logging follows the project's guidelines correctly:

• Uses structured log methods ending in S (e.g., InfoS, TraceS)
• First parameter is context.Context
• Second parameter is a static string message
• Key-value pairs use appropriate helpers (slog.Int(), etc.)
• Examples in db/interfaces.go:239-247 and db/sqlite.go:182-185 are excellent

2. Clean Removal of Unused Code

Removing the following unused components makes sense:

• adminrpc/ - Admin RPC service and generated code
• cmd/arkd/main.go and cmd/arkcli/main.go - Server and CLI binaries
• server.go, rpcserver.go, adminrpcserver.go - Server implementations
• config.go and logging.go - Configuration and global logging setup

3. Consistent Logger Injection

All database constructors now properly accept a btclog.Logger parameter

4. Test Fixture Updates

Test helpers correctly use btclog.Disabled for test logging

---

Issues and Concerns

1. Breaking Change to Public API

The signature changes to database constructors are breaking changes. Since this appears to be an internal/client repository, this is likely acceptable.

2. Missing Context in Some Log Calls

Several log calls use context.Background() instead of a proper context (db/migrations.go:190-192, db/sqlite.go:182-185, 214-219, 229-231, 238-242, 245). Consider if a context should be passed through the call chain for proper context propagation.

3. Potential Nil Logger Issue

The constructor in db/interfaces.go:223 does not validate the logger parameter. Consider adding a nil check or documenting that logger must not be nil.

4. Makefile Changes

The build targets now only build merge-sql-schemas. Ensure this is intentional.

---

Testing Recommendations

1. Verify All Callers Updated

Ensure all code that calls the modified database constructors has been updated with the new logger parameter.

2. Run Full Test Suite

Before merging verify: make lint and make unit pkg=./db timeout=5m
Ensure no ERR log lines appear in test output per CLAUDE.md guidelines.

---

Style and Convention Compliance

• Structured logging format correctly uses S methods with static messages
• Logger parameter properly positioned as last parameter (good Go convention)
• Test logging properly uses btclog.Disabled
• Error handling maintains consistent error wrapping with fmt.Errorf
• Follows 8-space tab convention and 80-character guideline

---

Security and Performance

Security: No Concerns

• No security-sensitive code paths affected
• Logger injection improves audit trails
• No credentials or sensitive data in log messages

Performance: No Concerns

• Structured logging has minimal performance impact
• Removing unused code reduces binary size
• Database connection handling unchanged

---

Final Recommendation: APPROVE with Minor Changes

This is a solid cleanup PR that improves code quality and logging practices. The concerns are minor and mostly involve:

1. Adding nil checks for logger parameters
2. Considering context propagation for migrations
3. Verifying all call sites are updated

Before Merge Checklist:

• Run make lint (must pass)
• Run make unit pkg=./db timeout=5m
• Verify no ERR lines in test output
• Add nil check for logger in NewTransactionExecutor
• Consider adding context parameter to migration functions (optional but recommended)

Great work on the cleanup! The codebase is definitely cleaner and more maintainable after this change.

[Roasbeef]

LGTM 🪳