feat: add tenant remove command#597
Conversation
WalkthroughAdds tenant model and DB operations, a RemoveTenant RPC and client method, RPC-server removal logic with checks and permission, CLI tenant remove command and CLI wallet metadata fixes, documentation for RemoveTenant, and tests for DB and RPC behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as boltzcli
participant Client as boltz client
participant RPC as RPC Server
participant DB as Database
rect rgb(220,240,250)
note over CLI,RPC: RemoveTenant flow
CLI->>Client: RemoveTenant(name)
Client->>RPC: RemoveTenantRequest{name}
RPC->>DB: GetTenantByName(name)
alt tenant not found
RPC-->>Client: NotFound
else found
RPC->>RPC: if tenant == default -> InvalidArgument
alt default tenant
RPC-->>Client: InvalidArgument
else
RPC->>DB: HasTenantWallets(tenantId)
alt has wallets
RPC-->>Client: FailedPrecondition
else no wallets
RPC->>DB: DeleteTenant(tenantId) (tx)
RPC-->>Client: Empty (success)
end
end
end
end
sequenceDiagram
participant CLI as boltzcli
participant Client as boltz client
participant RPC as RPC Server
participant DB as Database
rect rgb(240,250,220)
note over CLI,RPC: Create/Import Wallet with tenant
CLI->>Client: CreateWallet(params, tenantName?)
alt tenantName provided
Client->>RPC: Resolve tenant -> GetTenantByName(tenantName)
RPC->>DB: GetTenantByName
DB-->>RPC: Tenant{id}
RPC-->>Client: Tenant{id}
Client->>RPC: CreateWallet(params with tenant_id)
else none
Client->>RPC: CreateWallet(params)
end
RPC->>DB: CreateWallet(record)
DB-->>RPC: Wallet created
RPC-->>Client: Success
Client-->>CLI: Success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (9)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-07-16T09:46:41.613ZApplied to files:
🧬 Code graph analysis (3)pkg/boltzrpc/client/client.go (1)
internal/rpcserver/router.go (3)
internal/database/tenant.go (3)
🔇 Additional comments (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/rpcserver/router.go (1)
1468-1483: WalletParams.tenant_id is ignored in ImportWallet/CreateWallet
WalletParamsnow has atenant_idfield, and the tests (e.g.TestMacaroons/AdminCreateForTenant) expect an admin to be able to create a wallet for a specific tenant by setting it. However,ImportWalletstill unconditionally usesrequireTenantId(ctx):currency := serializers.ParseCurrency(&request.Params.Currency) credentials := &onchain.WalletCredentials{ WalletInfo: onchain.WalletInfo{ Name: request.Params.Name, Currency: currency, TenantId: requireTenantId(ctx), }, ... }So:
- For admin macaroons without a tenant bound, wallets are always created under
DefaultTenantId, regardless ofparams.tenant_id.- The new CLI
wallet create --tenantand the new tests’ expectations about cross-tenant wallet creation won’t actually be met.- There is currently no way for an admin to seed wallets directly into a different tenant without switching the whole context via
SetTenant.Suggested fix (outline):
- Derive a
tenantIdvariable that can be overridden when the caller is admin and atenant_idis provided, and validate the tenant exists:func (server *routedBoltzServer) ImportWallet(ctx context.Context, request *boltzrpc.ImportWalletRequest) (*boltzrpc.Wallet, error) { if request.Params == nil { return nil, errors.New("missing wallet parameters") } if err := checkName(request.Params.Name); err != nil { return nil, err } - currency := serializers.ParseCurrency(&request.Params.Currency) + currency := serializers.ParseCurrency(&request.Params.Currency) + tenantId := requireTenantId(ctx) + if request.Params.TenantId != nil { + // Only admins should be allowed to override the target tenant. + if !isAdmin(ctx) { + return nil, status.Errorf(codes.PermissionDenied, "only admin can specify tenant_id") + } + // Ensure the tenant exists before creating a wallet for it. + id := database.Id(request.Params.GetTenantId()) + if _, err := server.database.GetTenant(id); err != nil { + if errors.Is(err, sql.ErrNoRows) { + return nil, status.Errorf(codes.NotFound, "tenant %d does not exist", id) + } + return nil, err + } + tenantId = id + } credentials := &onchain.WalletCredentials{ WalletInfo: onchain.WalletInfo{ Name: request.Params.Name, Currency: currency, - TenantId: requireTenantId(ctx), + TenantId: tenantId, }, ... }This would:
- Make
tenant_idonWalletParamsactually effective for admin RPC/CLI callers.- Preserve existing behavior for tenant-scoped macaroons (they can’t escape their tenant even if they try to set
tenant_id).- Align runtime behavior with the new proto docs and
TestMacaroons/AdminCreateForTenant.
🧹 Nitpick comments (4)
internal/database/tenant.go (1)
89-99: HasTenantWallets is correct; consider EXISTS for efficiencyThe implementation is logically sound and matches the intended boolean “has wallets” semantics. If the
walletstable grows large and this check is on a hot path, you could switch to an existence check instead ofCOUNT(*), e.g.SELECT 1 FROM wallets WHERE tenantId = ? LIMIT 1, and treatsql.ErrNoRowsas “no wallets” to avoid scanning more rows than necessary.internal/rpcserver/router.go (1)
2345-2371: RemoveTenant handler enforces invariants; consider DB-level protection for racesThe handler correctly:
- Returns
NotFoundwhen the tenant name doesn’t exist.- Rejects deletion of the default tenant.
- Checks
HasTenantWalletsand fails withFailedPreconditionwhen wallets are associated.- Deletes the tenant otherwise and returns an empty response.
If you need a hard guarantee that a tenant with wallets can never be deleted even under concurrent load, consider enforcing this at the database level (e.g. a
FOREIGN KEY (tenantId) REFERENCES tenants(id) ON DELETE RESTRICT) or wrapping the wallet check and delete into a single transaction that re-verifies the precondition before delete. As written, there’s a small window betweenHasTenantWalletsandDeleteTenantwhere a new wallet could be created.internal/database/tenant_test.go (1)
43-47: Clarify or adjust the hardcoded tenant count expectation.The test asserts that at least 3 tenants exist, but by this point the test has created multiple tenants ("test-tenant", "named-tenant") plus the default tenant. The expected count should reflect the actual tenants created or use a more specific assertion.
Consider either:
- Asserting the exact count:
require.Equal(t, len(tenants), 3)(or the actual count at this point)- Adding a comment explaining why
>= 3is used- Counting only the tenants created in this test function to make the test more robust
t.Run("QueryTenants", func(t *testing.T) { tenants, err := db.QueryTenants() require.NoError(t, err) - require.GreaterOrEqual(t, len(tenants), 3) + // At this point we have: default tenant + test-tenant + named-tenant + require.GreaterOrEqual(t, len(tenants), 3, "should have at least default tenant plus created tenants") })cmd/boltzcli/commands.go (1)
1571-1578: Reduce code duplication with a helper function.The tenant lookup logic is duplicated between wallet create and wallet import commands. Consider extracting this into a reusable helper function.
Add a helper function before the wallet create command:
func getTenantIdFromName(ctx *cli.Context, tenantName string) (*uint64, error) { if tenantName == "" { return nil, nil } client := getClient(ctx) tenant, err := client.GetTenant(tenantName) if err != nil { return nil, fmt.Errorf("tenant '%s' does not exist", tenantName) } return &tenant.Id, nil }Then update both wallet create and import to use it:
if tenantName := ctx.String("tenant"); tenantName != "" { - client := getClient(ctx) - tenant, err := client.GetTenant(tenantName) + tenantId, err := getTenantIdFromName(ctx, tenantName) if err != nil { - return fmt.Errorf("tenant '%s' does not exist", tenantName) + return err } - info.TenantId = &tenant.Id + info.TenantId = tenantId }Also applies to: 1601-1608
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
pkg/boltzrpc/autoswaprpc/autoswaprpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/autoswaprpc/autoswaprpc_grpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/boltzrpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/boltzrpc_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
cmd/boltzcli/commands.go(3 hunks)docs/grpc.md(3 hunks)internal/database/tenant.go(1 hunks)internal/database/tenant_test.go(1 hunks)internal/macaroons/permissions.go(1 hunks)internal/rpcserver/router.go(2 hunks)internal/rpcserver/rpcserver_test.go(1 hunks)pkg/boltzrpc/boltzrpc.proto(3 hunks)pkg/boltzrpc/client/client.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-16T09:46:41.613Z
Learnt from: jackstar12
Repo: BoltzExchange/boltz-client PR: 487
File: internal/rpcserver/router.go:2304-2314
Timestamp: 2025-07-16T09:46:41.613Z
Learning: In the boltz-client codebase, RPC methods are protected by the macaroon permission system defined in internal/macaroons/permissions.go. Methods like GetSwapMnemonic and SetSwapMnemonic are already restricted to admin entities through the RPCServerPermissions map, so explicit isAdmin(ctx) checks are not needed in these methods.
Applied to files:
internal/macaroons/permissions.gointernal/rpcserver/rpcserver_test.go
🧬 Code graph analysis (5)
internal/database/tenant.go (1)
internal/database/database.go (1)
Database(240-247)
internal/rpcserver/router.go (4)
pkg/boltzrpc/boltzrpc.pb.go (3)
RemoveTenantRequest(510-516)RemoveTenantRequest(531-531)RemoveTenantRequest(546-548)internal/onchain/onchain.go (1)
Id(21-21)internal/database/database.go (1)
Id(336-336)internal/database/tenant.go (1)
DefaultTenantId(12-12)
internal/rpcserver/rpcserver_test.go (3)
pkg/boltzrpc/boltzrpc.pb.go (16)
WalletParams(4492-4503)WalletParams(4518-4518)WalletParams(4533-4535)Currency(132-132)Currency(161-163)Currency(165-167)Currency(174-176)Wallet(5899-5910)Wallet(5925-5925)Wallet(5940-5942)BakeMacaroonRequest(659-666)BakeMacaroonRequest(681-681)BakeMacaroonRequest(696-698)Wallets(5986-5992)Wallets(6007-6007)Wallets(6022-6024)pkg/boltzrpc/client/client.go (2)
FullPermissions(20-23)NewBoltzClient(29-34)pkg/boltzrpc/client/connection.go (1)
Connection(17-29)
pkg/boltzrpc/client/client.go (1)
pkg/boltzrpc/boltzrpc.pb.go (3)
RemoveTenantRequest(510-516)RemoveTenantRequest(531-531)RemoveTenantRequest(546-548)
internal/database/tenant_test.go (3)
internal/database/database.go (1)
Database(240-247)internal/database/tenant.go (3)
Tenant(7-10)DefaultTenantId(12-12)DefaultTenantName(13-13)pkg/boltz/currency.go (1)
CurrencyBtc(11-11)
🪛 markdownlint-cli2 (0.18.1)
docs/grpc.md
341-341: Link fragments should be valid
(MD051, link-fragments)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI (ubuntu-latest, 1.24.x)
🔇 Additional comments (9)
internal/macaroons/permissions.go (1)
188-205: LGTM: RemoveTenant correctly restricted to admin writeThe new
/boltzrpc.Boltz/RemoveTenantentry is consistent with the existing tenant admin RPCs (CreateTenant,ListTenants,GetTenant) and ensures only admin macaroons can remove tenants, so no extraisAdminchecks are needed in the handler. Based on learnings.pkg/boltzrpc/client/client.go (1)
194-197: LGTM: Client helper cleanly wraps RemoveTenant RPCThe helper mirrors the existing style (e.g.
Stop,RemoveWallet), passes the name through, and returns only the RPC error, which is exactly what the CLI needs.pkg/boltzrpc/boltzrpc.proto (1)
193-201: Proto surface for tenant removal and wallet tenant_id looks goodThe new pieces here line up well:
rpc RemoveTenant(RemoveTenantRequest) returns (google.protobuf.Empty);plusmessage RemoveTenantRequest { string name = 1; }are consistent with the router’sRemoveTenanthandler and the permission model (admin-only, and guarded by wallet association checks).- The
WalletParams.tenant_idfield and its comment (“Defaults to the current tenant. Only relevant for admin users.”) clearly describe the intended behavior and match how the tests use it (admin creating wallets for another tenant).From the proto/API perspective this all looks coherent; the remaining work is purely on the server side to honor
tenant_idinImportWallet/CreateWalletas noted in the router.go comment.Also applies to: 234-237, 784-786
internal/rpcserver/rpcserver_test.go (2)
527-561: Strong coverage for admin-created tenant walletsThis subtest nicely captures the intended semantics of
WalletParams.tenant_id:
- Admin creates a dedicated tenant.
- Admin creates a wallet with
TenantIdset to that tenant.- A macaroon scoped to the new tenant sees exactly that wallet and not the admin’s wallets.
- The original tenant still only sees its own wallet.
Once the server side honors
tenant_idinImportWallet/CreateWallet(see router.go comment), this will be a solid regression test for tenant-scoped wallet visibility.
564-614: Comprehensive RemoveTenant behavior testsThe
RemoveTenantsuite covers all the important cases:
NonExistent: verifiesNotFoundand error text.Default: protects the default admin tenant withInvalidArgument.WithWallets: ensures tenants with wallets (both the main test tenant and the “wallet-tenant”) are rejected withFailedPrecondition.WithoutWallets: validates the happy path by creating, fetching, deleting, and then observingNotFound.Permission: confirms a tenant-scoped macaroon getsPermissionDeniedfor tenant removal while admin succeeds.This is a good, end-to-end specification of the new RPC’s contract and will keep the server and DB logic honest.
docs/grpc.md (1)
329-329: LGTM! Documentation updates are clear and consistent.The documentation changes properly describe the new RemoveTenant RPC, including the important constraint that removal is only allowed when no wallets are associated. The typo fix and new tenant_id field in WalletParams are also correctly documented.
Note: The static analysis warning about the link fragment on line 341 appears to be a false positive, as the pattern
[.google.protobuf.Empty](#.google.protobuf.empty)is used consistently throughout this auto-generated documentation.Also applies to: 335-341, 1296-1304, 1643-1643
internal/database/tenant_test.go (1)
12-100: Excellent test coverage for tenant operations!The test comprehensively covers all critical tenant operations including creation, retrieval, listing, wallet associations, deletion, and the default tenant. The test structure is clear and well-organized using subtests.
cmd/boltzcli/commands.go (2)
1557-1558: Good improvements to user-facing text!The category name change from "wallet" to "Wallet", the updated usage text, and the typo fix ("ot" → "or") improve consistency and clarity.
Also applies to: 1595-1595
2327-2342: Tenant remove command looks good overall.The command is well-structured with clear description and proper error handling. Once the confirmation prompt is added, this will be production-ready.
kilrau
force-pushed
the
feat/add-tenant-remove
branch
from
b024cd6 to
fa71438
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/database/tenant.go (1)
7-41: Tighten error handling in CreateDefaultTenant, fix locking in write operations, and use sql.ErrNoRows properlyThree issues confirmed in this block:
CreateDefaultTenant swallows GetTenant errors
- Line 28:
defaultTenant, _ := d.GetTenant(DefaultTenantId)discards all errors, masking real database issues during startup.- Should explicitly check for
sql.ErrNoRowsand propagate other errors.Write operations lack d.lock synchronization
CreateTenant(lines 35–40) andDeleteTenant(lines 86–94) in tenant.go perform writes without acquiringd.lock.- This is inconsistent with read operations (
GetTenant,GetTenantByName,QueryTenants) which all used.lock.RLock().- Same pattern exists in wallet.go:
CreateWallet,UpdateWalletCredentials,DeleteWallet, andSetWalletSubaccountall lack locks.- For thread safety on shared state like
d.tx/d.db, write operations should acquired.lock.Lock().HasTenantWallets uses brittle string comparison
- Line 110:
if err.Error() == "sql: no rows in result set"should useerrors.Is(err, sql.ErrNoRows)instead, following Go standard library conventions.Example fixes:
func (d *Database) CreateDefaultTenant() error { - defaultTenant, _ := d.GetTenant(DefaultTenantId) - if defaultTenant == nil { + defaultTenant, err := d.GetTenant(DefaultTenantId) + if err != nil && !errors.Is(err, sql.ErrNoRows) { + return fmt.Errorf("failed to get default tenant: %w", err) + } + if defaultTenant != nil { + return nil + } - if err := d.CreateTenant(&Tenant{Id: DefaultTenantId, Name: DefaultTenantName}); err != nil { - return err - } - } - return nil + return d.CreateTenant(&Tenant{Id: DefaultTenantId, Name: DefaultTenantName}) } func (d *Database) CreateTenant(tenant *Tenant) error { + d.lock.Lock() + defer d.lock.Unlock() + query := "INSERT INTO tenants (name) VALUES (?) RETURNING id" row := d.QueryRow(query, tenant.Name) return row.Scan(&tenant.Id) } func (d *Database) HasTenantWallets(tenantId Id) (bool, error) { d.lock.RLock() defer d.lock.RUnlock() query := "SELECT 1 FROM wallets WHERE tenantId = ? LIMIT 1" var exists int err := d.QueryRow(query, tenantId).Scan(&exists) if err != nil { - if err.Error() == "sql: no rows in result set" { + if errors.Is(err, sql.ErrNoRows) { return false, nil } return false, fmt.Errorf("failed to check tenant wallets: %w", err) } return true, nil }Also applies to: DeleteTenant (lines 86–94) and wallet.go write methods.
🧹 Nitpick comments (1)
internal/database/tenant_test.go (1)
12-100: Good coverage of tenant lifecycle and wallet associationThis test suite nicely exercises tenant creation, lookup by id/name, listing, wallet association (
HasTenantWallets), deletion (including non‑existent id), and the default tenant invariants. For longer‑running test suites you might optionallydefer db.Close()afterConnect()to keep resource handling uniform across DB tests, but it’s not critical for an in‑memory database here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
pkg/boltzrpc/autoswaprpc/autoswaprpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/autoswaprpc/autoswaprpc_grpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/boltzrpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/boltzrpc_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
cmd/boltzcli/commands.go(3 hunks)docs/grpc.md(3 hunks)internal/database/tenant.go(7 hunks)internal/database/tenant_test.go(1 hunks)internal/macaroons/permissions.go(1 hunks)internal/rpcserver/router.go(2 hunks)internal/rpcserver/rpcserver_test.go(1 hunks)pkg/boltzrpc/boltzrpc.proto(3 hunks)pkg/boltzrpc/client/client.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/rpcserver/rpcserver_test.go
- internal/rpcserver/router.go
- pkg/boltzrpc/boltzrpc.proto
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-16T09:46:41.613Z
Learnt from: jackstar12
Repo: BoltzExchange/boltz-client PR: 487
File: internal/rpcserver/router.go:2304-2314
Timestamp: 2025-07-16T09:46:41.613Z
Learning: In the boltz-client codebase, RPC methods are protected by the macaroon permission system defined in internal/macaroons/permissions.go. Methods like GetSwapMnemonic and SetSwapMnemonic are already restricted to admin entities through the RPCServerPermissions map, so explicit isAdmin(ctx) checks are not needed in these methods.
Applied to files:
internal/macaroons/permissions.go
🧬 Code graph analysis (3)
internal/database/tenant_test.go (3)
internal/database/database.go (1)
Database(240-247)internal/database/tenant.go (3)
Tenant(9-12)DefaultTenantId(16-16)DefaultTenantName(18-18)pkg/boltz/currency.go (1)
CurrencyBtc(11-11)
internal/database/tenant.go (2)
pkg/boltzrpc/boltzrpc.pb.go (3)
Tenant(557-564)Tenant(579-579)Tenant(594-596)internal/database/database.go (1)
Database(240-247)
pkg/boltzrpc/client/client.go (1)
pkg/boltzrpc/boltzrpc.pb.go (3)
RemoveTenantRequest(510-516)RemoveTenantRequest(531-531)RemoveTenantRequest(546-548)
🪛 markdownlint-cli2 (0.18.1)
docs/grpc.md
341-341: Link fragments should be valid
(MD051, link-fragments)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI (ubuntu-latest, 1.24.x)
🔇 Additional comments (5)
internal/macaroons/permissions.go (1)
188-203: RemoveTenant RPC permission wiring looks correctThe new
/boltzrpc.Boltz/RemoveTenantentry reuses theadminwritepermission like other tenant-admin operations (CreateTenant, BakeMacaroon), which is consistent with the macaroon model in this codebase. No changes needed here.pkg/boltzrpc/client/client.go (1)
194-200: Client RemoveTenant wrapper is consistent with existing API surfaceThe
RemoveTenantmethod is a thin, idiomatic wrapper around the new RPC and matches the style of other client methods (CreateTenant, GetTenant, etc.). The doc comment correctly describes the high‑level semantics enforced server‑side.cmd/boltzcli/commands.go (3)
1555-1565: LGTM! Clean helper implementation.The helper function correctly handles both the "no tenant" case (empty string → nil) and tenant resolution with clear error messaging. The pattern is consistent with the existing
getWalletIdhelper.
1569-1623: LGTM! Tenant flag integration is clean and consistent.The tenant flag is correctly integrated into both wallet create and import commands with:
- Consistent flag definitions and usage descriptions
- Proper tenant resolution before wallet operations
- Appropriate error handling
Also fixes the typo on Line 1604: "BTC ot LBTC" → "BTC or LBTC".
2333-2362: LGTM! Addresses previous review feedback.The tenant remove command now includes:
- User confirmation prompt (addressing past review comment)
--forceflag for automation scenarios- Clear success messaging
- Proper error propagation from server
The implementation follows the same pattern as
removeWalletand correctly delegates wallet association validation to the server.
kilrau
force-pushed
the
feat/add-tenant-remove
branch
from
fa71438 to
ba61e0e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/boltzrpc/boltzrpc.proto (1)
779-786: Server currently ignoresWalletParams.tenant_idfor wallet creation/importThe new
tenant_idfield is documented as “Only relevant for admin users. Defaults to the current tenant.”, but ininternal/rpcserver/router.gothe ImportWallet/CreateWallet path still always usesrequireTenantId(ctx)when settingWalletCredentials.WalletInfo.TenantId, ignoringrequest.Params.TenantId. This makes admin‑sidetenant_idineffective and will cause theAdminCreateForTenanttest to fail, as well as not meeting the PR objective of targeting a specific tenant.You likely want logic in the RPC handler along the lines of:
- credentials := &onchain.WalletCredentials{ - WalletInfo: onchain.WalletInfo{ - Name: request.Params.Name, - Currency: currency, - TenantId: requireTenantId(ctx), - }, - ... - } + // Default to the current tenant (macaroon context), but allow admins to override via params.tenant_id. + tenantId := requireTenantId(ctx) + if isAdmin(ctx) && request.Params.TenantId != nil { + tenantId = database.Id(request.Params.GetTenantId()) + } + + credentials := &onchain.WalletCredentials{ + WalletInfo: onchain.WalletInfo{ + Name: request.Params.Name, + Currency: currency, + TenantId: tenantId, + }, + ... + }and the same behavior for CreateWallet, so that non‑admin tenants cannot escape their tenant, while admins can explicitly create wallets for another tenant.
internal/rpcserver/router.go (2)
1468-1495: Wallet creation/import ignoresWalletParams.TenantId, breaking admin “create for tenant” semanticsIn
ImportWallet, you always setcredentials.WalletInfo.TenantId = requireTenantId(ctx), andCreateWalletjust forwards to ImportWallet. As a result:
- Admins cannot choose a tenant via
params.tenant_id; wallets always end up under the current (or default) tenant.- The
AdminCreateForTenanttest ininternal/rpcserver/rpcserver_test.go(lines 527‑561) will fail, because it expectsCreateWalletwith aTenantIdset in params to create a wallet owned by that tenant.You likely want behavior along these lines:
func (server *routedBoltzServer) ImportWallet(ctx context.Context, request *boltzrpc.ImportWalletRequest) (*boltzrpc.Wallet, error) { @@ - currency := serializers.ParseCurrency(&request.Params.Currency) + currency := serializers.ParseCurrency(&request.Params.Currency) + + // Determine target tenant: + // - Non‑admin callers are restricted to their own tenant (macaroon/context). + // - Admin callers can override via params.tenant_id. + tenantId := requireTenantId(ctx) + if isAdmin(ctx) && request.Params.TenantId != nil { + tenantId = database.Id(request.Params.GetTenantId()) + } @@ credentials := &onchain.WalletCredentials{ WalletInfo: onchain.WalletInfo{ Name: request.Params.Name, Currency: currency, - TenantId: requireTenantId(ctx), + TenantId: tenantId, },
CreateWalletcan stay as a thin wrapper around ImportWallet. This preserves tenant isolation for normal tenants while giving admins the explicit override hook promised by the proto comment and tests.
3-6: Update router.go to use*emptypb.Emptyconsistently for all RPC methodsVerification confirms the type mismatch: the generated BoltzServer interface expects
*emptypb.Emptyfor Stop, Unlock, ChangeWalletPassword, GetPairs, and RemoveTenant. However, router.go currently uses*empty.Emptyfor Stop, Unlock, ChangeWalletPassword, and GetPairs—only RemoveTenant correctly uses*emptypb.Empty.Update the following methods to use
*emptypb.Emptyand remove the deprecatedgithub.com/golang/protobuf/ptypes/emptyimport:
- Stop (line 1920): parameter and return type
- Unlock (line 1966): return type
- ChangeWalletPassword (line 2094): return type
- GetPairs (line 2202): parameter type
🧹 Nitpick comments (5)
internal/database/tenant.go (3)
26-35: Consider propagating unexpected errors fromGetTenantinCreateDefaultTenant
CreateDefaultTenantignores the error fromGetTenant(DefaultTenantId)and only checks whether the returned tenant is nil. That means any DB error (e.g. connection issue) is treated as “no default tenant”, and you then proceed to create one, potentially masking a real problem.You may want to distinguish the “no rows” case from others, e.g.:
- defaultTenant, _ := d.GetTenant(DefaultTenantId) - if defaultTenant == nil { + defaultTenant, err := d.GetTenant(DefaultTenantId) + if errors.Is(err, sql.ErrNoRows) { + defaultTenant = nil + } else if err != nil { + return err + } + if defaultTenant == nil { if err := d.CreateTenant(&Tenant{Id: DefaultTenantId, Name: DefaultTenantName}); err != nil { return err } }so unexpected DB failures surface instead of being silently retried as a create.
93-105: Handle potential error fromRowsAffectedinDeleteTenant
DeleteTenantchecksRowsAffected()but discards its error:if rows, _ := result.RowsAffected(); rows == 0 { return fmt.Errorf("failed to delete tenant with id %d", id) }In normal cases this is fine, but ignoring the error can hide driver issues or statement types that don’t support
RowsAffected.A slightly safer pattern is:
- if rows, _ := result.RowsAffected(); rows == 0 { - return fmt.Errorf("failed to delete tenant with id %d", id) - } + rows, err := result.RowsAffected() + if err != nil { + return fmt.Errorf("failed to determine affected rows for tenant %d: %w", id, err) + } + if rows == 0 { + return fmt.Errorf("failed to delete tenant with id %d", id) + }This keeps your current behavior while surfacing unexpected driver errors.
107-121:HasTenantWalletsbehavior and race window are acceptable but rely on DB constraintsThe
HasTenantWalletsimplementation (RLock, simpleSELECT 1 ... LIMIT 1, treatingsql.ErrNoRowsas “no wallets”) is correct and fixes the previous brittleerr.Error() == "sql: no rows in result set"pattern.There is still a small race window between the
HasTenantWalletscheck andDeleteTenantin the RemoveTenant RPC; if you don’t already have a foreign key constraintwallets.tenantId REFERENCES tenants(id) ON DELETE RESTRICT, a concurrent wallet creation could slip in and leave orphaned wallets. If you don’t already, consider enforcing this at the schema level so the DB will reject deletions that would violate the “no wallets” guarantee, even under concurrency.docs/grpc.md (1)
335-341: RemoveTenant RPC documentation is consistent with the API and CLIThe new
RemoveTenantmethod andRemoveTenantRequestmessage are documented with:
- A clear precondition: only allowed when the tenant has no associated wallets.
- A
namefield onRemoveTenantRequest, consistent withGetTenantRequest.This aligns with the new
tenant removeCLI behavior and the intended safety check on the server side.If the RPC is admin-only (admin macaroon required), consider explicitly noting that in the
RemoveTenantdescription for extra clarity.Also applies to: 1296-1304
cmd/boltzcli/commands.go (1)
1555-1565: Avoid masking non‑NotFound errors ingetTenantIdFromNameRight now any error from
client.GetTenantis turned into “tenant '' does not exist”, which hides permission, connectivity, or server errors and can mislead users.Consider only mapping real “not found” errors to that message and otherwise returning the original error:
func getTenantIdFromName(ctx *cli.Context, tenantName string) (*uint64, error) { if tenantName == "" { return nil, nil } client := getClient(ctx) tenant, err := client.GetTenant(tenantName) if err != nil { - return nil, fmt.Errorf("tenant '%s' does not exist", tenantName) + st, ok := status.FromError(err) + if ok && st.Code() == codes.NotFound { + return nil, fmt.Errorf("tenant '%s' does not exist", tenantName) + } + return nil, err } return &tenant.Id, nil }You’d also need to import
codesalongside the existingstatusimport:import ( // ... "google.golang.org/grpc/codes" )This keeps the nicer “does not exist” message for actual 404s without obscuring other failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
pkg/boltzrpc/autoswaprpc/autoswaprpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/autoswaprpc/autoswaprpc_grpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/boltzrpc.pb.gois excluded by!**/*.pb.gopkg/boltzrpc/boltzrpc_grpc.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
cmd/boltzcli/commands.go(3 hunks)docs/grpc.md(7 hunks)internal/database/tenant.go(7 hunks)internal/database/tenant_test.go(1 hunks)internal/macaroons/permissions.go(1 hunks)internal/rpcserver/router.go(2 hunks)internal/rpcserver/rpcserver_test.go(1 hunks)pkg/boltzrpc/boltzrpc.proto(3 hunks)pkg/boltzrpc/client/client.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/database/tenant_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-16T09:46:41.613Z
Learnt from: jackstar12
Repo: BoltzExchange/boltz-client PR: 487
File: internal/rpcserver/router.go:2304-2314
Timestamp: 2025-07-16T09:46:41.613Z
Learning: In the boltz-client codebase, RPC methods are protected by the macaroon permission system defined in internal/macaroons/permissions.go. Methods like GetSwapMnemonic and SetSwapMnemonic are already restricted to admin entities through the RPCServerPermissions map, so explicit isAdmin(ctx) checks are not needed in these methods.
Applied to files:
internal/macaroons/permissions.gointernal/rpcserver/rpcserver_test.go
🧬 Code graph analysis (4)
internal/rpcserver/router.go (2)
pkg/boltzrpc/boltzrpc.pb.go (3)
RemoveTenantRequest(510-516)RemoveTenantRequest(531-531)RemoveTenantRequest(546-548)internal/database/tenant.go (1)
DefaultTenantId(18-18)
pkg/boltzrpc/client/client.go (1)
pkg/boltzrpc/boltzrpc.pb.go (3)
RemoveTenantRequest(510-516)RemoveTenantRequest(531-531)RemoveTenantRequest(546-548)
internal/rpcserver/rpcserver_test.go (3)
pkg/boltzrpc/boltzrpc.pb.go (16)
WalletParams(4492-4503)WalletParams(4518-4518)WalletParams(4533-4535)Currency(132-132)Currency(161-163)Currency(165-167)Currency(174-176)Wallet(5899-5910)Wallet(5925-5925)Wallet(5940-5942)BakeMacaroonRequest(659-666)BakeMacaroonRequest(681-681)BakeMacaroonRequest(696-698)Wallets(5986-5992)Wallets(6007-6007)Wallets(6022-6024)pkg/boltzrpc/client/client.go (2)
FullPermissions(20-23)NewBoltzClient(29-34)pkg/boltzrpc/client/connection.go (1)
Connection(17-29)
internal/database/tenant.go (2)
pkg/boltzrpc/boltzrpc.pb.go (3)
Tenant(557-564)Tenant(579-579)Tenant(594-596)internal/database/database.go (1)
Database(240-247)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI (ubuntu-latest, 1.24.x)
🔇 Additional comments (11)
pkg/boltzrpc/boltzrpc.proto (1)
193-201: RemoveTenant RPC/message shape looks goodThe RemoveTenant RPC and
RemoveTenantRequest { string name = 1; }are minimal and consistent with the server implementation that removes tenants by name with safety checks. No issues from the proto side.Also applies to: 234-237
internal/macaroons/permissions.go (1)
200-203: RemoveTenant correctly restricted to admin/write via macaroonsThe
/boltzrpc.Boltz/RemoveTenantentry using{Entity: "admin", Action: "write"}aligns with the authorization model and avoids needing extraisAdminchecks in the handler.pkg/boltzrpc/client/client.go (1)
194-197: ClientRemoveTenantwrapper is consistent and sufficientThe new
RemoveTenant(name string) errorhelper correctly forwards the name intoRemoveTenantRequestand follows the same pattern as other client methods.internal/rpcserver/router.go (1)
2345-2377: RemoveTenant handler enforces the right safety checksThe RemoveTenant RPC implementation correctly:
- looks up the tenant by name and returns
codes.NotFoundwhen missing,- prevents deletion of the default tenant by ID,
- runs a transaction that checks
HasTenantWalletsand returnscodes.FailedPreconditionwhen wallets exist,- and deletes the tenant when safe, returning an empty response.
This matches the intended semantics from the issue and the tests.
internal/rpcserver/rpcserver_test.go (1)
564-613: RemoveTenant tests comprehensively cover edge cases and permissionsThe
RemoveTenanttest block nicely exercises:
- non-existent name →
codes.NotFound,- attempt to remove default tenant →
codes.InvalidArgument,- tenants with wallets (both the main
tenantNameand"wallet-tenant") →codes.FailedPrecondition,- empty tenant lifecycle (create, remove, then
GetTenant→codes.NotFound),- and permission enforcement (tenant macaroon cannot remove, admin can).
These expectations align with the RemoveTenant handler’s logic and the macaroon permissions map.
docs/grpc.md (3)
85-85: Inline.google.protobuf.Emptytype formatting looks good and fixes the anchor issueSwitching from fragment links to inline code for
.google.protobuf.Emptyin these method tables keeps the docs clear and should resolve the previous markdownlint MD051 complaint about invalid#.google.protobuf.emptyanchors. No further change needed here.Also applies to: 285-285, 293-293, 309-309, 341-341, 1888-1888
329-329: GetTenant description typo fix is correctThe description now reads “Get a specific tenant.” which matches the RPC behavior and removes the previous typo.
1643-1643: WalletParams.tenant_id docs match new wallet tenant association behaviorDocumenting
tenant_idas an optional admin-only field that defaults to the current tenant mirrors the newwallet create/import --tenantCLI wiring and clarifies how wallets are scoped.cmd/boltzcli/commands.go (3)
1568-1571: Wallet command metadata is clearerChanging the wallet command
Categoryto"Wallet"andUsageto"Manage wallets"is concise and consistent with the other top‑level command categories.
1579-1596: Wallet create/import correctly support--tenantBoth
wallet createandwallet importnow:
- Resolve the
--tenantflag viagetTenantIdFromName, failing early when the tenant name is invalid.- Wire the resulting
*uint64intoWalletParams.TenantIdbefore callingcreateWallet/importWallet.- Document the flag as “Tenant name to associate the wallet with (admin only)”.
- Fix the “BTC ot LBTC” typo in the import description.
This cleanly exposes tenant scoping at the CLI level and matches the new
WalletParams.tenant_idfield on the RPC side.Also applies to: 1604-1623
2333-2362: Tenant remove command matches destructive‑operation UX expectationsThe new
tenant removesubcommand:
- Requires a tenant name via
requireNArgs(1, ...).- Prompts for confirmation by default, clearly including the tenant name in the message.
- Honors a
--force/-fflag to skip the prompt when desired.- Delegates to
client.RemoveTenant(tenantName)and reports success.This is consistent with the existing
wallet removeflow while adding a standard force override for scripted use.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/rpcserver/router.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/rpcserver/router.go (3)
internal/database/database.go (1)
Id(336-336)internal/onchain/wallet.go (1)
WalletInfo(34-40)internal/database/tenant.go (1)
DefaultTenantId(18-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI (ubuntu-latest, 1.24.x)
🔇 Additional comments (2)
internal/rpcserver/router.go (2)
44-44: LGTM: Import added for empty protobuf response.The
emptypbimport is correctly added to support theRemoveTenantmethod's return type.
1476-1495: LGTM: Admin tenant resolution logic is correct.The implementation correctly allows admin users to associate a wallet with any tenant while falling back to the context tenant for non-admin users. The tenant existence validation and error handling are appropriate.
kilrau
force-pushed
the
feat/add-tenant-remove
branch
from
1163ca6 to
6c016cc
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
| return result, nil | ||
| } | ||
|
|
||
| // parseTenant parses a database row into a Tenant struct. |
There was a problem hiding this comment.
For future reference, tell claude to skip adding these useless comments
There was a problem hiding this comment.
I did, several times. Removed manually as well but must have re-added in some commit and I missed it.
3 participants
closes #161
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.