Arkade Asset Exploration #814
Conversation
WalkthroughAdds end-to-end asset support: TLV asset packets and OpReturn extension, asset encoding/decoding, validation state machines, domain models and repositories (sqlite/postgres/badger) with migrations, service/indexer/gRPC integration (GetAssetGroup, teleport events), asset-aware tx building/forfeit verification, script helpers, tests, and new error codes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant AssetValidator
participant Repo
participant DB
Client->>Service: SubmitOffchainTx(intent, psbt)
activate Service
Service->>AssetValidator: Decode & validate asset packet(s)
activate AssetValidator
AssetValidator->>Repo: Query/GetAssetGroup / GetAssetByOutpoint
activate Repo
Repo->>DB: SQL queries (asset_group / anchors / metadata / teleport_asset)
DB-->>Repo: rows
deactivate Repo
AssetValidator-->>Service: validation result (ok / error)
deactivate AssetValidator
alt validation OK
Service->>Repo: InsertAssetGroup / InsertAssetAnchor / CreateTeleportAsset
Repo->>DB: insert/update SQL
DB-->>Repo: OK
Service-->>Client: success
else validation FAIL
Service-->>Client: error (ASSET_* codes)
end
deactivate Service
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
… update encoding/decoding logic
- Created a new CheckpointTx Builder - Modified BuildTxs to include the logic for Building AssetTransfer transaction
pkg/ark-lib/asset/asset.go
Outdated
| assetData = append(assetData, a.genesisTxId...) | ||
| assetData = append(assetData, batchTxId...) | ||
| assetData = append(assetData, encodedTlv...) |
There was a problem hiding this comment.
maybe we should encode these as var len bytes like len(buf)+buf?
pkg/ark-lib/extension/asset.go
Outdated
| } | ||
|
|
||
| func (a *Asset) encodeTlv() ([]byte, error) { | ||
| var tlvRecords []tlv.Record |
There was a problem hiding this comment.
do we really need this external dep tls.Record to serilalize/deserialize asset data?
There was a problem hiding this comment.
We can always implement the encoding and decoding logic using TLV schematics, which @Kukks recommended in the Arkade Asset docs https://github.com/ArkLabsHQ/arkade-assets/blob/master/arkade-assets.md#3-packet-format-canonical-tlv , but the tlv library provides a fast way of doing it, but in the future it can always be removed for custom implementation
pkg/ark-lib/asset/encoding.go
Outdated
| tlvTypeAssetID tlv.Type = 1 | ||
| tlvTypeOutput tlv.Type = 2 | ||
| tlvTypeControlPubkey tlv.Type = 3 | ||
| tlvTypeInput tlv.Type = 4 | ||
| tlvTypeMetadata tlv.Type = 6 | ||
| tlvTypeOutScriptPubKey tlv.Type = 7 | ||
| tlvTypeInTxid tlv.Type = 8 | ||
| tlvTypeOutAmount tlv.Type = 9 | ||
| tlvTypeInVout tlv.Type = 10 | ||
| tlvTypeImmutable tlv.Type = 11 |
There was a problem hiding this comment.
use iota:
| tlvTypeAssetID tlv.Type = 1 | |
| tlvTypeOutput tlv.Type = 2 | |
| tlvTypeControlPubkey tlv.Type = 3 | |
| tlvTypeInput tlv.Type = 4 | |
| tlvTypeMetadata tlv.Type = 6 | |
| tlvTypeOutScriptPubKey tlv.Type = 7 | |
| tlvTypeInTxid tlv.Type = 8 | |
| tlvTypeOutAmount tlv.Type = 9 | |
| tlvTypeInVout tlv.Type = 10 | |
| tlvTypeImmutable tlv.Type = 11 | |
| tlvTypeAssetID tlv.Type = iota | |
| tlvTypeOutput | |
| tlvTypeControlPubkey | |
| tlvTypeInput | |
| tlvTypeMetadata | |
| tlvTypeOutScriptPubKey | |
| tlvTypeInTxid | |
| tlvTypeOutAmount | |
| tlvTypeInVout | |
| tlvTypeImmutable |
…logic for output handling
…s and improve clarity
- Update Receiver struct to include AssetAmount, AssetId, and AssetTeleport fields. - Modify RegisterIntent method to handle asset outputs and seals. - Enhance asset output retrieval and encoding in transaction creation. - Introduce ArkAssetSealVtxoField for marking asset seal inputs in PSBT. - Refactor tree builder to accommodate multiple outputs, including asset scripts.
… add tests for asset seal vtxo handling
… struct; update transaction building and tests accordingly
…xosLeaves; add logging for script generation
…issuance and burning; update teleport pubkey comparison in utils
…oding, and validation logic accordingly
…roups in offchain transaction processing
…set handling and introduce byte reversal utility
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/core/application/indexer.go (1)
246-263: Bug: Asset enrichment happens after pagination but modifies wrong slice.The enrichment loop at lines 248-258 modifies
allVtxos[j], butvtxos(returned at line 261) was already sliced fromallVtxosat line 246 before enrichment. Since slice elements are copied by value, the enriched extensions won't appear in the returnedvtxosslice.Either enrich before pagination or iterate over
vtxosinstead ofallVtxos.Proposed fix: enrich before pagination
- vtxos, pageResp := paginate(allVtxos, page, maxPageSizeSpendableVtxos) - - for j, v := range allVtxos { + for j, v := range allVtxos { // add asset to vtxo if present asst, err := i.repoManager.Assets().GetAssetByOutpoint(ctx, v.Outpoint) if err == nil && asst != nil { assetExtension := domain.AssetExtension{ AssetID: asst.AssetID, Amount: asst.Amount, } allVtxos[j].Extensions = append(allVtxos[j].Extensions, assetExtension) } } + + vtxos, pageResp := paginate(allVtxos, page, maxPageSizeSpendableVtxos) return &GetVtxosResp{internal/infrastructure/db/service.go (1)
219-289: Missing asset store creation for Postgres.The postgres case creates stores for round, vtxo, scheduledSession, offchainTx, conviction, and intentFees, but does not create the asset store. Compare with sqlite (lines 350-353) and badger (lines 211-214) cases which both create
assetStore.Proposed fix: add asset store creation for postgres
intentFeesStore, err = intentFeesStoreFactory(db) if err != nil { return nil, fmt.Errorf("failed to create intent fees store: %w", err) } + assetStore, err = assetStoreFactory(db) + if err != nil { + return nil, fmt.Errorf("failed to create asset store: %w", err) + } case "sqlite":
🤖 Fix all issues with AI agents
In `@api-spec/protobuf/ark/v1/indexer.proto`:
- Around line 78-83: Update the protobuf doc comments to match the RPC names and
fix typos: change the comment that incorrectly says "GetAsset" to
"GetAssetGroup" and ensure it describes returning asset group information for
the specified asset ID (for rpc GetAssetGroup and messages
GetAssetGroupRequest/GetAssetGroupResponse), and correct any misspelling of
"subscribtion" to "subscription" in the other RPC doc comments (the similar typo
around the RPCs at the later block referenced by lines 110-112).
In `@internal/core/application/asset_validation.go`:
- Around line 339-369: The validateInput branch for AssetTypeTeleport
dereferences intent.Proof without checking if intent is nil; update
validateInput to check the return from s.repoManager.Rounds().GetIntentByTxid
(the intent variable) for nil before using intent.Proof, and if nil return a
TELEPORT_VALIDATION_FAILED error (using errors.TELEPORT_VALIDATION_FAILED.New
with a clear message like "intent not found for teleport input validation" and
WithMetadata(errors.TeleportValidationMetadata{})); keep the existing error
handling for GetIntentByTxid failures and proceed to decode intent.Proof only
when intent is non-nil so decodedProof and subsequent calls
(s.validateTeleportInput) cannot panic.
In `@internal/core/application/indexer.go`:
- Around line 274-284: The loop over allVtxos is appending a
domain.AssetExtension with Amount incorrectly hardcoded to 0; update the
creation of assetExtension in the block where
i.repoManager.Assets().GetAssetByOutpoint(ctx, v.Outpoint) returns asst so that
AssetExtension.Amount is set to asst.Amount (use the actual asset amount) before
appending to allVtxos[j].Extensions; ensure you reference allVtxos,
i.repoManager.Assets().GetAssetByOutpoint, and domain.AssetExtension when making
the change.
In `@pkg/errors/errors.go`:
- Around line 381-443: ASSET_NOT_FOUND and INTENT_NOT_FOUND currently share the
same numeric code (34); change INTENT_NOT_FOUND's numeric ID to an unused value
(e.g., 45) in its Code literal so all error codes are unique. Update the
INTENT_NOT_FOUND declaration (the Code[any] composite literal) to use the new
integer while keeping its name and grpc code intact.
♻️ Duplicate comments (20)
internal/infrastructure/db/service_test.go (1)
1706-1743: Avoid asset group ID collisions across subtests.Subtests share the same DB;
"asset-1"is already inserted earlier and can cause a PK conflict here. Use a unique ID (or reset state between subtests).🐛 Suggested fix
- asset := domain.AssetGroup{ - ID: "asset-1", + asset := domain.AssetGroup{ + ID: "asset-quantity-test",internal/core/application/service.go (5)
1131-1138: Asset storage failures are still best-effort.
storeAssetDetailsFromArkTxerrors are logged but the offchain tx is accepted, which can desync asset state. Decide and enforce the intended semantics.🐛 One possible fix (fail the acceptance)
- if assetOutputIndex >= 0 { - if err := s.storeAssetDetailsFromArkTx( - ctx, *arkPtx.UnsignedTx, assetOutputIndex, - ); err != nil { - log.WithError(err).Errorf( - "failed to store asset details for offchain tx %s", txid, - ) - } - } + if assetOutputIndex >= 0 { + if err := s.storeAssetDetailsFromArkTx( + ctx, *arkPtx.UnsignedTx, assetOutputIndex, + ); err != nil { + return nil, errors.INTERNAL_ERROR.New( + "failed to store asset details for offchain tx %s: %w", txid, err, + ) + } + }
1611-1619: Dead error check after ignoring GetArkPsbtFields error.
erris discarded and then checked, which is stale/always-nil.🐛 Suggested fix
- // we ignore error cause sometimes the taproot tree is not required - taptreeFields, _ := txutils.GetArkPsbtFields( + // we ignore error cause sometimes the taproot tree is not required + taptreeFields, err := txutils.GetArkPsbtFields( &proof.Packet, i+1, txutils.VtxoTaprootTreeField, ) - if err != nil { + if err != nil { return "", errors.INVALID_PSBT_INPUT.New( "failed to get asset seal field for input %d: %w", i+1, err, ).WithMetadata(errors.InputMetadata{Txid: proofTxid, InputIndex: i + 1}) }
1928-1967: Teleport asset persistence still ignores errors and uses loop index.Insert errors are logged but do not fail registration, and
OutputIndexusesirather than the encoded vout.🐛 Suggested fix
- if err := s.repoManager.Assets().InsertTeleportAsset(ctx, domain.TeleportAsset{ - IntentID: intent.Id, - Script: teleportScript, - OutputIndex: uint32(i), - AssetID: asst.AssetId.ToString(), - Amount: output.Amount, - IsClaimed: false, - }); err != nil { - log.WithError(err).Warn("failed to insert teleport asset") - } + if err := s.repoManager.Assets().InsertTeleportAsset(ctx, domain.TeleportAsset{ + IntentID: intent.Id, + Script: teleportScript, + OutputIndex: uint32(output.Vout), + AssetID: asst.AssetId.ToString(), + Amount: output.Amount, + IsClaimed: false, + }); err != nil { + return "", errors.INTERNAL_ERROR.New("failed to insert teleport asset: %w", err) + }
4488-4509: Surface insufficient-balance errors on quantity decrease.
DecreaseAssetGroupQuantitycan no-op if quantity is insufficient; callers should get a deterministic error.
4533-4538: Teleport events should use the real output vout.
OutputVoutshould reflectassetOut.Vout, not the loop index.🐛 Suggested fix
- OutputVout: uint32(outIdx), + OutputVout: assetOut.Vout,internal/core/application/asset_validation.go (3)
431-465: Enforce non-zero asset amounts.Inputs/outputs should reject zero amounts per spec.
🐛 Suggested fix
case assetOutputInit: m.sumInputs = sumAssetInputs(m.assetGp.Inputs) m.sumOutputs = sumAssetOutputs(m.assetGp.Outputs) m.teleportInputs = make(map[string]uint64) m.localScriptAmts = make(map[string]uint64) m.processed = 0 for _, in := range m.assetGp.Inputs { + if in.Amount == 0 { + return errors.ASSET_INPUT_INVALID.New("asset input amount must be > 0"). + WithMetadata(errors.AssetInputMetadata{}) + } if in.Type == extension.AssetTypeTeleport { teleportScriptHex := hex.EncodeToString(in.Witness.Script[:]) m.teleportInputs[teleportScriptHex] += in.Amount } } m.state = assetOutputCollect case assetOutputCollect: for _, assetOut := range m.assetGp.Outputs { + if assetOut.Amount == 0 { + return errors.ASSET_OUTPUT_INVALID.New("asset output amount must be > 0"). + WithMetadata(errors.AssetOutputMetadata{}) + } switch assetOut.Type {
567-574: Nil AssetId check needed before dereference.Issuance groups can have a nil AssetId; dereferencing will panic.
🐛 Suggested fix
- if assetOutput.Type == extension.AssetTypeTeleport && + if assetGroup.AssetId != nil && + assetOutput.Type == extension.AssetTypeTeleport && bytes.Equal(assetOutput.Script, script) && assetId == *assetGroup.AssetId && index == uint32(i) {
595-654: Asset input matching should be scoped to asset ID/type.Current matching scans all outputs across all groups, allowing cross-group or teleport outputs to satisfy a local input.
internal/core/application/utils.go (2)
219-229: Match assets by outpoint, not filtered slice index.
outsexcludes anchor outputs, so indexing byVOutcan skip or mis-attach assets when the slice is filtered. Build avout -> outs indexmap before attaching extensions to avoid silent drops.🔧 Suggested fix
- // Add AssetGroup if Present - for _, asst := range assetList { - idx := int(asst.VOut) - if idx < 0 || idx >= len(outs) { - continue - } - - outs[idx].Extensions = append(outs[idx].Extensions, domain.AssetExtension{ - AssetID: asst.AssetID, - Amount: asst.Amount, - }) - } + // Add AssetGroup if Present - match by outpoint + outIndexByVout := make(map[uint32]int, len(outs)) + for i := range outs { + outIndexByVout[outs[i].Outpoint.VOut] = i + } + for _, asst := range assetList { + idx, ok := outIndexByVout[asst.VOut] + if !ok { + continue + } + outs[idx].Extensions = append(outs[idx].Extensions, domain.AssetExtension{ + AssetID: asst.AssetID, + Amount: asst.Amount, + }) + }
195-198: Guard P2TR script slicing to avoid panics on short/non-P2TR scripts.
out.PkScript[2:]assumes a 34-byte P2TR script; short or non-P2TR scripts can panic or yield bogus keys. Add a length/type guard before slicing.🔒 Suggested guard
} else { + if len(out.PkScript) < 34 { + continue // skip non-P2TR outputs + } pubKey = hex.EncodeToString(out.PkScript[2:]) isSubDust = script.IsSubDustScript(out.PkScript) }internal/infrastructure/db/sqlite/sqlc/query.sql (2)
422-427: Prevent silent no-op on AddAsset asset_id mismatch.With
ON CONFLICT ... WHERE asset.asset_id = excluded.asset_id, a mismatch yields zero rows updated with no error. This can silently drop writes. Consider:execrowsand treat 0 rows affected as a conflict, or change the constraint/update semantics.🔧 Suggested SQLC change (and handle rows affected)
--- name: AddAsset :exec +-- name: AddAsset :execrows INSERT INTO asset (anchor_id, asset_id, vout, amount) VALUES (?, ?, ?, ?) ON CONFLICT(anchor_id, vout) DO UPDATE SET amount = excluded.amount WHERE asset.asset_id = excluded.asset_id;
460-463: Surface insufficient-quantity failures in SubtractFromAssetQuantity.The guarded update can return 0 rows without error when quantity is insufficient or the group is missing. Use
:execrowsand check rows affected to return a domain error.🔧 Suggested SQLC change (and handle rows affected)
--- name: SubtractFromAssetQuantity :exec +-- name: SubtractFromAssetQuantity :execrows UPDATE asset_group SET quantity = quantity - ? WHERE id = ? AND quantity >= ?;internal/infrastructure/db/postgres/sqlc/queries/query.sql.go (2)
16-38: Prevent silent no-op on AddAsset (generated SQLC).
ON CONFLICT ... WHERE asset.asset_id = EXCLUDED.asset_idcan result in zero rows updated with no error. Update the source SQL to:execrowsand treat 0 rows affected as a conflict (then regenerate).
1969-1983: Surface insufficient-quantity failures in SubtractFromAssetQuantity (generated SQLC).The guarded update can silently succeed with zero rows; use
:execrowsand check rows affected in the repo layer (then regenerate).internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go (2)
14-37: Prevent silent no-op on AddAsset (generated SQLC).With the guarded
ON CONFLICTclause, an asset_id mismatch results in zero rows updated without an error. Fix in the source SQL (:execrows) and handle 0 rows affected, then regenerate.
2036-2051: Surface insufficient-quantity failures in SubtractFromAssetQuantity (generated SQLC).The guard can yield 0 rows with no error, masking insufficient balance or missing group. Switch to
:execrowsin the source SQL and handle the rows-affected result.api-spec/openapi/swagger/ark/v1/indexer.openapi.json (1)
8-24: Path parameter naming mismatch still exists.The path uses
{asset_id}(snake_case) but the parameter name isassetId(camelCase). This inconsistency may cause routing issues depending on the framework.internal/interface/grpc/handlers/indexer.go (2)
61-66: Still missing nil check forresp.AssetGroup.While the code now checks
resp == nilat line 61, if the service returns a non-nil response with a nilAssetGroup, accessingresp.AssetGroup.Metadataat line 66 will panic.Proposed fix
if resp == nil { return nil, status.Errorf(codes.NotFound, "asset not found: %s", assetId) } + if resp.AssetGroup == nil { + return nil, status.Errorf(codes.NotFound, "asset group not found for id %s", assetId) + } assetMetadata := make([]*arkv1.AssetMetadata, 0)
486-494: Both listener channels could still be nil, causing unexpected behavior.If neither a script subscription nor a teleport subscription exists for the given
subscriptionId, bothscriptChandteleportChwill be nil. While the select won't technically block forever (timer and context.Done work), this silently succeeds when no subscriptions exist.Proposed fix
scriptCh, err := h.scriptSubsHandler.getListenerChannel(subscriptionId) if err != nil && !strings.Contains(err.Error(), "listener not found") { return status.Error(codes.Internal, err.Error()) } teleportCh, err := h.teleportSubsHandler.getListenerChannel(subscriptionId) if err != nil && !strings.Contains(err.Error(), "listener not found") { return status.Error(codes.Internal, err.Error()) } + if scriptCh == nil && teleportCh == nil { + return status.Error(codes.NotFound, "no active subscriptions found for id") + } + // create a Timer that will fire after one heartbeat interval
🧹 Nitpick comments (9)
internal/infrastructure/db/postgres/migration/20260114000000_add_intent_txid.up.sql (1)
1-2: Add a txid index (and optional uniqueness) to match rollback and lookups.The down migration drops
idx_intent_txid, but the up migration doesn’t create it. IfGetIntentByTxidis used, an index avoids full scans and keeps the migration pair symmetric.💡 Suggested migration addition
ALTER TABLE intent ADD COLUMN txid TEXT; + +CREATE INDEX idx_intent_txid ON intent (txid); +-- If txid should be 1:1 with intent, consider UNIQUE instead.api-spec/protobuf/ark/v1/service.proto (1)
153-157: Confirmtxidis actually bindable on GET.Please verify that
meshapi.gateway.httpsupports mappingoneofquery parameters (e.g.,/v1/intent?txid=...). If not, consider an explicit path binding (e.g.,/v1/intent/{txid}) or a non-oneof request field.internal/infrastructure/db/sqlite/migration/20260114000000_add_intent_txid.up.sql (1)
1-2: Add a txid index to match rollback and lookup usage.The down migration removes
idx_intent_txid, but the up migration doesn’t create it. Consider adding the index to prevent full scans on txid lookups.💡 Suggested migration addition
ALTER TABLE intent ADD COLUMN txid TEXT; + +CREATE INDEX idx_intent_txid ON intent (txid);internal/infrastructure/db/postgres/intent_txid_migration_test.go (2)
20-27: Consider adding table cleanup to improve test isolation.The test creates tables but doesn't drop them in
t.Cleanup. This could cause issues if tests are run multiple times or in parallel.♻️ Suggested cleanup addition
t.Cleanup(func() { + // Drop test tables to ensure clean state for subsequent runs + //nolint:errcheck + db.Exec(`DROP TABLE IF EXISTS intent; DROP TABLE IF EXISTS round;`) //nolint:errcheck db.Close() })
76-77: Consider usingdefer rows.Close()immediately after the query.While the current placement works, the idiomatic Go pattern is to defer
Close()right after successful query execution to ensure cleanup even if early returns or panics occur.♻️ Suggested refactor
rows, err := db.Query(` SELECT id, txid, proof FROM intent; `) require.NoError(t, err) + defer rows.Close() var got []row // check we have two rows with txids populated for rows.Next() { var r row err = rows.Scan(&r.ID, &r.Txid, &r.Proof) require.NoError(t, err) got = append(got, r) } require.NoError(t, rows.Err()) require.Len(t, got, 2) - - // nolint:errcheck - rows.Close()internal/infrastructure/db/badger/ark_repo.go (1)
300-305: Intentionally silent error handling on index upsert.The
nolint:errcheckis documented with the rationale that index failures should not fail the critical round upsert. This is a reasonable trade-off, though consider logging failures for observability.Optional: log index upsert failures for observability
// upsert intent index for each intent with a txid for _, it := range rnd.Intents { - // do not fail the whole round upsert if intent index upsert fails - // nolint:errcheck - r.upsertIntentIndex(ctx, it.Txid, rnd.Id, it.Id) + if err := r.upsertIntentIndex(ctx, it.Txid, rnd.Id, it.Id); err != nil { + // Log but don't fail the whole round upsert + // Consider adding: log.WithError(err).Warnf("failed to upsert intent index for txid %s", it.Txid) + } }internal/infrastructure/db/sqlite/intent_txid_migration.go (1)
24-31: Use%wfor error wrapping to preserve error chain.Lines 26 and 31 use
%sfor error formatting, but the corresponding PostgreSQL migration uses%w. This loses the underlying error information for debugging and error inspection witherrors.Is/errors.As.Proposed fix
// backfill existing intents with derived txids from proof (in-place UPDATE) if err := backfillIntent(ctx, dbh); err != nil { - return fmt.Errorf("failed to backfill txids: %s", err) + return fmt.Errorf("failed to backfill txids: %w", err) } // create index on intent txid column if err := createIntentTxidIndex(ctx, dbh); err != nil { - return fmt.Errorf("failed to create intent txid index: %s", err) + return fmt.Errorf("failed to create intent txid index: %w", err) }api-spec/openapi/swagger/ark/v1/service.openapi.json (1)
371-409: Add description for the GetIntent endpoint.Unlike other endpoints in this spec (e.g.,
GetInfo,RegisterIntent,SubmitTx), the new/v1/intentendpoint lacks adescriptionfield. Adding one would maintain API documentation consistency and help clients understand the endpoint's purpose.internal/infrastructure/db/postgres/sqlc/query.sql (1)
472-477: Add blank line between query blocks for consistency.There's a missing newline separator between
UpdateTeleportAssetandSelectLatestIntentFees, which differs from the formatting pattern used elsewhere in the file.Proposed fix
-- name: UpdateTeleportAsset :exec UPDATE teleport_asset SET is_claimed = `@is_claimed` WHERE script = `@script` AND intent_id = `@intent_id` AND asset_id = `@asset_id` AND group_index = `@group_index`; + -- name: SelectLatestIntentFees :one
| // GetAsset returns the asset information and metadata for the specified asset ID. | ||
| rpc GetAssetGroup(GetAssetGroupRequest) returns (GetAssetGroupResponse) { | ||
| option (meshapi.gateway.http) = { | ||
| get: "/v1/indexer/assetGroup/{asset_id}" | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fix doc comment mismatch/typos in new RPCs.
The comment says “GetAsset” but the RPC is GetAssetGroup, and “subscribtion” is misspelled.
✏️ Suggested wording fixes
- // GetAsset returns the asset information and metadata for the specified asset ID.
+ // GetAssetGroup returns the asset information and metadata for the specified asset ID.
...
- // teleport hashes. It can also be used to update an existing subscribtion by adding new hashes to it.
+ // teleport hashes. It can also be used to update an existing subscription by adding new hashes to it.Also applies to: 110-112
🤖 Prompt for AI Agents
In `@api-spec/protobuf/ark/v1/indexer.proto` around lines 78 - 83, Update the
protobuf doc comments to match the RPC names and fix typos: change the comment
that incorrectly says "GetAsset" to "GetAssetGroup" and ensure it describes
returning asset group information for the specified asset ID (for rpc
GetAssetGroup and messages GetAssetGroupRequest/GetAssetGroupResponse), and
correct any misspelling of "subscribtion" to "subscription" in the other RPC doc
comments (the similar typo around the RPCs at the later block referenced by
lines 110-112).
| func (m *assetGroupValidationMachine) validateInput(s *service, input extension.AssetInput) error { | ||
| grpAsset := m.group() | ||
| if input.Type == extension.AssetTypeTeleport { | ||
| if grpAsset.AssetId == nil { | ||
| return errors.TELEPORT_VALIDATION_FAILED.New("asset ID is required for teleport input validation"). | ||
| WithMetadata(errors.TeleportValidationMetadata{}) | ||
| } | ||
|
|
||
| txHash, err := chainhash.NewHash(input.Witness.IntentId) | ||
| if err != nil { | ||
| return errors.TELEPORT_VALIDATION_FAILED.New("invalid intent ID for teleport input validation: %w", err). | ||
| WithMetadata(errors.TeleportValidationMetadata{}) | ||
| } | ||
|
|
||
| intent, err := s.repoManager.Rounds().GetIntentByTxid(context.Background(), txHash.String()) | ||
| if err != nil { | ||
| return errors.TELEPORT_VALIDATION_FAILED.New("error retrieving intent for teleport input validation: %w", err). | ||
| WithMetadata(errors.TeleportValidationMetadata{}) | ||
| } | ||
|
|
||
| decodedProof, err := psbt.NewFromRawBytes(strings.NewReader(intent.Proof), true) | ||
| if err != nil { | ||
| return errors.TELEPORT_VALIDATION_FAILED.New("error decoding intent proof for teleport input validation: %w", err). | ||
| WithMetadata(errors.TeleportValidationMetadata{}) | ||
| } | ||
|
|
||
| if err := s.validateTeleportInput(*decodedProof, m.arkTx, *grpAsset.AssetId, uint32(input.Vin), input.Witness.Script); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Guard against nil intent before decoding proof.
GetIntentByTxid can return a nil intent; dereferencing intent.Proof will panic.
🐛 Suggested fix
- intent, err := s.repoManager.Rounds().GetIntentByTxid(context.Background(), txHash.String())
+ intent, err := s.repoManager.Rounds().GetIntentByTxid(context.Background(), txHash.String())
if err != nil {
return errors.TELEPORT_VALIDATION_FAILED.New("error retrieving intent for teleport input validation: %w", err).
WithMetadata(errors.TeleportValidationMetadata{})
}
+ if intent == nil {
+ return errors.TELEPORT_VALIDATION_FAILED.New(
+ "intent %s not found for teleport input validation", txHash.String(),
+ ).WithMetadata(errors.TeleportValidationMetadata{})
+ }
decodedProof, err := psbt.NewFromRawBytes(strings.NewReader(intent.Proof), true)🤖 Prompt for AI Agents
In `@internal/core/application/asset_validation.go` around lines 339 - 369, The
validateInput branch for AssetTypeTeleport dereferences intent.Proof without
checking if intent is nil; update validateInput to check the return from
s.repoManager.Rounds().GetIntentByTxid (the intent variable) for nil before
using intent.Proof, and if nil return a TELEPORT_VALIDATION_FAILED error (using
errors.TELEPORT_VALIDATION_FAILED.New with a clear message like "intent not
found for teleport input validation" and
WithMetadata(errors.TeleportValidationMetadata{})); keep the existing error
handling for GetIntentByTxid failures and proceed to decode intent.Proof only
when intent is non-nil so decodedProof and subsequent calls
(s.validateTeleportInput) cannot panic.
| for j, v := range allVtxos { | ||
| // add asset to vtxo if present | ||
| asst, err := i.repoManager.Assets().GetAssetByOutpoint(ctx, v.Outpoint) | ||
| if err == nil && asst != nil { | ||
| assetExtension := domain.AssetExtension{ | ||
| AssetID: asst.AssetID, | ||
| Amount: 0, | ||
| } | ||
| allVtxos[j].Extensions = append(allVtxos[j].Extensions, assetExtension) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Amount is hardcoded to 0 instead of using the actual asset amount.
At line 280, Amount: 0 should be Amount: asst.Amount to match the behavior in GetVtxos and provide meaningful asset data.
Proposed fix
assetExtension := domain.AssetExtension{
AssetID: asst.AssetID,
- Amount: 0,
+ Amount: asst.Amount,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for j, v := range allVtxos { | |
| // add asset to vtxo if present | |
| asst, err := i.repoManager.Assets().GetAssetByOutpoint(ctx, v.Outpoint) | |
| if err == nil && asst != nil { | |
| assetExtension := domain.AssetExtension{ | |
| AssetID: asst.AssetID, | |
| Amount: 0, | |
| } | |
| allVtxos[j].Extensions = append(allVtxos[j].Extensions, assetExtension) | |
| } | |
| } | |
| for j, v := range allVtxos { | |
| // add asset to vtxo if present | |
| asst, err := i.repoManager.Assets().GetAssetByOutpoint(ctx, v.Outpoint) | |
| if err == nil && asst != nil { | |
| assetExtension := domain.AssetExtension{ | |
| AssetID: asst.AssetID, | |
| Amount: asst.Amount, | |
| } | |
| allVtxos[j].Extensions = append(allVtxos[j].Extensions, assetExtension) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/core/application/indexer.go` around lines 274 - 284, The loop over
allVtxos is appending a domain.AssetExtension with Amount incorrectly hardcoded
to 0; update the creation of assetExtension in the block where
i.repoManager.Assets().GetAssetByOutpoint(ctx, v.Outpoint) returns asst so that
AssetExtension.Amount is set to asst.Amount (use the actual asset amount) before
appending to allVtxos[j].Extensions; ensure you reference allVtxos,
i.repoManager.Assets().GetAssetByOutpoint, and domain.AssetExtension when making
the change.
| var ASSET_NOT_FOUND = Code[AssetValidationMetadata]{ | ||
| 34, | ||
| "ASSET_NOT_FOUND", | ||
| grpccodes.NotFound, | ||
| } | ||
|
|
||
| var ASSET_INPUT_INVALID = Code[AssetInputMetadata]{ | ||
| 35, | ||
| "ASSET_INPUT_INVALID", | ||
| grpccodes.InvalidArgument, | ||
| } | ||
|
|
||
| var ASSET_OUTPUT_INVALID = Code[AssetOutputMetadata]{ | ||
| 36, | ||
| "ASSET_OUTPUT_INVALID", | ||
| grpccodes.InvalidArgument, | ||
| } | ||
|
|
||
| var CONTROL_ASSET_INVALID = Code[ControlAssetMetadata]{ | ||
| 37, | ||
| "CONTROL_ASSET_INVALID", | ||
| grpccodes.InvalidArgument, | ||
| } | ||
|
|
||
| var CONTROL_ASSET_NOT_FOUND = Code[ControlAssetMetadata]{ | ||
| 38, | ||
| "CONTROL_ASSET_NOT_FOUND", | ||
| grpccodes.NotFound, | ||
| } | ||
|
|
||
| var TELEPORT_VALIDATION_FAILED = Code[TeleportValidationMetadata]{ | ||
| 39, | ||
| "TELEPORT_VALIDATION_FAILED", | ||
| grpccodes.InvalidArgument, | ||
| } | ||
|
|
||
| var CHECKPOINT_TX_INVALID = Code[CheckpointValidationMetadata]{ | ||
| 40, | ||
| "CHECKPOINT_TX_INVALID", | ||
| grpccodes.InvalidArgument, | ||
| } | ||
|
|
||
| var CHECKPOINT_TX_NOT_FOUND = Code[CheckpointValidationMetadata]{ | ||
| 41, | ||
| "CHECKPOINT_TX_NOT_FOUND", | ||
| grpccodes.NotFound, | ||
| } | ||
|
|
||
| var OFFCHAIN_TX_INVALID = Code[OffchainTxValidationMetadata]{ | ||
| 42, | ||
| "OFFCHAIN_TX_INVALID", | ||
| grpccodes.InvalidArgument, | ||
| } | ||
|
|
||
| var ASSET_PACKET_INVALID = Code[AssetValidationMetadata]{ | ||
| 43, | ||
| "ASSET_PACKET_INVALID", | ||
| grpccodes.InvalidArgument, | ||
| } | ||
|
|
||
| var INTENT_FEE_EVALUATION_FAILED = Code[any]{44, "INTENT_FEE_EVALUATION_FAILED", grpccodes.Internal} | ||
|
|
||
| var INTENT_NOT_FOUND = Code[any]{34, "INTENT_NOT_FOUND", grpccodes.NotFound} |
There was a problem hiding this comment.
Duplicate error code value (34) across ASSET_NOT_FOUND and INTENT_NOT_FOUND.
Error codes should be unique; duplicating 34 makes error classification ambiguous for clients. Assign a new unused code.
🔧 Proposed fix
-var INTENT_NOT_FOUND = Code[any]{34, "INTENT_NOT_FOUND", grpccodes.NotFound}
+var INTENT_NOT_FOUND = Code[any]{45, "INTENT_NOT_FOUND", grpccodes.NotFound}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var ASSET_NOT_FOUND = Code[AssetValidationMetadata]{ | |
| 34, | |
| "ASSET_NOT_FOUND", | |
| grpccodes.NotFound, | |
| } | |
| var ASSET_INPUT_INVALID = Code[AssetInputMetadata]{ | |
| 35, | |
| "ASSET_INPUT_INVALID", | |
| grpccodes.InvalidArgument, | |
| } | |
| var ASSET_OUTPUT_INVALID = Code[AssetOutputMetadata]{ | |
| 36, | |
| "ASSET_OUTPUT_INVALID", | |
| grpccodes.InvalidArgument, | |
| } | |
| var CONTROL_ASSET_INVALID = Code[ControlAssetMetadata]{ | |
| 37, | |
| "CONTROL_ASSET_INVALID", | |
| grpccodes.InvalidArgument, | |
| } | |
| var CONTROL_ASSET_NOT_FOUND = Code[ControlAssetMetadata]{ | |
| 38, | |
| "CONTROL_ASSET_NOT_FOUND", | |
| grpccodes.NotFound, | |
| } | |
| var TELEPORT_VALIDATION_FAILED = Code[TeleportValidationMetadata]{ | |
| 39, | |
| "TELEPORT_VALIDATION_FAILED", | |
| grpccodes.InvalidArgument, | |
| } | |
| var CHECKPOINT_TX_INVALID = Code[CheckpointValidationMetadata]{ | |
| 40, | |
| "CHECKPOINT_TX_INVALID", | |
| grpccodes.InvalidArgument, | |
| } | |
| var CHECKPOINT_TX_NOT_FOUND = Code[CheckpointValidationMetadata]{ | |
| 41, | |
| "CHECKPOINT_TX_NOT_FOUND", | |
| grpccodes.NotFound, | |
| } | |
| var OFFCHAIN_TX_INVALID = Code[OffchainTxValidationMetadata]{ | |
| 42, | |
| "OFFCHAIN_TX_INVALID", | |
| grpccodes.InvalidArgument, | |
| } | |
| var ASSET_PACKET_INVALID = Code[AssetValidationMetadata]{ | |
| 43, | |
| "ASSET_PACKET_INVALID", | |
| grpccodes.InvalidArgument, | |
| } | |
| var INTENT_FEE_EVALUATION_FAILED = Code[any]{44, "INTENT_FEE_EVALUATION_FAILED", grpccodes.Internal} | |
| var INTENT_NOT_FOUND = Code[any]{34, "INTENT_NOT_FOUND", grpccodes.NotFound} | |
| var ASSET_NOT_FOUND = Code[AssetValidationMetadata]{ | |
| 34, | |
| "ASSET_NOT_FOUND", | |
| grpccodes.NotFound, | |
| } | |
| var ASSET_INPUT_INVALID = Code[AssetInputMetadata]{ | |
| 35, | |
| "ASSET_INPUT_INVALID", | |
| grpccodes.InvalidArgument, | |
| } | |
| var ASSET_OUTPUT_INVALID = Code[AssetOutputMetadata]{ | |
| 36, | |
| "ASSET_OUTPUT_INVALID", | |
| grpccodes.InvalidArgument, | |
| } | |
| var CONTROL_ASSET_INVALID = Code[ControlAssetMetadata]{ | |
| 37, | |
| "CONTROL_ASSET_INVALID", | |
| grpccodes.InvalidArgument, | |
| } | |
| var CONTROL_ASSET_NOT_FOUND = Code[ControlAssetMetadata]{ | |
| 38, | |
| "CONTROL_ASSET_NOT_FOUND", | |
| grpccodes.NotFound, | |
| } | |
| var TELEPORT_VALIDATION_FAILED = Code[TeleportValidationMetadata]{ | |
| 39, | |
| "TELEPORT_VALIDATION_FAILED", | |
| grpccodes.InvalidArgument, | |
| } | |
| var CHECKPOINT_TX_INVALID = Code[CheckpointValidationMetadata]{ | |
| 40, | |
| "CHECKPOINT_TX_INVALID", | |
| grpccodes.InvalidArgument, | |
| } | |
| var CHECKPOINT_TX_NOT_FOUND = Code[CheckpointValidationMetadata]{ | |
| 41, | |
| "CHECKPOINT_TX_NOT_FOUND", | |
| grpccodes.NotFound, | |
| } | |
| var OFFCHAIN_TX_INVALID = Code[OffchainTxValidationMetadata]{ | |
| 42, | |
| "OFFCHAIN_TX_INVALID", | |
| grpccodes.InvalidArgument, | |
| } | |
| var ASSET_PACKET_INVALID = Code[AssetValidationMetadata]{ | |
| 43, | |
| "ASSET_PACKET_INVALID", | |
| grpccodes.InvalidArgument, | |
| } | |
| var INTENT_FEE_EVALUATION_FAILED = Code[any]{44, "INTENT_FEE_EVALUATION_FAILED", grpccodes.Internal} | |
| var INTENT_NOT_FOUND = Code[any]{45, "INTENT_NOT_FOUND", grpccodes.NotFound} |
🤖 Prompt for AI Agents
In `@pkg/errors/errors.go` around lines 381 - 443, ASSET_NOT_FOUND and
INTENT_NOT_FOUND currently share the same numeric code (34); change
INTENT_NOT_FOUND's numeric ID to an unused value (e.g., 45) in its Code literal
so all error codes are unique. Update the INTENT_NOT_FOUND declaration (the
Code[any] composite literal) to use the new integer while keeping its name and
grpc code intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/core/application/service.go`:
- Around line 1655-1661: The code appends every non-asset output to
offchainOutputs causing on-chain outputs to be miscounted; update the logic so
offchainOutputs is only appended inside the off-chain branch where rcv.PubKey is
set (i.e., when hasOffChainReceiver is toggled and rcv.PubKey =
hex.EncodeToString(output.PkScript[2:])); leave receivers = append(receivers,
rcv) unchanged but move offchainOutputs = append(offchainOutputs, *output) into
that same off-chain branch so only true off-chain outputs affect fee
calculations.
♻️ Duplicate comments (5)
internal/core/application/service.go (5)
1712-1717: Remove unreachable err check.
Line 1713 checkserrfrom a prior call that already returned on error; it’s always nil here.
1131-1138: Make asset persistence semantics explicit.
Line 1132 logs and continues whenstoreAssetDetailsFromArkTxfails, which can desync asset state from an accepted offchain tx. Decide whether this should fail submission or be explicitly best‑effort (warn + comment/metric).
1928-1960: Teleport asset insert should be atomic and use the explicit vout.
Line 1951 uses the loop index and errors are only logged, so registration can succeed with missing teleport assets. Useoutput.Voutand fail the registration on insert failure.🛠️ Proposed fix
- if err := s.repoManager.Assets().InsertTeleportAsset(ctx, domain.TeleportAsset{ + if err := s.repoManager.Assets().InsertTeleportAsset(ctx, domain.TeleportAsset{ IntentID: proofTxid, Script: teleportScript, - OutputIndex: uint32(i), + OutputIndex: uint32(output.Vout), AssetID: asst.AssetId.ToString(), Amount: output.Amount, IsClaimed: false, }); err != nil { - log.WithError(err).Warn("failed to insert teleport asset") + return "", errors.INTERNAL_ERROR.New("failed to insert teleport asset: %w", err). + WithMetadata(map[string]any{ + "intent_id": proofTxid, + "asset_id": asst.AssetId.ToString(), + "vout": output.Vout, + }) }
4488-4505: Surface insufficient balance on quantity decreases.
IfDecreaseAssetGroupQuantityno‑ops (e.g.,WHERE quantity >= delta) and returns no rows, this can silently succeed. Return a deterministic insufficient‑balance error whentotalIn < totalOutor when the repo indicates no rows updated.
4537-4538: UseassetOut.VoutforOutputVout.
The loop index can drift from the encoded vout.🛠️ Proposed fix
- OutputVout: uint32(outIdx), + OutputVout: uint32(assetOut.Vout),
| hasOffChainReceiver = true | ||
| rcv.PubKey = hex.EncodeToString(output.PkScript[2:]) | ||
| } | ||
|
|
||
| receivers = append(receivers, rcv) | ||
| offchainOutputs = append(offchainOutputs, *output) | ||
| } |
There was a problem hiding this comment.
On‑chain outputs are being counted as off‑chain outputs.
Line 1660 appends every non‑asset output to offchainOutputs, even when it’s on‑chain, which will skew fee calculation. Move the append into the off‑chain branch.
🛠️ Proposed fix
} else {
...
hasOffChainReceiver = true
rcv.PubKey = hex.EncodeToString(output.PkScript[2:])
+ offchainOutputs = append(offchainOutputs, *output)
}
receivers = append(receivers, rcv)
- offchainOutputs = append(offchainOutputs, *output)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| hasOffChainReceiver = true | |
| rcv.PubKey = hex.EncodeToString(output.PkScript[2:]) | |
| } | |
| receivers = append(receivers, rcv) | |
| offchainOutputs = append(offchainOutputs, *output) | |
| } | |
| hasOffChainReceiver = true | |
| rcv.PubKey = hex.EncodeToString(output.PkScript[2:]) | |
| offchainOutputs = append(offchainOutputs, *output) | |
| } | |
| receivers = append(receivers, rcv) | |
| } |
🤖 Prompt for AI Agents
In `@internal/core/application/service.go` around lines 1655 - 1661, The code
appends every non-asset output to offchainOutputs causing on-chain outputs to be
miscounted; update the logic so offchainOutputs is only appended inside the
off-chain branch where rcv.PubKey is set (i.e., when hasOffChainReceiver is
toggled and rcv.PubKey = hex.EncodeToString(output.PkScript[2:])); leave
receivers = append(receivers, rcv) unchanged but move offchainOutputs =
append(offchainOutputs, *output) into that same off-chain branch so only true
off-chain outputs affect fee calculations.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/core/application/service.go`:
- Around line 4474-4495: The current updateAssetQuantity flow can return success
when DecreaseAssetGroupQuantity silently affects 0 rows in SQLite/Postgres;
modify the fix so updateAssetQuantity validates and errors on insufficient
balance or ensure the SQL backends report zero-row updates as errors: either (A)
before calling repoManager.Assets().DecreaseAssetGroupQuantity in
updateAssetQuantity, fetch the current quantity and compare (e.g., via a
Get/GetAssetGroupQuantity method) and return an explicit insufficient-balance
error if totalIn < totalOut, or (B) change the SQLite/Postgres
DecreaseAssetGroupQuantity implementations to check rowsAffected after the
UPDATE (UPDATE ... WHERE quantity >= ...) and return a descriptive error when
rowsAffected == 0 so updateAssetQuantity receives an error instead of silently
succeeding; apply the same approach for consistency with the Badger
implementation and keep IncreaseAssetGroupQuantity behavior unchanged.
♻️ Duplicate comments (5)
internal/core/application/service.go (5)
4510-4531:OutputVoutshould useassetOut.Voutinstead of loop index.Line 4523 uses the loop index
outIdxforOutputVout, but this should use the actual vout fromassetOut.Voutto ensure consistency with the asset packet encoding.🐛 Proposed fix
- event := TeleportAsset{ - TeleportHash: hex.EncodeToString(assetOut.Script), - AnchorOutpoint: anchorOutpoint, - AssetID: ast.AssetId.ToString(), - OutputVout: uint32(outIdx), - CreatedAt: createdAt, - ExpiresAt: expiresAt, - } + event := TeleportAsset{ + TeleportHash: hex.EncodeToString(assetOut.Script), + AnchorOutpoint: anchorOutpoint, + AssetID: ast.AssetId.ToString(), + OutputVout: assetOut.Vout, + CreatedAt: createdAt, + ExpiresAt: expiresAt, + }
1578-1587: Teleport asset insertion errors should fail registration.
InsertTeleportAsseterrors are logged but the receiver is still added. This creates inconsistent state where the intent references a teleport asset that wasn't persisted.🐛 Proposed fix
if err := s.repoManager.Assets().InsertTeleportAsset(ctx, domain.TeleportAsset{ IntentID: proofTxid, Script: teleportScript, OutputIndex: uint32(i), AssetID: asst.AssetId.ToString(), Amount: output.Amount, IsClaimed: false, }); err != nil { - log.WithError(err).Warn("failed to insert teleport asset") + return "", errors.INTERNAL_ERROR.New( + "failed to insert teleport asset: %w", err, + ).WithMetadata(map[string]any{ + "intent_id": proofTxid, + "asset_id": asst.AssetId.ToString(), + }) }
1742-1747: Remove dead code block.This error check references
errfrom thes.cache.OffchainTxs().Includes()call (line 1726), which was already handled at lines 1727-1731. At this point,errcan only benil, making this block unreachable.🐛 Proposed fix
taptreeFields, _ := txutils.GetArkPsbtFields( &proof.Packet, i+1, txutils.VtxoTaprootTreeField, ) - - if err != nil { - return "", errors.INVALID_PSBT_INPUT.New( - "failed to get asset seal field for input %d: %w", i+1, err, - ).WithMetadata(errors.InputMetadata{Txid: proofTxid, InputIndex: i + 1}) - } tapscripts := make([]string, 0)
1685-1691: On-chain outputs are incorrectly counted as off-chain outputs.Line 1690 appends every non-asset output to
offchainOutputs, including on-chain outputs. This will skew fee calculation. The append should only occur inside the off-chain branch.🐛 Proposed fix
} else { hasOffChainReceiver = true rcv.PubKey = hex.EncodeToString(output.PkScript[2:]) + offchainOutputs = append(offchainOutputs, *output) } receivers = append(receivers, rcv) - offchainOutputs = append(offchainOutputs, *output) }
1131-1139: Clarify error handling semantics for asset storage.Asset storage errors are logged but don't fail the transaction, which could create inconsistent state where the offchain tx succeeds but asset state is not persisted. If best-effort is intentional, add a comment explaining why and consider emitting a metric for monitoring. Otherwise, return the error to fail the transaction.
🛠️ Suggested approaches
Option 1: Make it blocking
if assetOutputIndex >= 0 { if err := s.storeAssetDetailsFromArkTx( ctx, *arkPtx.UnsignedTx, assetOutputIndex, ); err != nil { - log.WithError(err).Errorf( - "failed to store asset details for offchain tx %s", txid, - ) + return nil, errors.INTERNAL_ERROR.New( + "failed to store asset details for offchain tx %s: %w", txid, err, + ) } }Option 2: Document best-effort behavior
if assetOutputIndex >= 0 { + // Best-effort: asset state can be reconstructed from the tx if storage fails. + // Log error for monitoring but don't fail the transaction. if err := s.storeAssetDetailsFromArkTx( ctx, *arkPtx.UnsignedTx, assetOutputIndex, ); err != nil { - log.WithError(err).Errorf( + log.WithError(err).Warnf( "failed to store asset details for offchain tx %s", txid, ) } }
🧹 Nitpick comments (4)
internal/core/application/service.go (4)
355-358: Remove or re-enable commented-out code.The
restoreWatchingVtxoscall is commented out without explanation. This could affect vtxo watching restoration on service restart. Either remove this dead code or re-enable it with a comment explaining the rationale for the change.
4320-4329: Variable shadowing creates potential confusion.Line 4326 declares a new
assetIdvariable that shadows the outer scope'sassetId(which isnilin the issuance case). While the logic is correct, this shadowing can cause confusion during maintenance.Consider using a different variable name like
newAssetIdfor clarity.♻️ Proposed refactor
if assetId == nil { var controlAsset string txHash := arkTx.TxHash() var txHashBytes [32]byte copy(txHashBytes[:], txHash[:]) - assetId := extension.AssetId{ + newAssetId := extension.AssetId{ TxHash: txHashBytes, Index: uint16(i), } // ... update usages of assetId to newAssetId in this block
3803-3806: Clarify comment to match the actual condition.The comment says "skip OP_RETURN outputs" but the condition checks if
vtxo.Amount < dustLimit. Consider updating the comment to be more precise.📝 Suggested comment update
- // skip OP_RETURN outputs + // skip subdust outputs (including OP_RETURN-style vtxos) if vtxo.Amount < dustLimit { continue }
4442-4461: Consider consistent error handling for teleport input claims.
UpdateTeleportAsseterrors are logged but not propagated, similar to other asset storage operations. Ensure this is consistent with the intended error handling semantics discussed earlier.
| func (s *service) updateAssetQuantity( | ||
| ctx context.Context, | ||
| assetID string, | ||
| totalIn, totalOut uint64, | ||
| ) error { | ||
| if totalOut > totalIn { | ||
| delta := totalOut - totalIn | ||
| if err := s.repoManager.Assets().IncreaseAssetGroupQuantity(ctx, assetID, delta); err != nil { | ||
| return fmt.Errorf("error updating asset quantity: %s", err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| if totalIn > totalOut { | ||
| delta := totalIn - totalOut | ||
| if err := s.repoManager.Assets().DecreaseAssetGroupQuantity(ctx, assetID, delta); err != nil { | ||
| return fmt.Errorf("error updating asset quantity: %s", err) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "DecreaseAssetGroupQuantity" --type go -A 10Repository: arkade-os/arkd
Length of output: 5259
🏁 Script executed:
sed -n '426,460p' internal/infrastructure/db/badger/asset_repo.goRepository: arkade-os/arkd
Length of output: 876
🏁 Script executed:
rg -n "SubtractFromAssetQuantity" --type go -A 8 | head -50Repository: arkade-os/arkd
Length of output: 4226
Silent no-op vulnerability in SQLite and Postgres implementations of DecreaseAssetGroupQuantity.
The SQLite and Postgres implementations use UPDATE ... WHERE quantity >= amount, which silently succeeds (no error) when the balance is insufficient and zero rows are affected. This allows updateAssetQuantity to return success even when the quantity decrease failed.
The Badger implementation correctly validates and returns an error when balance is insufficient. For consistency across all backends, the application should either:
- Add explicit balance validation in
updateAssetQuantitybefore callingDecreaseAssetGroupQuantity, or - Modify the SQLite/Postgres implementations to detect and report when the UPDATE affected zero rows.
Affected SQL queries
SQLite (internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go:2038-2039):
UPDATE asset_group
SET quantity = quantity - ?
WHERE id = ? AND quantity >= ?Postgres (internal/infrastructure/db/postgres/sqlc/queries/query.sql.go:1971-1972):
UPDATE asset_group
SET quantity = quantity - $1
WHERE id = $2 AND quantity >= $1Both return no error if the WHERE condition fails (0 rows affected), unlike the Badger implementation which explicitly checks and errors.
🤖 Prompt for AI Agents
In `@internal/core/application/service.go` around lines 4474 - 4495, The current
updateAssetQuantity flow can return success when DecreaseAssetGroupQuantity
silently affects 0 rows in SQLite/Postgres; modify the fix so
updateAssetQuantity validates and errors on insufficient balance or ensure the
SQL backends report zero-row updates as errors: either (A) before calling
repoManager.Assets().DecreaseAssetGroupQuantity in updateAssetQuantity, fetch
the current quantity and compare (e.g., via a Get/GetAssetGroupQuantity method)
and return an explicit insufficient-balance error if totalIn < totalOut, or (B)
change the SQLite/Postgres DecreaseAssetGroupQuantity implementations to check
rowsAffected after the UPDATE (UPDATE ... WHERE quantity >= ...) and return a
descriptive error when rowsAffected == 0 so updateAssetQuantity receives an
error instead of silently succeeding; apply the same approach for consistency
with the Badger implementation and keep IncreaseAssetGroupQuantity behavior
unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/infrastructure/db/postgres/sqlc/queries/query.sql.go`:
- Around line 90-104: The AddToAssetQuantity SQL currently executes silently
when no row matches; update the SQL tag to use :execrows (change the query to
`-- name: AddToAssetQuantity :execrows`) and regenerate sqlc so the generated
AddToAssetQuantity method returns the rows affected (e.g., (int64, error) or
(sql.Result, error) depending on your generator). Then adjust the implementation
of AddToAssetQuantity to return the number of rows affected alongside the error
so callers can detect a missing asset group (align behavior with
SubtractFromAssetQuantity) and update any call sites to handle a 0-rows-affected
result.
- Around line 1994-2017: The UpdateTeleportAsset SQL currently uses :exec
(updateTeleportAsset) so UpdateTeleportAsset(ctx, arg UpdateTeleportAssetParams)
returns nil even when no row matched; change the SQL comment to use :execrows
(so sqlc generates a result-aware method) and update the generated Go method
(UpdateTeleportAsset) to return rowsAffected (or an error) by checking the Exec
result's RowsAffected from q.db.ExecContext (or the sqlc-generated return) and
return a specific error when rowsAffected == 0 to surface non-existent teleport
assets.
♻️ Duplicate comments (5)
internal/infrastructure/db/postgres/sqlc/queries/query.sql.go (2)
16-39: Silent no-op on conflicting asset IDs already flagged.The previous review identified that this
ON CONFLICT ... WHEREpattern can silently succeed with zero rows affected whenasset.asset_id != EXCLUDED.asset_id. Please address the earlier feedback to return rows affected or enforce a stricter constraint.
1969-1983: Insufficient-quantity/missing-group failure already flagged.The previous review identified that this
quantity >= $1guard can yield zero updated rows with no error, letting callers assume subtraction succeeded. Please address the earlier feedback to return rows affected.internal/infrastructure/db/sqlite/sqlc/queries/query.sql.go (1)
2036-2051: Regenerate after switching to:execrows.Once
SubtractFromAssetQuantityis changed inquery.sql, re-run sqlc so this method returns rows affected.internal/infrastructure/db/sqlite/sqlc/query.sql (1)
460-463: Expose rows affected for guarded subtraction.
quantity >= ?is a guard; with:execthe caller can’t tell whether subtraction happened. Use:execrowsand treat 0 rows as “insufficient quantity,” then regenerate sqlc outputs.🛠️ Suggested SQL change
--- name: SubtractFromAssetQuantity :exec +-- name: SubtractFromAssetQuantity :execrows UPDATE asset_group SET quantity = quantity - ? WHERE id = ? AND quantity >= ?;Suggested verification (ensure callers handle rows affected after regen):
#!/bin/bash rg -n "SubtractFromAssetQuantity" --type go -C 3internal/infrastructure/db/postgres/sqlc/query.sql (1)
454-457: Expose rows affected for guarded subtraction.This guarded update can silently no‑op; switch to
:execrowsand have the repo treat 0 rows as insufficient quantity.🛠️ Suggested SQL change
--- name: SubtractFromAssetQuantity :exec +-- name: SubtractFromAssetQuantity :execrows UPDATE asset_group SET quantity = quantity - `@quantity` WHERE id = `@id` AND quantity >= `@quantity`;
| const addToAssetQuantity = `-- name: AddToAssetQuantity :exec | ||
| UPDATE asset_group | ||
| SET quantity = quantity + $1 | ||
| WHERE id = $2 | ||
| ` | ||
|
|
||
| type AddToAssetQuantityParams struct { | ||
| Quantity int64 | ||
| ID string | ||
| } | ||
|
|
||
| func (q *Queries) AddToAssetQuantity(ctx context.Context, arg AddToAssetQuantityParams) error { | ||
| _, err := q.db.ExecContext(ctx, addToAssetQuantity, arg.Quantity, arg.ID) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Silent no-op when asset group doesn't exist.
Similar to SubtractFromAssetQuantity, this function returns nil even when the id doesn't match any row. Callers cannot distinguish a successful quantity increase from an operation on a non-existent asset group.
Consider changing to :execrows and returning rows affected so callers can detect and handle missing asset groups.
🔧 Proposed fix (update the sqlc query definition and regenerate)
-const addToAssetQuantity = `-- name: AddToAssetQuantity :exec
+const addToAssetQuantity = `-- name: AddToAssetQuantity :execrows
UPDATE asset_group
SET quantity = quantity + $1
WHERE id = $2
`
@@
-func (q *Queries) AddToAssetQuantity(ctx context.Context, arg AddToAssetQuantityParams) error {
- _, err := q.db.ExecContext(ctx, addToAssetQuantity, arg.Quantity, arg.ID)
- return err
+func (q *Queries) AddToAssetQuantity(ctx context.Context, arg AddToAssetQuantityParams) (int64, error) {
+ result, err := q.db.ExecContext(ctx, addToAssetQuantity, arg.Quantity, arg.ID)
+ if err != nil {
+ return 0, err
+ }
+ return result.RowsAffected()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const addToAssetQuantity = `-- name: AddToAssetQuantity :exec | |
| UPDATE asset_group | |
| SET quantity = quantity + $1 | |
| WHERE id = $2 | |
| ` | |
| type AddToAssetQuantityParams struct { | |
| Quantity int64 | |
| ID string | |
| } | |
| func (q *Queries) AddToAssetQuantity(ctx context.Context, arg AddToAssetQuantityParams) error { | |
| _, err := q.db.ExecContext(ctx, addToAssetQuantity, arg.Quantity, arg.ID) | |
| return err | |
| } | |
| const addToAssetQuantity = `-- name: AddToAssetQuantity :execrows | |
| UPDATE asset_group | |
| SET quantity = quantity + $1 | |
| WHERE id = $2 | |
| ` | |
| type AddToAssetQuantityParams struct { | |
| Quantity int64 | |
| ID string | |
| } | |
| func (q *Queries) AddToAssetQuantity(ctx context.Context, arg AddToAssetQuantityParams) (int64, error) { | |
| result, err := q.db.ExecContext(ctx, addToAssetQuantity, arg.Quantity, arg.ID) | |
| if err != nil { | |
| return 0, err | |
| } | |
| return result.RowsAffected() | |
| } |
🤖 Prompt for AI Agents
In `@internal/infrastructure/db/postgres/sqlc/queries/query.sql.go` around lines
90 - 104, The AddToAssetQuantity SQL currently executes silently when no row
matches; update the SQL tag to use :execrows (change the query to `-- name:
AddToAssetQuantity :execrows`) and regenerate sqlc so the generated
AddToAssetQuantity method returns the rows affected (e.g., (int64, error) or
(sql.Result, error) depending on your generator). Then adjust the implementation
of AddToAssetQuantity to return the number of rows affected alongside the error
so callers can detect a missing asset group (align behavior with
SubtractFromAssetQuantity) and update any call sites to handle a 0-rows-affected
result.
| const updateTeleportAsset = `-- name: UpdateTeleportAsset :exec | ||
| UPDATE teleport_asset | ||
| SET is_claimed = $1 | ||
| WHERE script = $2 AND intent_id = $3 AND asset_id = $4 AND group_index = $5 | ||
| ` | ||
|
|
||
| type UpdateTeleportAssetParams struct { | ||
| IsClaimed bool | ||
| Script string | ||
| IntentID string | ||
| AssetID string | ||
| GroupIndex int64 | ||
| } | ||
|
|
||
| func (q *Queries) UpdateTeleportAsset(ctx context.Context, arg UpdateTeleportAssetParams) error { | ||
| _, err := q.db.ExecContext(ctx, updateTeleportAsset, | ||
| arg.IsClaimed, | ||
| arg.Script, | ||
| arg.IntentID, | ||
| arg.AssetID, | ||
| arg.GroupIndex, | ||
| ) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Silent no-op when teleport asset doesn't exist.
If no row matches the composite key (script, intent_id, asset_id, group_index), this function returns nil, making it impossible for callers to distinguish a successful claim update from an operation on a non-existent teleport asset.
Consider changing to :execrows and returning rows affected.
🔧 Proposed fix (update the sqlc query definition and regenerate)
-const updateTeleportAsset = `-- name: UpdateTeleportAsset :exec
+const updateTeleportAsset = `-- name: UpdateTeleportAsset :execrows
UPDATE teleport_asset
SET is_claimed = $1
WHERE script = $2 AND intent_id = $3 AND asset_id = $4 AND group_index = $5
`
@@
-func (q *Queries) UpdateTeleportAsset(ctx context.Context, arg UpdateTeleportAssetParams) error {
- _, err := q.db.ExecContext(ctx, updateTeleportAsset,
+func (q *Queries) UpdateTeleportAsset(ctx context.Context, arg UpdateTeleportAssetParams) (int64, error) {
+ result, err := q.db.ExecContext(ctx, updateTeleportAsset,
arg.IsClaimed,
arg.Script,
arg.IntentID,
arg.AssetID,
arg.GroupIndex,
)
- return err
+ if err != nil {
+ return 0, err
+ }
+ return result.RowsAffected()
}🤖 Prompt for AI Agents
In `@internal/infrastructure/db/postgres/sqlc/queries/query.sql.go` around lines
1994 - 2017, The UpdateTeleportAsset SQL currently uses :exec
(updateTeleportAsset) so UpdateTeleportAsset(ctx, arg UpdateTeleportAssetParams)
returns nil even when no row matched; change the SQL comment to use :execrows
(so sqlc generates a result-aware method) and update the generated Go method
(UpdateTeleportAsset) to return rowsAffected (or an error) by checking the Exec
result's RowsAffected from q.db.ExecContext (or the sqlc-generated return) and
return a specific error when rowsAffected == 0 to surface non-existent teleport
assets.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@internal/core/application/service.go`:
- Around line 4423-4433: The loop over asstGp.Outputs that appends
domain.NormalAsset entries (creating asst with Outpoint/AssetID/Amount and
appending to assetList) is currently including teleport outputs; update that
loop to skip outputs that are teleports using the same predicate used earlier
for issuance (i.e., the teleport-detection check used when building issuance
assetList). Concretely, inside the for _, out := range asstGp.Outputs loop, add
the same conditional (e.g., if isTeleport(out) or the existing out teleport
check used above) to continue/skip teleport outputs before constructing the
domain.NormalAsset so assetList only receives non-teleport outputs.
- Around line 355-358: The commented-out call to svc.restoreWatchingVtxos in
service.go should be either reinstated or removed with justification; restore it
by uncommenting and handling its error (e.g., return fmt.Errorf on failure)
before starting go svc.listenToScannerNotifications() if vtxo watching is
required, or delete the commented lines and add a brief code comment explaining
why restoreWatchingVtxos is intentionally disabled so future readers know this
is deliberate rather than accidental.
- Around line 3242-3243: Remove the stray debug print fmt.Printf("events %+v",
changes): either delete this line or replace it with the project’s structured
logger call (do not use fmt.Printf). Locate the occurrence of fmt.Printf and the
variable changes in internal/core/application/service.go and swap it for the
appropriate logger method used throughout the service (e.g., logger.Debug/Infof
with context) or simply remove the statement before merging.
- Around line 2877-2878: Remove the leftover debug fmt.Printf("events %+v",
round.Events()) call: either delete the line or replace it with a trace-level
log using the project's logger (e.g. use logrus.WithField("events",
round.Events()).Trace(...) or logger.Tracef(..., round.Events())) so that event
data is logged via the project's logging framework rather than printed to
stdout.
♻️ Duplicate comments (7)
internal/core/application/service.go (7)
1131-1139: Clarify error handling semantics for asset storage.This block logs the error but doesn't fail the transaction. As noted in previous review, this creates potential inconsistency where the offchain transaction succeeds but asset state is not persisted.
Consider either:
- Make asset storage blocking (return error)
- Add a comment explaining why best-effort is acceptable here
1576-1593: Teleport asset insertion errors should fail registration.When
InsertTeleportAssetfails, the error is only logged but the receiver is still added to the intent. This creates inconsistent state where the intent references a teleport asset that wasn't persisted.if err := s.repoManager.Assets().InsertTeleportAsset(ctx, domain.TeleportAsset{ IntentID: proofTxid, Script: teleportScript, OutputIndex: uint32(i), AssetID: asst.AssetId.ToString(), Amount: output.Amount, IsClaimed: false, }); err != nil { - log.WithError(err).Warn("failed to insert teleport asset") + return "", errors.INTERNAL_ERROR.New( + "failed to insert teleport asset: %w", err, + ).WithMetadata(map[string]any{ + "intent_id": proofTxid, + "asset_id": asst.AssetId.ToString(), + }) }
1578-1581: Useoutput.Voutinstead of loop index for OutputIndex.The loop index
imay not match the actual vout encoded in the asset output. Useoutput.Voutfor consistency with the asset packet encoding.if err := s.repoManager.Assets().InsertTeleportAsset(ctx, domain.TeleportAsset{ IntentID: proofTxid, Script: teleportScript, - OutputIndex: uint32(i), + OutputIndex: output.Vout, AssetID: asst.AssetId.ToString(),
1685-1691: On-chain outputs incorrectly appended to offchainOutputs.Line 1690 appends to
offchainOutputsunconditionally after the if-else block, meaning on-chain outputs are also added. This will skew fee calculations.🐛 Proposed fix
} else { ... hasOffChainReceiver = true rcv.PubKey = hex.EncodeToString(output.PkScript[2:]) + offchainOutputs = append(offchainOutputs, *output) } receivers = append(receivers, rcv) - offchainOutputs = append(offchainOutputs, *output)
4478-4499: Silent failure vulnerability inupdateAssetQuantity.As noted in a previous review, the SQLite and Postgres implementations of
DecreaseAssetGroupQuantityuseUPDATE ... WHERE quantity >= amount, which silently succeeds when balance is insufficient (0 rows affected). This allowsupdateAssetQuantityto return success even when the decrease failed.Consider adding explicit validation before calling
DecreaseAssetGroupQuantity, or modifying the SQL implementations to checkrowsAffected.
4523-4534:OutputVoutshould useassetOut.Voutinstead of loop index.The
OutputVoutis set tooutIdx(loop index) but should useassetOut.Voutto reflect the actual output index from the asset packet.🐛 Proposed fix
event := TeleportAsset{ TeleportHash: hex.EncodeToString(assetOut.Script), AnchorOutpoint: anchorOutpoint, AssetID: ast.AssetId.ToString(), - OutputVout: uint32(outIdx), + OutputVout: assetOut.Vout, CreatedAt: createdAt, ExpiresAt: expiresAt, }
1742-1747: Dead code:errwas already handled above.This block appears to be unreachable. The
errvariable from theGetArkPsbtFieldscall on line 1739 is discarded with_, making this error check pointless. This was flagged in a previous review as addressed, but the code remains.- if err != nil { - return "", errors.INVALID_PSBT_INPUT.New( - "failed to get asset seal field for input %d: %w", i+1, err, - ).WithMetadata(errors.InputMetadata{Txid: proofTxid, InputIndex: i + 1}) - }
🧹 Nitpick comments (1)
internal/core/application/service.go (1)
3807-3810: Clarify comment to match the actual logic.The comment says "skip OP_RETURN outputs" but the condition actually skips all sub-dust outputs. Consider updating the comment for accuracy:
- // skip OP_RETURN outputs + // skip sub-dust outputs (including OP_RETURN-style vtxo scripts) if vtxo.Amount < dustLimit { continue }
| // if err := svc.restoreWatchingVtxos(); err != nil { | ||
| // return nil, fmt.Errorf("failed to restore watching vtxos: %s", err) | ||
| // } | ||
| go svc.listenToScannerNotifications() |
There was a problem hiding this comment.
Remove or restore commented-out code.
This commented-out call to restoreWatchingVtxos may cause vtxos to not be watched after service restart, potentially missing fraud detection events. Either:
- Restore the call if watching is required
- Remove the commented code entirely if it's intentionally disabled, with a code comment explaining why
- // if err := svc.restoreWatchingVtxos(); err != nil {
- // return nil, fmt.Errorf("failed to restore watching vtxos: %s", err)
- // }
+ if err := svc.restoreWatchingVtxos(); err != nil {
+ return nil, fmt.Errorf("failed to restore watching vtxos: %s", err)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // if err := svc.restoreWatchingVtxos(); err != nil { | |
| // return nil, fmt.Errorf("failed to restore watching vtxos: %s", err) | |
| // } | |
| go svc.listenToScannerNotifications() | |
| if err := svc.restoreWatchingVtxos(); err != nil { | |
| return nil, fmt.Errorf("failed to restore watching vtxos: %s", err) | |
| } | |
| go svc.listenToScannerNotifications() |
🤖 Prompt for AI Agents
In `@internal/core/application/service.go` around lines 355 - 358, The
commented-out call to svc.restoreWatchingVtxos in service.go should be either
reinstated or removed with justification; restore it by uncommenting and
handling its error (e.g., return fmt.Errorf on failure) before starting go
svc.listenToScannerNotifications() if vtxo watching is required, or delete the
commented lines and add a brief code comment explaining why restoreWatchingVtxos
is intentionally disabled so future readers know this is deliberate rather than
accidental.
| for _, out := range asstGp.Outputs { | ||
| asst := domain.NormalAsset{ | ||
| Outpoint: domain.Outpoint{ | ||
| Txid: arkTx.TxID(), | ||
| VOut: uint32(out.Vout), | ||
| }, | ||
| AssetID: assetId.ToString(), | ||
| Amount: out.Amount, | ||
| } | ||
| assetList = append(assetList, asst) | ||
| } |
There was a problem hiding this comment.
Teleport outputs should be excluded from assetList for existing assets.
For issuance (lines 4363-4366), teleport outputs are correctly skipped when building the asset list. However, for existing assets, all outputs are added including teleport outputs. This inconsistency may cause teleport outputs to be incorrectly stored as normal assets.
🐛 Proposed fix
for _, out := range asstGp.Outputs {
+ if out.Type == extension.AssetTypeTeleport {
+ continue
+ }
+
asst := domain.NormalAsset{
Outpoint: domain.Outpoint{
Txid: arkTx.TxID(),
VOut: uint32(out.Vout),
},
AssetID: assetId.ToString(),
Amount: out.Amount,
}
assetList = append(assetList, asst)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, out := range asstGp.Outputs { | |
| asst := domain.NormalAsset{ | |
| Outpoint: domain.Outpoint{ | |
| Txid: arkTx.TxID(), | |
| VOut: uint32(out.Vout), | |
| }, | |
| AssetID: assetId.ToString(), | |
| Amount: out.Amount, | |
| } | |
| assetList = append(assetList, asst) | |
| } | |
| for _, out := range asstGp.Outputs { | |
| if out.Type == extension.AssetTypeTeleport { | |
| continue | |
| } | |
| asst := domain.NormalAsset{ | |
| Outpoint: domain.Outpoint{ | |
| Txid: arkTx.TxID(), | |
| VOut: uint32(out.Vout), | |
| }, | |
| AssetID: assetId.ToString(), | |
| Amount: out.Amount, | |
| } | |
| assetList = append(assetList, asst) | |
| } |
🤖 Prompt for AI Agents
In `@internal/core/application/service.go` around lines 4423 - 4433, The loop over
asstGp.Outputs that appends domain.NormalAsset entries (creating asst with
Outpoint/AssetID/Amount and appending to assetList) is currently including
teleport outputs; update that loop to skip outputs that are teleports using the
same predicate used earlier for issuance (i.e., the teleport-detection check
used when building issuance assetList). Concretely, inside the for _, out :=
range asstGp.Outputs loop, add the same conditional (e.g., if isTeleport(out) or
the existing out teleport check used above) to continue/skip teleport outputs
before constructing the domain.NormalAsset so assetList only receives
non-teleport outputs.
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)
pkg/ark-lib/tree/forfeit_tx.go (1)
15-22: Deduct extensionAnchor value from the forfeit output total.If
extensionAnchorcarries a non‑zero value,forfeitOutis overstated (onlyANCHOR_VALUEis subtracted), so outputs exceed inputs and the rebuilt txid will never match. SubtractextensionAnchor.Value(or assert it must be zero) when computingsumPrevout.🐛 Proposed fix
sumPrevout := int64(0) for _, prevout := range prevouts { sumPrevout += prevout.Value } sumPrevout -= txutils.ANCHOR_VALUE + if extensionAnchor != nil { + sumPrevout -= extensionAnchor.Value + }
🤖 Fix all issues with AI agents
In `@internal/infrastructure/tx-builder/covenantless/builder.go`:
- Around line 1212-1273: The loop over assetPkt.Assets currently returns output
inside the loop causing only the first AssetGroup to be validated; move the
final "return output, nil" out of the for _, asset := range assetPkt.Assets loop
(or enforce that assetPkt.Assets has length 1) so every asset group is fully
validated against vtxoMap, the asset input checks (in.Vin range), extension
type/assertion (domain.ExtAsset and domain.AssetExtension) and foundAsset logic
for each asset group before returning from the function.
🧹 Nitpick comments (1)
internal/infrastructure/tx-builder/covenantless/utils.go (1)
4-7: Avoid unconditional stdout logging in leaf construction.
log.Printfruns for every offchain receiver and can create noisy output in production. Consider removing it or gating behind a debug logger.♻️ Suggested cleanup
-import ( +import ( "encoding/hex" "fmt" - "log" "github.com/arkade-os/arkd/internal/core/domain" "github.com/arkade-os/arkd/pkg/ark-lib/extension" "github.com/arkade-os/arkd/pkg/ark-lib/script" "github.com/arkade-os/arkd/pkg/ark-lib/tree" "github.com/btcsuite/btcd/btcec/v2/schnorr" "github.com/btcsuite/btcd/btcutil" "github.com/btcsuite/btcd/chaincfg" "github.com/btcsuite/btcd/txscript" "github.com/btcsuite/btcd/wire" ) @@ - log.Printf("plain vtxo script %s", hex.EncodeToString(vtxoScript)) - leaves = append(leaves, tree.Leaf{ Script: hex.EncodeToString(vtxoScript), Amount: receiver.Amount, CosignersPublicKeys: cosigners, })Also applies to: 120-120
| for _, asset := range assetPkt.Assets { | ||
| if asset.AssetId == nil { | ||
| // Issuance assets don't have inputs to validate against existing VTXOs | ||
| continue | ||
| } | ||
| assetID := asset.AssetId.ToString() | ||
| for _, in := range asset.Inputs { | ||
| if in.Type == extension.AssetTypeTeleport { | ||
| continue | ||
| } | ||
|
|
||
| if int(in.Vin) >= len(pkt.UnsignedTx.TxIn) { | ||
| return nil, fmt.Errorf( | ||
| "asset input index out of range for txid %s: vin=%d", | ||
| txid, in.Vin, | ||
| ) | ||
| } | ||
|
|
||
| prevOut := pkt.UnsignedTx.TxIn[in.Vin].PreviousOutPoint | ||
| outpoint := domain.Outpoint{ | ||
| Txid: prevOut.Hash.String(), | ||
| VOut: prevOut.Index, | ||
| } | ||
|
|
||
| vtxo, ok := vtxoMap[outpoint] | ||
| if !ok { | ||
| return nil, fmt.Errorf( | ||
| "vtxo not found for asset input txid=%s assetID=%s outpoint=%s", | ||
| txid, assetID, outpoint.String(), | ||
| ) | ||
| } | ||
|
|
||
| foundAsset := false | ||
| for _, ext := range vtxo.Extensions { | ||
| if ext.Type() != domain.ExtAsset { | ||
| continue | ||
| } | ||
|
|
||
| assetExt, ok := ext.(domain.AssetExtension) | ||
| if !ok { | ||
| return nil, fmt.Errorf( | ||
| "extension type is ExtAsset but concrete type is %T", | ||
| ext, | ||
| ) | ||
| } | ||
|
|
||
| if assetExt.AssetID == assetID { | ||
| foundAsset = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !foundAsset { | ||
| return nil, fmt.Errorf( | ||
| "vtxo %s missing asset extension for assetID %s", | ||
| outpoint.String(), assetID, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| return output, nil | ||
| } |
There was a problem hiding this comment.
Validate all asset groups before returning the extension anchor.
The function returns after the first AssetGroup, so any additional groups in the same packet are never validated and could bypass checks. Move the return after the loop (or enforce a single‑group invariant).
🐛 Proposed fix
- for _, asset := range assetPkt.Assets {
+ for _, asset := range assetPkt.Assets {
if asset.AssetId == nil {
// Issuance assets don't have inputs to validate against existing VTXOs
continue
}
assetID := asset.AssetId.ToString()
for _, in := range asset.Inputs {
if in.Type == extension.AssetTypeTeleport {
continue
}
// ...
}
-
- return output, nil
}
+ return output, nil🤖 Prompt for AI Agents
In `@internal/infrastructure/tx-builder/covenantless/builder.go` around lines 1212
- 1273, The loop over assetPkt.Assets currently returns output inside the loop
causing only the first AssetGroup to be validated; move the final "return
output, nil" out of the for _, asset := range assetPkt.Assets loop (or enforce
that assetPkt.Assets has length 1) so every asset group is fully validated
against vtxoMap, the asset input checks (in.Vin range), extension type/assertion
(domain.ExtAsset and domain.AssetExtension) and foundAsset logic for each asset
group before returning from the function.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/infrastructure/db/service.go`:
- Around line 392-394: Assets() returns s.assetStore but the Postgres
initialization never assigns assetStore and Close() doesn't shut it down; update
the Postgres branch of the service initialization (where the Postgres DB client
is created) to instantiate and assign s.assetStore (the concrete Postgres
AssetRepository) and then modify the service.Close() method to call
s.assetStore.Close() (or the repository's Close/Shutdown method) alongside other
resources so the asset store is not nil and is properly closed on shutdown.
♻️ Duplicate comments (21)
internal/infrastructure/tx-builder/covenantless/builder.go (1)
1212-1261: Critical: Early return bypasses validation of subsequent asset groups.The
return output, nilat line 1260 is inside thefor _, asset := range assetPkt.Assetsloop. This means only the first asset group with a non-nilAssetIdis validated—any additional groups in the packet are silently skipped, potentially allowing invalid asset transitions to pass verification.Additionally, there's variable shadowing at line 1245 where
assetshadows the outer loop variable.🐛 Proposed fix
for _, asset := range assetPkt.Assets { if asset.AssetId == nil { // Issuance assets don't have inputs to validate against existing VTXOs continue } assetID := asset.AssetId.ToString() for _, in := range asset.Inputs { if in.Type == extension.AssetTypeTeleport { continue } if int(in.Vin) >= len(pkt.UnsignedTx.TxIn) { return nil, fmt.Errorf( "asset input index out of range for txid %s: vin=%d", txid, in.Vin, ) } prevOut := pkt.UnsignedTx.TxIn[in.Vin].PreviousOutPoint outpoint := domain.Outpoint{ Txid: prevOut.Hash.String(), VOut: prevOut.Index, } vtxo, ok := vtxoMap[outpoint] if !ok { return nil, fmt.Errorf( "vtxo not found for asset input txid=%s assetID=%s outpoint=%s", txid, assetID, outpoint.String(), ) } foundAsset := false - for _, asset := range vtxo.Assets { - if asset.AssetID == assetID { + for _, vtxoAsset := range vtxo.Assets { + if vtxoAsset.AssetID == assetID { foundAsset = true break } } if !foundAsset { return nil, fmt.Errorf( "vtxo %s missing asset extension for assetID %s", outpoint.String(), assetID, ) } } - - return output, nil } + + return output, nil }internal/core/application/indexer.go (1)
273-280: Use the stored asset amount when enriching by outpoint.
Amountis hardcoded to0, so clients always see a zero balance even when the asset exists.🐛 Fix amount propagation
- allVtxos[j].Assets = append(allVtxos[j].Assets, domain.Asset{ - AssetID: asset.AssetID, - Amount: 0, - }) + allVtxos[j].Assets = append(allVtxos[j].Assets, domain.Asset{ + AssetID: asset.AssetID, + Amount: asset.Amount, + })internal/interface/grpc/handlers/indexer_teleport_test.go (2)
178-193: Verify the mock stream expectation.The mock expectation on
Sendis never asserted, so the test can pass even ifSendwas never called.✅ Add expectation verification
time.Sleep(200 * time.Millisecond) // wait for processing + + stream.AssertExpectations(t) }
195-201: Avoid asserting inside the goroutine.
assert.NoErrorinside the goroutine can race and may not fail the test. Capture the error and assert in the main goroutine.🔧 Safer goroutine assertion
+ errCh := make(chan error, 1) // 3. Start GetSubscription loop go func() { err := svc.GetSubscription(&arkv1.GetSubscriptionRequest{ SubscriptionId: subRes.SubscriptionId, }, stream) - assert.NoError(t, err) + errCh <- err }() + + // ... after sending the event + select { + case err := <-errCh: + assert.NoError(t, err) + case <-time.After(500 * time.Millisecond): + // still running is OK for this test + }internal/core/application/utils.go (2)
196-199: Guard PkScript slicing for non‑asset outputs.
out.PkScript[2:]assumes a valid P2TR script length. Short/invalid scripts can panic or yield wrong pubkeys.🛡️ Defensive length check
} else { + if len(out.PkScript) < 34 { + continue // skip non-P2TR outputs + } pubKey = hex.EncodeToString(out.PkScript[2:]) isSubDust = script.IsSubDustScript(out.PkScript) }
220-231: Match assets to outputs by outpoint, not slice index.
outsis filtered (anchors skipped), so usingasst.VOutas a slice index can silently drop valid assets. Match by outpoint instead.🔧 Safer attachment by outpoint
- // Add AssetGroup if Present - for _, asst := range assetList { - idx := int(asst.VOut) - if idx < 0 || idx >= len(outs) { - continue - } - - outs[idx].Assets = append(outs[idx].Assets, domain.Asset{ - AssetID: asst.AssetID, - Amount: asst.Amount, - }) - } + // Add AssetGroup if Present - match by outpoint + outByVout := make(map[uint32]int, len(outs)) + for i := range outs { + outByVout[outs[i].Outpoint.VOut] = i + } + for _, asst := range assetList { + idx, ok := outByVout[asst.VOut] + if !ok { + continue + } + outs[idx].Assets = append(outs[idx].Assets, domain.Asset{ + AssetID: asst.AssetID, + Amount: asst.Amount, + }) + }internal/core/application/service.go (9)
355-357: Restore or explicitly remove the watcher restoration.Leaving
restoreWatchingVtxoscommented out risks missing watches after restart. Either restore it or remove it with a clear rationale.🔧 Reinstate watcher restoration
- // if err := svc.restoreWatchingVtxos(); err != nil { - // return nil, fmt.Errorf("failed to restore watching vtxos: %s", err) - // } + if err := svc.restoreWatchingVtxos(); err != nil { + return nil, fmt.Errorf("failed to restore watching vtxos: %s", err) + }
1131-1138: Clarify whether asset persistence is blocking.
storeAssetDetailsFromArkTxfailures are logged but the offchain tx still succeeds, which can permanently desync asset state. Decide whether this must fail the submission or is best‑effort.🔧 Blocking option (fail submission)
if assetOutputIndex >= 0 { if err := s.storeAssetDetailsFromArkTx( ctx, *arkPtx.UnsignedTx, assetOutputIndex, ); err != nil { - log.WithError(err).Errorf( - "failed to store asset details for offchain tx %s", txid, - ) + return nil, errors.INTERNAL_ERROR.New( + "failed to store asset details for offchain tx %s: %w", txid, err, + ) } }
1566-1587: Persist teleport assets with correct vout and fail on insert errors.
OutputIndexuses the loop index (can diverge from the encoded vout) and insert errors are swallowed, which can leave intents inconsistent.🐛 Use output.Vout and propagate insert failures
- if err := s.repoManager.Assets().InsertTeleportAsset(ctx, domain.TeleportAsset{ + if err := s.repoManager.Assets().InsertTeleportAsset(ctx, domain.TeleportAsset{ IntentID: proofTxid, Script: teleportScript, - OutputIndex: uint32(i), + OutputIndex: uint32(output.Vout), AssetID: asst.AssetId.ToString(), Amount: output.Amount, IsClaimed: false, }); err != nil { - log.WithError(err).Warn("failed to insert teleport asset") + return "", errors.INTERNAL_ERROR.New( + "failed to insert teleport asset: %w", err, + ).WithMetadata(map[string]any{ + "intent_id": proofTxid, + "asset_id": asst.AssetId.ToString(), + "vout": output.Vout, + }) }
1689-1691: Only count off‑chain outputs inoffchainOutputs.
offchainOutputsis appended for all outputs, so on‑chain outputs inflate fee calculations. Move the append into the off‑chain branch.🛠️ Move append into off‑chain branch
} else { ... hasOffChainReceiver = true rcv.PubKey = hex.EncodeToString(output.PkScript[2:]) + offchainOutputs = append(offchainOutputs, *output) } receivers = append(receivers, rcv) - offchainOutputs = append(offchainOutputs, *output)
1743-1747: Remove the staleerrcheck afterGetArkPsbtFields.
errhere refers to an earlier scope and is always nil, so this block is dead code. Either capture the error fromGetArkPsbtFieldsor remove the check.🧹 Remove dead check
- if err != nil { - return "", errors.INVALID_PSBT_INPUT.New( - "failed to get asset seal field for input %d: %w", i+1, err, - ).WithMetadata(errors.InputMetadata{Txid: proofTxid, InputIndex: i + 1}) - }
3240-3241: Remove stray stdout debug print.Use the project logger or drop the line before merge.
🧹 Remove debug print
- fmt.Printf("events %+v", changes)
4421-4431: Skip teleport outputs when building normal assets for updates.The update path currently adds teleport outputs to
assetList, unlike the issuance path.🛠️ Skip teleport outputs
for _, out := range asstGp.Outputs { + if out.Type == extension.AssetTypeTeleport { + continue + } asst := domain.NormalAsset{ Outpoint: domain.Outpoint{ Txid: arkTx.TxID(), VOut: uint32(out.Vout), }, AssetID: assetId.ToString(), Amount: out.Amount, } assetList = append(assetList, asst) }
4476-4493: Ensure quantity decreases fail on insufficient balance.
DecreaseAssetGroupQuantitycan be a silent no‑op in SQL backends (0 rows affected), yetupdateAssetQuantityreturns success. Add a balance check here or make the repo methods report zero‑row updates as errors.
4511-4526: Use the encoded asset vout forOutputVout.The loop index can diverge from the encoded vout; events should carry the actual
assetOut.Vout.🐛 Fix OutputVout
- OutputVout: uint32(outIdx), + OutputVout: assetOut.Vout,internal/infrastructure/db/service.go (1)
567-569: Guard PkScript slicing for non‑asset outputs.
out.PkScript[2:]can panic or return invalid data for short/non‑P2TR scripts.🛡️ Defensive check
} else { + if len(out.PkScript) < 34 { + log.Warnf("skipping non-P2TR output at index %d", outIndex) + continue + } isSubDust = script.IsSubDustScript(out.PkScript) pubKey = out.PkScript[2:] }api-spec/openapi/swagger/ark/v1/indexer.openapi.json (1)
8-23: Path parameter naming mismatch (already flagged).The path uses
{asset_id}(snake_case) but the parameter name isassetId(camelCase). This was previously flagged and should be aligned.api-spec/protobuf/ark/v1/indexer.proto (2)
78-83: Doc comment mismatch (already flagged).The comment says "GetAsset" but the RPC is
GetAssetGroup. This was previously flagged.
110-117: Typo in doc comment (already flagged)."subscribtion" should be "subscription" at line 111. This was previously flagged.
internal/interface/grpc/handlers/indexer.go (2)
50-82: Potential nil pointer dereference onresp.AssetGroup.While there's a nil check for
respat line 61, ifindexerSvc.GetAssetGroupreturns a non-nil response with a nilAssetGroupfield, accessingresp.AssetGroup.Metadataat line 66 will panic. The previous review suggested checkingresp.AssetGroup == nilas well.🐛 Proposed fix
if resp == nil { return nil, status.Errorf(codes.NotFound, "asset not found: %s", assetId) } + if resp.AssetGroup == nil { + return nil, status.Errorf(codes.NotFound, "asset group not found for id %s", assetId) + } assetMetadata := make([]*arkv1.AssetMetadata, 0)
486-494: Both listener channels could be nil, causing no event delivery.If neither a script subscription nor a teleport subscription exists for the given
subscriptionId, bothscriptChandteleportChwill be nil. While the stream won't fully block (timer and context cancellation still work), the client will only receive heartbeats and never actual events. Consider returning an error if no subscriptions exist for clarity.🐛 Proposed fix
teleportCh, err := h.teleportSubsHandler.getListenerChannel(subscriptionId) if err != nil && !strings.Contains(err.Error(), "listener not found") { return status.Error(codes.Internal, err.Error()) } + if scriptCh == nil && teleportCh == nil { + return status.Error(codes.NotFound, "no active subscriptions found for id") + } + // create a Timer that will fire after one heartbeat interval
🧹 Nitpick comments (3)
internal/infrastructure/tx-builder/covenantless/builder.go (1)
288-302: Remove unusedforfeitTxPtxListand fix typo.
forfeitTxPtxListis accumulated but never read afterward—this is dead code.- Line 296 has a typo: "verfity" → "verify".
♻️ Proposed fix
- forfeitTxPtxList := make([]*psbt.Packet, 0) - for _, forfeitTx := range forfeitTxs { tx, err := psbt.NewFromRawBytes(strings.NewReader(forfeitTx), true) if err != nil { return nil, err } - // verfity asset forfeit transaction + // verify asset forfeit transaction extensionAnchor, err := verifyAssetForfeitTransaction(indexedVtxos, tx) if err != nil { return nil, err } - - forfeitTxPtxList = append(forfeitTxPtxList, tx)internal/core/application/indexer.go (1)
246-257: Enrich only paged Vtxos to avoid unnecessary asset lookups.The enrichment loop runs over all Vtxos even though only a page is returned, which can trigger a lot of extra DB calls. Consider enriching just the paged slice (or batch fetching) instead.
♻️ Suggested refactor
- vtxos, pageResp := paginate(allVtxos, page, maxPageSizeSpendableVtxos) - - for j, v := range allVtxos { - // add asset to vtxo if present - asset, err := i.repoManager.Assets().GetAssetByOutpoint(ctx, v.Outpoint) - if err == nil && asset != nil { - allVtxos[j].Assets = append(allVtxos[j].Assets, domain.Asset{ - AssetID: asset.AssetID, - Amount: asset.Amount, - }) - } - } + vtxos, pageResp := paginate(allVtxos, page, maxPageSizeSpendableVtxos) + + for j := range vtxos { + // add asset to vtxo if present + asset, err := i.repoManager.Assets().GetAssetByOutpoint(ctx, vtxos[j].Outpoint) + if err == nil && asset != nil { + vtxos[j].Assets = append(vtxos[j].Assets, domain.Asset{ + AssetID: asset.AssetID, + Amount: asset.Amount, + }) + } + }internal/interface/grpc/handlers/indexer_teleport_test.go (1)
159-168: Check setup errors from asset/PSBT encoding.Ignoring these errors can let the test pass with invalid setup data.
✅ Safer setup
- opretTxOut, _ := assetGroup.EncodeAssetPacket() + opretTxOut, err := assetGroup.EncodeAssetPacket() + assert.NoError(t, err) // Create PSBT tx := wire.NewMsgTx(2) tx.AddTxOut(&opretTxOut) - ptx, _ := psbt.NewFromUnsignedTx(tx) + ptx, err := psbt.NewFromUnsignedTx(tx) + assert.NoError(t, err) // Encode PSBT to Base64 (ignoring error for test) - b64Ptx, _ := ptx.B64Encode() + b64Ptx, err := ptx.B64Encode() + assert.NoError(t, err)
| func (s *service) Assets() domain.AssetRepository { | ||
| return s.assetStore | ||
| } |
There was a problem hiding this comment.
Initialize and close the asset store for Postgres.
Assets() returns s.assetStore, but the Postgres branch never assigns it, and Close() doesn’t close it. This will return nil (and leak resources) in Postgres deployments.
🐛 Initialize assetStore and close it
case "postgres":
...
convictionStore, err = convictionStoreFactory(db)
if err != nil {
return nil, fmt.Errorf("failed to create conviction store: %w", err)
}
+ assetStore, err = assetStoreFactory(db)
+ if err != nil {
+ return nil, fmt.Errorf("failed to create asset store: %w", err)
+ }
intentFeesStore, err = intentFeesStoreFactory(db)
if err != nil {
return nil, fmt.Errorf("failed to create intent fees store: %w", err)
}
...
func (s *service) Close() {
s.eventStore.Close()
s.roundStore.Close()
s.vtxoStore.Close()
s.scheduledSessionStore.Close()
s.offchainTxStore.Close()
s.convictionStore.Close()
+ s.assetStore.Close()
}🤖 Prompt for AI Agents
In `@internal/infrastructure/db/service.go` around lines 392 - 394, Assets()
returns s.assetStore but the Postgres initialization never assigns assetStore
and Close() doesn't shut it down; update the Postgres branch of the service
initialization (where the Postgres DB client is created) to instantiate and
assign s.assetStore (the concrete Postgres AssetRepository) and then modify the
service.Close() method to call s.assetStore.Close() (or the repository's
Close/Shutdown method) alongside other resources so the asset store is not nil
and is properly closed on shutdown.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/infrastructure/db/service_test.go`:
- Around line 1747-1766: The subtest "insert get and update teleport asset" is
not self-contained because it inserts a teleport asset that depends on a group
record; before calling svc.Assets().InsertTeleportAsset(ctx, asset) create a
locally-scoped unique group ID (e.g., groupID := randomString(16)) and insert
that group using the project's group insertion helper (call the service method
used elsewhere, e.g., svc.Groups().InsertGroup(ctx, domain.Group{ID: groupID,
...}) or the equivalent group insert function) and set the teleport asset’s
grouping FK fields to that ID so the FK constraint is satisfied within this
subtest.
In `@internal/interface/grpc/handlers/indexer.go`:
- Around line 619-661: The goroutine uses the loop-scoped variable event
(accessing event.Txid and event.Tx), which can race as event is reused; capture
the needed values (e.g., txid := event.Txid; tx := event.Tx) and pass them into
the closure (or accept them as parameters) when invoking the goroutine for
listener *listener[*arkv1.GetSubscriptionResponse]; also do the same fix for the
script subscription handler. Replace the current non-blocking send/drop
semantics on listener.ch with a guaranteed-delivery approach by making
listener.ch a buffered channel sized appropriately (ensure listener.ch is
created as a buffered channel where listener is constructed in
teleportSubsHandler) so the goroutine can send without silently dropping
messages and without blocking the main loop.
♻️ Duplicate comments (10)
internal/infrastructure/db/service.go (2)
420-427: CloseassetStoreinservice.Close().
assetStoreis now initialized but never closed, which can leak DB resources. Add a Close call (with a nil guard if needed).✅ Suggested fix
func (s *service) Close() { s.eventStore.Close() s.roundStore.Close() s.vtxoStore.Close() s.scheduledSessionStore.Close() s.offchainTxStore.Close() s.convictionStore.Close() + if s.assetStore != nil { + s.assetStore.Close() + } }
543-574: Skip non-asset OP_RETURN outputs and validate P2TR length before slicing.If asset packets are chunked, later OP_RETURN chunks may not match
ContainsAssetPacketand will be parsed as P2TR, creating bogus vtxos or risking panics on short scripts. Add an explicit OP_RETURN skip (for non-asset outputs) and a length/type guard beforeout.PkScript[2:].🛠️ Possible fix
} else { + if bytes.HasPrefix(out.PkScript, []byte{txscript.OP_RETURN}) { + // Skip non-asset OP_RETURN outputs (e.g., chunked asset packets) + continue + } + if len(out.PkScript) < 34 { + log.Warnf("skipping non-P2TR output at index %d", outIndex) + continue + } isSubDust = script.IsSubDustScript(out.PkScript) pubKey = out.PkScript[2:] }In ark-lib extension (v0.8.1-0.20260102141947-c66f85422174), does ContainsAssetPacket return true for every OP_RETURN chunk of a chunked asset packet, or only the first? What is the expected handling of additional OP_RETURN outputs?internal/infrastructure/db/postgres/asset_repo.go (4)
32-34: Avoid closing a shared*sql.DBin repositoryClose().If the DB handle is shared across repositories, closing it here can break other stores. Let the service manage lifecycle or make this a no-op.
147-168: Make anchor + asset inserts atomic.
CreateAssetAnchorfollowed byAddAssetcalls can leave an orphaned anchor on failure. Wrap these writes in a single transaction.
206-239: Wrap asset group creation + metadata upserts in a transaction.A failure during metadata insertion can leave a partially created group. Use a transaction to keep the group and its metadata consistent.
284-292: Surface insufficient quantity on decrease.
SubtractFromAssetQuantitycan update zero rows (quantity < amount) but this method still returns nil. CheckRowsAffected()and return a clear error.api-spec/protobuf/ark/v1/indexer.proto (1)
78-103: Fix RPC doc comment naming + subscription typo.Line 78 says “GetAsset” while the RPC is
GetAssetGroup, and Line 102 misspells “subscription”.✏️ Proposed fix
- // GetAsset returns the asset information and metadata for the specified asset ID. + // GetAssetGroup returns the asset information and metadata for the specified asset ID. ... - // scripts. It can also be used to update an existing subscribtion by adding new scripts to it. + // scripts. It can also be used to update an existing subscription by adding new scripts to it.api-spec/openapi/swagger/ark/v1/indexer.openapi.json (1)
8-23: Align path parameter naming with template (and fix description).Line 8 uses
{asset_id}but Line 17 declaresassetId, which breaks OpenAPI parameter binding. Line 13 also says “GetAsset” instead of “GetAssetGroup”.💡 Proposed fix
- "description": "GetAsset returns the asset information and metadata for the specified asset ID.", + "description": "GetAssetGroup returns the asset information and metadata for the specified asset ID.", ... - "name": "assetId", + "name": "asset_id",internal/interface/grpc/handlers/indexer.go (2)
50-80: Guard against nil AssetGroup to avoid panic.Line 66 dereferences
resp.AssetGroup.Metadatawithout checkingresp.AssetGroup. If the service returns a nil AssetGroup, this will panic. Add a nil guard and return NotFound.🐛 Proposed fix
- if resp == nil { - return nil, status.Errorf(codes.NotFound, "asset not found: %s", assetId) - } + if resp == nil || resp.AssetGroup == nil { + return nil, status.Errorf(codes.NotFound, "asset group not found: %s", assetId) + }
433-442: Handle missing subscriptions before entering select.If both
scriptChandteleportChare nil, the select will never receive events and the stream will only emit heartbeats. Consider returning NotFound early.🐛 Proposed fix
teleportCh, err := h.teleportSubsHandler.getListenerChannel(subscriptionId) if err != nil && !strings.Contains(err.Error(), "listener not found") { return status.Error(codes.Internal, err.Error()) } + if scriptCh == nil && teleportCh == nil { + return status.Error(codes.NotFound, "no active subscriptions found for id") + }
| t.Run("insert get and update teleport asset", func(t *testing.T) { | ||
| ctx := context.Background() | ||
| script := randomString(32) | ||
| intentID := randomString(32) | ||
| assetID := "asset-3" | ||
| outputIndex := uint32(7) | ||
| amount := uint64(5000) | ||
|
|
||
| asset := domain.TeleportAsset{ | ||
| Script: script, | ||
| IntentID: intentID, | ||
| AssetID: assetID, | ||
| OutputIndex: outputIndex, | ||
| Amount: amount, | ||
| IsClaimed: false, | ||
| } | ||
|
|
||
| err := svc.Assets().InsertTeleportAsset(ctx, asset) | ||
| require.NoError(t, err, "InsertTeleportAsset should succeed") | ||
|
|
There was a problem hiding this comment.
Make the teleport-asset subtest self-contained.
It relies on a prior subtest to create "asset-3". Running this subtest alone will fail FK constraints. Create a local group with a unique ID in this subtest.
✅ Suggested adjustment
- assetID := "asset-3"
+ assetID := "asset-teleport-1"
+ err := svc.Assets().InsertAssetGroup(ctx, domain.AssetGroup{
+ ID: assetID,
+ Quantity: 1,
+ })
+ require.NoError(t, err)
asset := domain.TeleportAsset{
Script: script,
IntentID: intentID,
AssetID: assetID,
OutputIndex: outputIndex,
Amount: amount,
IsClaimed: false,
}
- err := svc.Assets().InsertTeleportAsset(ctx, asset)
+ err = svc.Assets().InsertTeleportAsset(ctx, asset)🤖 Prompt for AI Agents
In `@internal/infrastructure/db/service_test.go` around lines 1747 - 1766, The
subtest "insert get and update teleport asset" is not self-contained because it
inserts a teleport asset that depends on a group record; before calling
svc.Assets().InsertTeleportAsset(ctx, asset) create a locally-scoped unique
group ID (e.g., groupID := randomString(16)) and insert that group using the
project's group insertion helper (call the service method used elsewhere, e.g.,
svc.Groups().InsertGroup(ctx, domain.Group{ID: groupID, ...}) or the equivalent
group insert function) and set the teleport asset’s grouping FK fields to that
ID so the FK constraint is satisfied within this subtest.
| teleportListenersCopy := h.teleportSubsHandler.getListenersCopy() | ||
| if len(teleportListenersCopy) > 0 { | ||
| parsedTeleportEvents := make([]*arkv1.TeleportEvent, 0) | ||
| for _, te := range event.TeleportAssets { | ||
| parsedTeleportEvents = append(parsedTeleportEvents, &arkv1.TeleportEvent{ | ||
| TeleportHash: te.TeleportHash, | ||
| AnchorOutpoint: te.AnchorOutpoint.String(), | ||
| AssetId: te.AssetID, | ||
| Amount: te.Amount, | ||
| OutputVout: te.OutputVout, | ||
| CreatedAt: te.CreatedAt, | ||
| ExpiresAt: te.ExpiresAt, | ||
| }) | ||
| } | ||
|
|
||
| for _, l := range teleportListenersCopy { | ||
| teleportEvents := make([]*arkv1.TeleportEvent, 0) | ||
| involvedHashes := make([]string, 0) | ||
|
|
||
| for _, tpEvent := range parsedTeleportEvents { | ||
| if _, ok := l.topics[tpEvent.TeleportHash]; ok { | ||
| involvedHashes = append(involvedHashes, tpEvent.TeleportHash) | ||
| teleportEvents = append(teleportEvents, tpEvent) | ||
| } | ||
| } | ||
|
|
||
| if len(teleportEvents) > 0 { | ||
| go func(listener *listener[*arkv1.GetSubscriptionResponse]) { | ||
| select { | ||
| case listener.ch <- &arkv1.GetSubscriptionResponse{ | ||
| Data: &arkv1.GetSubscriptionResponse_Event{ | ||
| Event: &arkv1.IndexerSubscriptionEvent{ | ||
| Txid: event.Txid, | ||
| TeleportEvents: teleportEvents, | ||
| TeleportHashes: involvedHashes, | ||
| Tx: event.Tx, | ||
| }, | ||
| }, | ||
| }: | ||
| default: | ||
| // channel is full, skip this message to prevent blocking | ||
| } | ||
| }(l) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp/repo && cat -n internal/interface/grpc/handlers/indexer.go | sed -n '580,670p'Repository: arkade-os/arkd
Length of output: 117
🏁 Script executed:
cd /tmp/repo && cat -n internal/interface/grpc/handlers/indexer.go | sed -n '550,700p'Repository: arkade-os/arkd
Length of output: 117
🏁 Script executed:
cd /tmp/repo && rg -n "func.*teleportListenersCopy\|func.*\(.*event.*\)" internal/interface/grpc/handlers/indexer.go | head -20Repository: arkade-os/arkd
Length of output: 117
🏁 Script executed:
cd /tmp/repo && ast-grep --pattern 'func $$$($_) {
$$$
teleportListenersCopy := h.teleportSubsHandler.getListenersCopy()
$$$
}'Repository: arkade-os/arkd
Length of output: 117
🏁 Script executed:
cat -n internal/interface/grpc/handlers/indexer.go | sed -n '550,700p'Repository: arkade-os/arkd
Length of output: 5650
🏁 Script executed:
rg -n "for.*event" internal/interface/grpc/handlers/indexer.go -B 5 | head -40Repository: arkade-os/arkd
Length of output: 1230
🏁 Script executed:
ast-grep --pattern 'for $_, event := range $_ {
$$$
}'Repository: arkade-os/arkd
Length of output: 12093
Capture loop variable event before spawning goroutines to avoid race conditions.
The goroutines at lines 597–616 and 646–661 reference event.Txid and event.Tx without capturing them. Since event is reused on each loop iteration, these references race with subsequent events. Pass the values as parameters to the closure.
Additionally, replace the non-blocking send with drop semantics (select { default: }) with buffered channels to maintain delivery guarantees for subscription events, as these should never be dropped.
Proposed fix
teleportListenersCopy := h.teleportSubsHandler.getListenersCopy()
if len(teleportListenersCopy) > 0 {
+ txid := event.Txid
+ tx := event.Tx
parsedTeleportEvents := make([]*arkv1.TeleportEvent, 0)
for _, te := range event.TeleportAssets {
parsedTeleportEvents = append(parsedTeleportEvents, &arkv1.TeleportEvent{
@@
if len(teleportEvents) > 0 {
- go func(listener *listener[*arkv1.GetSubscriptionResponse]) {
+ go func(listener *listener[*arkv1.GetSubscriptionResponse], txid string, tx []byte, teleportEvents []*arkv1.TeleportEvent, involvedHashes []string) {
select {
case listener.ch <- &arkv1.GetSubscriptionResponse{
Data: &arkv1.GetSubscriptionResponse_Event{
Event: &arkv1.IndexerSubscriptionEvent{
- Txid: event.Txid,
+ Txid: txid,
TeleportEvents: teleportEvents,
- TeleportHashes: involvedHashes,
- Tx: event.Tx,
+ TeleportHashes: involvedHashes,
+ Tx: tx,
},
},
- }:
- default:
- // channel is full, skip this message to prevent blocking
+ }
}
- }(l)
+ }(l, txid, tx, teleportEvents, involvedHashes)
}
}
}Apply the same fix to the script subscription handler at lines 597–616.
🤖 Prompt for AI Agents
In `@internal/interface/grpc/handlers/indexer.go` around lines 619 - 661, The
goroutine uses the loop-scoped variable event (accessing event.Txid and
event.Tx), which can race as event is reused; capture the needed values (e.g.,
txid := event.Txid; tx := event.Tx) and pass them into the closure (or accept
them as parameters) when invoking the goroutine for listener
*listener[*arkv1.GetSubscriptionResponse]; also do the same fix for the script
subscription handler. Replace the current non-blocking send/drop semantics on
listener.ch with a guaranteed-delivery approach by making listener.ch a buffered
channel sized appropriately (ensure listener.ch is created as a buffered channel
where listener is constructed in teleportSubsHandler) so the goroutine can send
without silently dropping messages and without blocking the main loop.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/core/application/service.go`:
- Around line 4400-4404: The error message at the s.ensureAssetPresence callsite
is misleading—change it to accurately reflect that the validation failed because
the control asset is missing or the asset list is empty; update the returned
error from the s.ensureAssetPresence block to something like "control asset not
present in assetGroupList" and include the underlying error (wrap with %w) so
callers see the specific reason reported by ensureAssetPresence (keep reference
to s.ensureAssetPresence and the controlAssetID variable when constructing the
message).
♻️ Duplicate comments (8)
internal/core/application/service.go (8)
355-358: Restore or remove commented-outrestoreWatchingVtxoscall.This commented-out code disables vtxo watching restoration on service startup, potentially missing fraud detection events. Either restore it or remove it with a comment explaining why it's intentionally disabled.
1131-1139: Clarify error handling semantics for asset storage.Asset storage errors are logged but don't fail the transaction. This creates potential inconsistency where the offchain transaction succeeds but asset state isn't persisted. Make this behavior explicit—either return the error to fail the transaction, or add a comment explaining why best-effort is acceptable.
1578-1587: Teleport asset insertion errors should fail registration.
InsertTeleportAsseterrors are logged but the receiver is still added to the intent. This creates inconsistent state where the intent references a teleport asset that wasn't persisted. Return an error instead.Additionally,
OutputIndex: uint32(i)uses the loop index which may not match the actual output vout.
1685-1691: On-chain outputs are incorrectly added tooffchainOutputs.Line 1690 appends the output to
offchainOutputsregardless of whether it's on-chain or off-chain. The append should only occur in the off-chain branch (wherehasOffChainReceiveris set), not unconditionally at the end.
4419-4428: Teleport outputs should be excluded fromassetListfor existing assets.For issuance (lines 4359-4362), teleport outputs are correctly skipped. However, for existing assets, all outputs including teleports are added to
assetList. Add the same check:for _, out := range asstGp.Outputs { + if out.Type == extension.AssetTypeTeleport { + continue + } + asst := domain.NormalAsset{
4474-4495: Silent failure risk inDecreaseAssetGroupQuantity.The SQLite and Postgres implementations use
UPDATE ... WHERE quantity >= amount, which succeeds with zero rows affected when balance is insufficient. This function returns success even when the decrease failed. Add validation or modify the DB implementations to report zero-row updates as errors.
4519-4530:OutputVoutshould useassetOut.Voutinstead of loop index.Line 4523 sets
OutputVout: uint32(outIdx)using the loop iteration index. For teleport outputs, this should reflect the actual vout from the asset output:event := TeleportAsset{ TeleportHash: hex.EncodeToString(assetOut.Script), AnchorOutpoint: anchorOutpoint, AssetID: ast.AssetId.ToString(), - OutputVout: uint32(outIdx), + OutputVout: assetOut.Vout, CreatedAt: createdAt, ExpiresAt: expiresAt, }
1742-1747: Dead code:errcheck references already-handled error.The
errvariable here is from thes.cache.OffchainTxs().Includes()call on line 1726, which was already checked and handled on lines 1727-1736. At this point,errcan only benil. The error fromGetArkPsbtFieldson line 1739 is discarded with_.Remove this unreachable block:
- if err != nil { - return "", errors.INVALID_PSBT_INPUT.New( - "failed to get asset seal field for input %d: %w", i+1, err, - ).WithMetadata(errors.InputMetadata{Txid: proofTxid, InputIndex: i + 1}) - }
🧹 Nitpick comments (2)
internal/core/application/service.go (2)
4442-4461: Consider returning errors frommarkTeleportInputsClaimed.Currently,
UpdateTeleportAsseterrors are only logged. While this may be acceptable for a background operation, it could leave teleport assets in an inconsistent claimed state. Consider whether failures here should be surfaced.
4319-4342: Variable shadowing: innerassetIdshadows outer variable.Line 4326 declares
assetIdwhich shadows the outerassetIdfrom line 4317. While this works correctly for the issuance flow, it can be confusing. Consider renaming tonewAssetIdfor clarity:- assetId := extension.AssetId{ + newAssetId := extension.AssetId{ TxHash: txHashBytes, Index: uint16(i), }And update subsequent references within this block.
| if err := s.ensureAssetPresence(ctx, assetGroupList, *controlAssetID); err != nil { | ||
| return fmt.Errorf( | ||
| "cannot update asset metadata while control asset is being issued", | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ensureAssetPresence method definition
ast-grep --pattern $'func ($_ *service) ensureAssetPresence($$$) $_ {
$$$
}'Repository: arkade-os/arkd
Length of output: 1658
Remove or clarify the error message at lines 4401-4403.
The ensureAssetPresence method (in internal/core/application/asset_validation.go:657-676) validates that the control asset is present in the assetGroupList. However, the error message "cannot update asset metadata while control asset is being issued" does not match what the method actually checks—it returns errors when the asset is missing or no assets are provided, not when an asset is "being issued". Provide an error message that accurately reflects the validation failure.
🤖 Prompt for AI Agents
In `@internal/core/application/service.go` around lines 4400 - 4404, The error
message at the s.ensureAssetPresence callsite is misleading—change it to
accurately reflect that the validation failed because the control asset is
missing or the asset list is empty; update the returned error from the
s.ensureAssetPresence block to something like "control asset not present in
assetGroupList" and include the underlying error (wrap with %w) so callers see
the specific reason reported by ensureAssetPresence (keep reference to
s.ensureAssetPresence and the controlAssetID variable when constructing the
message).
… with `Txid` and `Index`, and standardize integer encoding/decoding using `tlv` helpers.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/core/application/asset_validation.go`:
- Around line 365-366: The call to validateTeleportInput is passing the wrong
teleport index; replace the uint32(input.Vin) argument with the teleport index
from the witness (uint32(input.Witness.Index)) so
validateTeleportInput(decodedProof, m.arkTx, *grpAsset.AssetId,
uint32(input.Witness.Index), input.Witness.Script) receives the correct index;
update the call site where decodedProof, m.arkTx, *grpAsset.AssetId, and
input.Witness.Script are provided (the existing call that currently uses
input.Vin).
In `@internal/core/application/service.go`:
- Around line 4452-4461: The loop over grpAsset.Inputs is passing the wrong
index to UpdateTeleportAsset: it currently uses in.Vin but teleport claims
encode their index in in.Witness.Index; update the call in the block that checks
in.Type == extension.AssetTypeTeleport to pass in.Witness.Index instead of
in.Vin (keep intentId and teleportScript as-is and still call
s.repoManager.Assets().UpdateTeleportAsset(ctx, teleportScript, intentId,
assetIdStr, in.Witness.Index, true) and preserve the existing error handling).
In `@pkg/ark-lib/go.mod`:
- Line 14: The go.mod pins github.com/lightningnetwork/lnd/tlv at v1.2.6; update
that module version to v1.3.2 in go.mod (replace v1.2.6 with v1.3.2 for
github.com/lightningnetwork/lnd/tlv), then run go mod tidy and rebuild/tests (go
build / go test) to ensure dependency resolution and that no breakage occurred
from the bump.
♻️ Duplicate comments (20)
pkg/ark-lib/offchain/tx_test.go (3)
133-138: Remove unusedcheckpointTxMapto avoid compile failure.
Line 133-138 declares and populatescheckpointTxMapbut never uses it; Go will fail the test build.🛠️ Proposed fix
- checkpointTxMap := make(map[string]string) - for _, cp := range checkpoints { - encoded, err := cp.B64Encode() - require.NoError(t, err) - checkpointTxMap[cp.UnsignedTx.TxHash().String()] = encoded - } + for _, cp := range checkpoints { + _, err := cp.B64Encode() + require.NoError(t, err) + }
140-149: FixassetGroupIndexto useoutputsNoAnchorindexing.
Line 140-149 setsassetGroupIndexusing the original output index, but you later index intooutputsNoAnchor.🛠️ Proposed fix
outputsNoAnchor := make([]*wire.TxOut, 0, len(arkTx.UnsignedTx.TxOut)-1) assetGroupIndex := -1 for idx, out := range arkTx.UnsignedTx.TxOut { if bytes.Equal(out.PkScript, txutils.ANCHOR_PKSCRIPT) { continue } outputsNoAnchor = append(outputsNoAnchor, out) if extension.ContainsAssetPacket(out.PkScript) { - assetGroupIndex = idx + assetGroupIndex = len(outputsNoAnchor) - 1 } }
171-175: Remove or userebuiltCheckpointIDs.
Line 171-175 buildsrebuiltCheckpointIDsbut never uses it, which will not compile.🛠️ Proposed fix (remove unused map)
- // Map rebuilt checkpoint txids for quick lookup. - rebuiltCheckpointIDs := make(map[string]struct{}) - for _, cp := range rebuiltCheckpoints { - rebuiltCheckpointIDs[cp.UnsignedTx.TxHash().String()] = struct{}{} - }pkg/ark-lib/extension/encoding.go (1)
218-247: Guard varint‑derived allocations to avoid OOM on malformed payloads.
Line 224/231/241/324/406 allocate slices directly from untrusted lengths/counts. A crafted payload can force huge allocations beforeReadFullfails. Add upper bounds (and remaining‑length checks when possible) beforemake(...).🛡️ Suggested guard pattern (apply to metadata/script/output lengths)
- count, err := tlv.ReadVarInt(r, scratch) + count, err := tlv.ReadVarInt(r, scratch) if err != nil { return nil, err } + if count > maxMetadataEntries { + return nil, fmt.Errorf("metadata count too large: %d", count) + } meta := make([]Metadata, count) for i := uint64(0); i < count; i++ { // Key kLen, err := tlv.ReadVarInt(r, scratch) if err != nil { return nil, err } + if kLen > maxFieldLen { + return nil, fmt.Errorf("metadata key too large: %d", kLen) + } kBytes := make([]byte, kLen)Also applies to: 293-343, 379-409
pkg/ark-lib/extension/asset.go (1)
94-119: Spec mismatch: localVout/Vinshould beuint16, notuint32.
Line 96 and Line 116 use 32‑bit indexes, which breaks wire‑format compatibility with the spec and withAssetId.Index/AssetRef.GroupIndex. Align touint16and update encoding/decoding helpers accordingly.🐛 Proposed fix
type AssetOutput struct { Type AssetType - Vout uint32 // For Local + Vout uint16 // For Local Script []byte // For Teleport Amount uint64 } @@ type AssetInput struct { Type AssetType - Vin uint32 // For Local + Vin uint16 // For Local Witness TeleportWitness // For Teleport Amount uint64 }internal/core/application/utils.go (2)
196-199: Add a length check before slicingPkScript.
Line 197 slicesout.PkScript[2:]without validating length or script type; short or non‑P2TR outputs can panic.🛡️ Proposed fix
- } else { - pubKey = hex.EncodeToString(out.PkScript[2:]) - isSubDust = script.IsSubDustScript(out.PkScript) - } + } else { + if len(out.PkScript) < 34 { + continue // skip non‑P2TR/short scripts + } + pubKey = hex.EncodeToString(out.PkScript[2:]) + isSubDust = script.IsSubDustScript(out.PkScript) + }
220-231: Asset→output matching is wrong after filtering anchor outputs.
Line 222 indexes intooutsusingasst.VOut, butoutsexcludes anchor outputs, so valid assets can be skipped or mis‑attached. Match by outpoint instead of slice index.🐛 Suggested fix
- // Add AssetGroup if Present - for _, asst := range assetList { - idx := int(asst.VOut) - if idx < 0 || idx >= len(outs) { - continue - } - - outs[idx].Assets = append(outs[idx].Assets, domain.Asset{ - AssetID: asst.AssetID, - Amount: asst.Amount, - }) - } + // Add AssetGroup if Present - match by outpoint + outByVout := make(map[uint32]*domain.Vtxo, len(outs)) + for i := range outs { + outByVout[outs[i].Outpoint.VOut] = &outs[i] + } + for _, asst := range assetList { + if v, ok := outByVout[asst.VOut]; ok { + v.Assets = append(v.Assets, domain.Asset{ + AssetID: asst.AssetID, + Amount: asst.Amount, + }) + } + }internal/core/application/asset_validation.go (4)
353-363: Guard against nil intent before decoding proof.
Line 359 dereferencesintent.Proofwithout checking ifGetIntentByTxidreturned nil, which can panic.🐛 Proposed fix
intent, err := s.repoManager.Rounds().GetIntentByTxid(context.Background(), txHash.String()) if err != nil { return errors.TELEPORT_VALIDATION_FAILED.New("error retrieving intent for teleport input validation: %w", err). WithMetadata(errors.TeleportValidationMetadata{}) } + if intent == nil { + return errors.TELEPORT_VALIDATION_FAILED.New( + "intent %s not found for teleport input validation", txHash.String(), + ).WithMetadata(errors.TeleportValidationMetadata{}) + } decodedProof, err := psbt.NewFromRawBytes(strings.NewReader(intent.Proof), true)
440-463: Enforce non‑zero asset amounts per spec.
Line 440+ aggregates amounts without checking for zero values; zero‑amount inputs/outputs should be rejected early.🛡️ Suggested checks
for _, in := range m.assetGp.Inputs { if in.Type == extension.AssetTypeTeleport { + if in.Amount == 0 { + return errors.ASSET_INPUT_INVALID.New("asset input amount must be > 0"). + WithMetadata(errors.AssetInputMetadata{}) + } teleportScriptHex := hex.EncodeToString(in.Witness.Script[:]) m.teleportInputs[teleportScriptHex] += in.Amount } } @@ for _, assetOut := range m.assetGp.Outputs { + if assetOut.Amount == 0 { + return errors.ASSET_OUTPUT_INVALID.New("asset output amount must be > 0"). + WithMetadata(errors.AssetOutputMetadata{}) + } switch assetOut.Type {
569-575: Avoid nil deref when comparingassetGroup.AssetId.
Line 574 dereferencesassetGroup.AssetIdwithout checking for nil; issuance groups can be nil and will panic.🐛 Proposed fix
- if assetOutput.Type == extension.AssetTypeTeleport && + if assetGroup.AssetId != nil && + assetOutput.Type == extension.AssetTypeTeleport && bytes.Equal(assetOutput.Script, script) && assetId == *assetGroup.AssetId && index == uint32(i) {
642-652: Filter prev‑out checks by asset group and local output type.
Line 648 matches byvout/amountacross all groups and types, so a local input can validate against a teleport output or a different asset group. Pass the expected asset ID and only consider local outputs for that group.💡 Sketch of fix
-func (s *service) verifyAssetInputPrevOut( - ctx context.Context, - input extension.AssetInput, - prev wire.OutPoint, -) error { +func (s *service) verifyAssetInputPrevOut( + ctx context.Context, + input extension.AssetInput, + prev wire.OutPoint, + assetID *extension.AssetId, +) error { @@ - for _, asset := range assetGroup.Assets { - totalAssetOuts = append(totalAssetOuts, asset.Outputs...) - } - - for _, assetOut := range totalAssetOuts { - if assetOut.Vout == prev.Index && input.Amount == assetOut.Amount { - return nil - } - } + for _, asset := range assetGroup.Assets { + if assetID == nil || asset.AssetId == nil || *asset.AssetId != *assetID { + continue + } + for _, assetOut := range asset.Outputs { + if assetOut.Type != extension.AssetTypeLocal { + continue + } + if assetOut.Vout == prev.Index && input.Amount == assetOut.Amount { + return nil + } + } + }internal/core/application/service.go (9)
355-357: Restore or removerestoreWatchingVtxosexplicitly.
The commented block suggests missing functionality after restart. Either re‑enable it or remove with an explanation.🛠️ Proposed fix (restore)
- // if err := svc.restoreWatchingVtxos(); err != nil { - // return nil, fmt.Errorf("failed to restore watching vtxos: %s", err) - // } + if err := svc.restoreWatchingVtxos(); err != nil { + return nil, fmt.Errorf("failed to restore watching vtxos: %s", err) + }
1131-1138: Asset storage failure is silently ignored.
Line 1131 logs errors but keeps the offchain tx accepted, risking permanent asset/tx divergence. Decide if storage is blocking or best‑effort and make that explicit.🛠️ Option A: fail fast
if assetOutputIndex >= 0 { if err := s.storeAssetDetailsFromArkTx( ctx, *arkPtx.UnsignedTx, assetOutputIndex, ); err != nil { - log.WithError(err).Errorf( - "failed to store asset details for offchain tx %s", txid, - ) + return nil, errors.INTERNAL_ERROR.New( + "failed to store asset details for offchain tx %s: %w", txid, err, + ) } }
1578-1587: Teleport asset insert failures should abort registration.
Line 1585 only logs and still appends a receiver, creating inconsistent state.🐛 Proposed fix
if err := s.repoManager.Assets().InsertTeleportAsset(ctx, domain.TeleportAsset{ IntentID: proofTxid, Script: teleportScript, OutputIndex: uint32(i), AssetID: asst.AssetId.ToString(), Amount: output.Amount, IsClaimed: false, }); err != nil { - log.WithError(err).Warn("failed to insert teleport asset") + return "", errors.INTERNAL_ERROR.New( + "failed to insert teleport asset: %w", err, + ).WithMetadata(map[string]any{ + "teleport_script": teleportScript, + "asset_id": asst.AssetId.ToString(), + }) }
1689-1691:offchainOutputsshould exclude on‑chain outputs.
Line 1689 appends every output tooffchainOutputs, which skews fee computation.🐛 Proposed fix
} else { ... hasOffChainReceiver = true rcv.PubKey = hex.EncodeToString(output.PkScript[2:]) + offchainOutputs = append(offchainOutputs, *output) } receivers = append(receivers, rcv) - offchainOutputs = append(offchainOutputs, *output)
1743-1747: Dead error check:errwas never set here.
Line 1743 checkserrafter callingGetArkPsbtFieldswith_, so the condition is unreachable. Capture the error or remove the block.🛠️ Suggested fix
- taptreeFields, _ := txutils.GetArkPsbtFields( + taptreeFields, err := txutils.GetArkPsbtFields( &proof.Packet, i+1, txutils.VtxoTaprootTreeField, ) - if err != nil { + if err != nil { return "", errors.INVALID_PSBT_INPUT.New( "failed to get asset seal field for input %d: %w", i+1, err, ).WithMetadata(errors.InputMetadata{Txid: proofTxid, InputIndex: i + 1}) }
4402-4405: Error message doesn’t match the actual validation.
Line 4402 wrapsensureAssetPresence, but the message claims “control asset is being issued.” Update the message to reflect “control asset missing in packet.”✏️ Suggested fix
if err := s.ensureAssetPresence(ctx, assetGroupList, *controlAssetID); err != nil { - return fmt.Errorf( - "cannot update asset metadata while control asset is being issued", - ) + return fmt.Errorf("control asset not present in asset group list: %w", err) }
4421-4430: Teleport outputs should be excluded from normal asset anchoring.
Line 4421 appends all outputs for existing assets, including teleport outputs, which should not be stored as normal assets.🐛 Proposed fix
for _, out := range asstGp.Outputs { + if out.Type == extension.AssetTypeTeleport { + continue + } asst := domain.NormalAsset{ Outpoint: domain.Outpoint{ Txid: arkTx.TxID(), VOut: uint32(out.Vout), },
4476-4493:DecreaseAssetGroupQuantitycan silently no‑op in SQL backends.
Line 4491 relies on repo behavior that may not error when balance is insufficient (0 rows updated), leading to silent inconsistency. Add explicit balance validation or detect zero‑row updates.
4521-4526:OutputVoutshould reflect the asset output’s vout, not loop index.
Line 4525 usesoutIdx, which can diverge from the encoded vout. UseassetOut.Voutfor consistency.🐛 Proposed fix
event := TeleportAsset{ TeleportHash: hex.EncodeToString(assetOut.Script), AnchorOutpoint: anchorOutpoint, AssetID: ast.AssetId.ToString(), - OutputVout: uint32(outIdx), + OutputVout: assetOut.Vout, CreatedAt: createdAt, ExpiresAt: expiresAt, }
| for _, in := range grpAsset.Inputs { | ||
| if in.Type != extension.AssetTypeTeleport { | ||
| continue | ||
| } | ||
|
|
||
| intentId := hex.EncodeToString(in.Witness.Txid[:]) | ||
| teleportScript := hex.EncodeToString(in.Witness.Script) | ||
| if err := s.repoManager.Assets().UpdateTeleportAsset(ctx, teleportScript, intentId, assetIdStr, in.Vin, true); err != nil { | ||
| log.WithError(err).Warn("failed to update teleport asset") | ||
| } |
There was a problem hiding this comment.
Teleport claim uses the wrong index field.
Line 4459 passes in.Vin to UpdateTeleportAsset, but teleport inputs encode their index in in.Witness.Index. This can mis‑mark claims.
🐛 Proposed fix
- if err := s.repoManager.Assets().UpdateTeleportAsset(ctx, teleportScript, intentId, assetIdStr, in.Vin, true); err != nil {
+ if err := s.repoManager.Assets().UpdateTeleportAsset(ctx, teleportScript, intentId, assetIdStr, in.Witness.Index, true); err != nil {
log.WithError(err).Warn("failed to update teleport asset")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, in := range grpAsset.Inputs { | |
| if in.Type != extension.AssetTypeTeleport { | |
| continue | |
| } | |
| intentId := hex.EncodeToString(in.Witness.Txid[:]) | |
| teleportScript := hex.EncodeToString(in.Witness.Script) | |
| if err := s.repoManager.Assets().UpdateTeleportAsset(ctx, teleportScript, intentId, assetIdStr, in.Vin, true); err != nil { | |
| log.WithError(err).Warn("failed to update teleport asset") | |
| } | |
| for _, in := range grpAsset.Inputs { | |
| if in.Type != extension.AssetTypeTeleport { | |
| continue | |
| } | |
| intentId := hex.EncodeToString(in.Witness.Txid[:]) | |
| teleportScript := hex.EncodeToString(in.Witness.Script) | |
| if err := s.repoManager.Assets().UpdateTeleportAsset(ctx, teleportScript, intentId, assetIdStr, in.Witness.Index, true); err != nil { | |
| log.WithError(err).Warn("failed to update teleport asset") | |
| } |
🤖 Prompt for AI Agents
In `@internal/core/application/service.go` around lines 4452 - 4461, The loop over
grpAsset.Inputs is passing the wrong index to UpdateTeleportAsset: it currently
uses in.Vin but teleport claims encode their index in in.Witness.Index; update
the call in the block that checks in.Type == extension.AssetTypeTeleport to pass
in.Witness.Index instead of in.Vin (keep intentId and teleportScript as-is and
still call s.repoManager.Assets().UpdateTeleportAsset(ctx, teleportScript,
intentId, assetIdStr, in.Witness.Index, true) and preserve the existing error
handling).
| github.com/btcsuite/btcd/btcutil/psbt v1.1.9 | ||
| github.com/btcsuite/btcd/chaincfg/chainhash v1.1.0 | ||
| github.com/btcsuite/btcwallet v0.16.10-0.20240718224643-db3a4a2543bd | ||
| github.com/lightningnetwork/lnd/tlv v1.2.6 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest release version github.com/lightningnetwork/lnd/tlv
💡 Result:
The latest released version of github.com/lightningnetwork/lnd/tlv is v1.3.2 (tag: tlv/v1.3.2), published June 10, 2025. [1][2]
Sources:
- pkg.go.dev listing for the tlv package (shows v1.3.2). [1]
- Release listing/tag index (tlv/v1.3.2 — Jun 10, 2025). [2]
🌐 Web query:
security advisories github.com/lightningnetwork/lnd/tlv CVE
💡 Result:
Here are known security advisories / CVEs affecting github.com/lightningnetwork/lnd (including parsing/ TLV/onion-related issues):
- CVE-2024-38359 — "lnd onion processing" DoS (patched in v0.17.0). [1]
- CVE-2022-39389 — witness block parsing DoS (patched in v0.15.4). [2]
- CVE-2022-44797 — btcd witness-size handling affecting lnd (patched in v0.15.2). [3]
- CVE-2020-26895 — improper validation accepting high-S signatures (patched in v0.10.0). [4]
- CVE-2019-12999 — improper access control (patched in v0.7.1). [5]
If you want, I can (a) fetch the lnd repository security/advisories page with direct GH advisory links, (b) show affected versions and mitigation steps for any specific CVE, or (c) search for additional CVEs referencing the lnd tlv package. Which would you prefer?
Update github.com/lightningnetwork/lnd/tlv to v1.3.2.
The pinned version v1.2.6 is outdated; v1.3.2 is the latest release. Consider updating to the current version to get latest fixes and features.
🤖 Prompt for AI Agents
In `@pkg/ark-lib/go.mod` at line 14, The go.mod pins
github.com/lightningnetwork/lnd/tlv at v1.2.6; update that module version to
v1.3.2 in go.mod (replace v1.2.6 with v1.3.2 for
github.com/lightningnetwork/lnd/tlv), then run go mod tidy and rebuild/tests (go
build / go test) to ensure dependency resolution and that no breakage occurred
from the bump.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/core/application/asset_validation.go`:
- Around line 484-505: When handling the case where outAmount > inAmount, guard
against a nil AssetId before calling m.assetGp.AssetId.ToString(): check if
m.assetGp == nil or m.assetGp.AssetId == nil and return an appropriate asset
validation error (e.g., errors.ASSET_VALIDATION_FAILED.New or
errors.ASSET_NOT_FOUND.New) with metadata instead of dereferencing; update the
block around m.assetGp.AssetId.ToString() to first validate the pointer and only
call ToString() when non-nil.
♻️ Duplicate comments (3)
internal/core/application/asset_validation.go (3)
353-363: Guard against nil intent before accessingintent.Proof.
GetIntentByTxidmay return(nil, nil)when the intent is not found. Accessingintent.Proofon line 359 will panic.🐛 Proposed fix
intent, err := s.repoManager.Rounds().GetIntentByTxid(context.Background(), txHash.String()) if err != nil { return errors.TELEPORT_VALIDATION_FAILED.New("error retrieving intent for teleport input validation: %w", err). WithMetadata(errors.TeleportValidationMetadata{}) } + if intent == nil { + return errors.TELEPORT_VALIDATION_FAILED.New( + "intent %s not found for teleport input validation", txHash.String(), + ).WithMetadata(errors.TeleportValidationMetadata{}) + } decodedProof, err := psbt.NewFromRawBytes(strings.NewReader(intent.Proof), true)
570-580: Guard against nilAssetIdin teleport output matching.Line 574 dereferences
assetGroup.AssetIdwithout checking for nil. For issuance groups in the intent proof,AssetIdis nil and this will panic.🐛 Proposed fix
for _, assetGroup := range assetPacket.Assets { + if assetGroup.AssetId == nil { + continue + } for i, assetOutput := range assetGroup.Outputs { if assetOutput.Type == extension.AssetTypeTeleport && bytes.Equal(assetOutput.Script, script) && assetId == *assetGroup.AssetId && index == uint32(i) {
642-655: Asset input validation can match outputs from different asset groups or teleport outputs.
verifyAssetInputPrevOutaggregates outputs from all asset groups and matches only byVoutandAmount, ignoring the asset ID and output type. This allows a local input to incorrectly validate against a teleport output or an output from a different asset group.🐛 Proposed fix: Filter by asset ID and local output type
The function needs the current asset ID to filter matches. Consider passing the asset ID as a parameter:
func (s *service) verifyAssetInputPrevOut( ctx context.Context, input extension.AssetInput, prev wire.OutPoint, + currentAssetId *extension.AssetId, ) error { // ... existing code to retrieve and decode offchain tx ... - totalAssetOuts := make([]extension.AssetOutput, 0) - for _, asset := range assetGroup.Assets { - totalAssetOuts = append(totalAssetOuts, asset.Outputs...) - } - - for _, assetOut := range totalAssetOuts { - if assetOut.Vout == prev.Index && input.Amount == assetOut.Amount { - return nil + for _, asset := range assetGroup.Assets { + // Skip groups with different asset IDs + if currentAssetId != nil && asset.AssetId != nil && *currentAssetId != *asset.AssetId { + continue + } + for _, assetOut := range asset.Outputs { + // Only match local outputs + if assetOut.Type == extension.AssetTypeLocal && + assetOut.Vout == prev.Index && + input.Amount == assetOut.Amount { + return nil + } } }
🧹 Nitpick comments (1)
internal/core/application/asset_validation.go (1)
447-464: Simplify vout lookup by direct bounds check.The inner loop searches for a matching index when a direct bounds check would suffice.
♻️ Suggested simplification
case extension.AssetTypeLocal: - for index, txout := range m.arkTx.TxOut { - if index == int(assetOut.Vout) { - m.localScriptAmts[hex.EncodeToString(txout.PkScript)] += assetOut.Amount - m.processed++ - break - } + if int(assetOut.Vout) >= len(m.arkTx.TxOut) { + return errors.ASSET_OUTPUT_INVALID.New("asset output vout %d out of range", assetOut.Vout). + WithMetadata(errors.AssetOutputMetadata{OutputIndex: int(assetOut.Vout)}) } + txout := m.arkTx.TxOut[assetOut.Vout] + m.localScriptAmts[hex.EncodeToString(txout.PkScript)] += assetOut.Amount + m.processed++
| if outAmount > inAmount { | ||
| // verify if extra amount is covered by existing input | ||
| if m.sumInputs == m.sumOutputs { | ||
| continue | ||
| } | ||
|
|
||
| assetID := m.assetGp.AssetId.ToString() | ||
| controlAssetDetails, err := s.repoManager.Assets().GetAssetGroupByID( | ||
| m.ctx, assetID, | ||
| ) | ||
| if err != nil { | ||
| return errors.ASSET_VALIDATION_FAILED.New( | ||
| "error retrieving asset %s: %w", | ||
| assetID, | ||
| err, | ||
| ).WithMetadata(errors.AssetValidationMetadata{AssetID: assetID}) | ||
| } | ||
|
|
||
| if controlAssetDetails == nil { | ||
| return errors.ASSET_NOT_FOUND.New("asset %s does not exist", assetID). | ||
| WithMetadata(errors.AssetValidationMetadata{AssetID: assetID}) | ||
| } |
There was a problem hiding this comment.
Guard against nil AssetId before dereferencing.
When outAmount > inAmount and sums don't match, line 490 calls m.assetGp.AssetId.ToString() without verifying AssetId is non-nil. For issuance groups, AssetId can be nil, causing a panic.
🐛 Proposed fix
if m.sumInputs == m.sumOutputs {
continue
}
+ if m.assetGp.AssetId == nil {
+ return errors.ASSET_VALIDATION_FAILED.New(
+ "cannot validate minting for issuance group without asset ID",
+ ).WithMetadata(errors.AssetValidationMetadata{})
+ }
assetID := m.assetGp.AssetId.ToString()🤖 Prompt for AI Agents
In `@internal/core/application/asset_validation.go` around lines 484 - 505, When
handling the case where outAmount > inAmount, guard against a nil AssetId before
calling m.assetGp.AssetId.ToString(): check if m.assetGp == nil or
m.assetGp.AssetId == nil and return an appropriate asset validation error (e.g.,
errors.ASSET_VALIDATION_FAILED.New or errors.ASSET_NOT_FOUND.New) with metadata
instead of dereferencing; update the block around m.assetGp.AssetId.ToString()
to first validate the pointer and only call ToString() when non-nil.
Arkade Asset Offchain Flow
This pull request implements the initial offchain asset processing flow for
Arkade Asset #855.
The goal of this change is to introduce the minimal primitives and logic required to represent,
validate, and index Arkade Assets in the offchain domain, forming the basis for later
on-chain flow.
Scope of Change
Canonical encoding and serialization rules for Arkade Asset objects.
Structural and semantic validation of asset definitions and transitions.
Storage and lookup of asset states and ownership.
Status
This PR establishes the first off-chain asset pipeline and is intentionally minimal.
Further work will extend this layer with on-chain flows.
Summary by CodeRabbit
New Features
API Enhancements
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.