feat: Asset and Extension#139
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds asset persistence to a local store upon issuance, introduces a variadic options pattern for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AssetStore as Local AssetStore
participant TxStore as Transaction Store
participant Indexer
Client->>Client: IssueAsset(ctx, params)
Client->>Client: Save spend transaction
Client->>Client: persistIssuedAssets(issued_ids)
loop For each issued asset
Client->>AssetStore: UpsertAsset(assetId, assetInfo)
AssetStore-->>Client: Success (log errors if occur)
end
Client->>Indexer: GetTransactionHistory(ctx)
Indexer-->>Client: [Transactions with AssetPackets]
loop For each transaction with empty Assets
Client->>Client: deriveAssetsFromPacket(packet)
Client->>Client: Group by asset id, sum amounts
Client->>TxStore: Update Transaction.Assets
end
Client->>AssetStore: GetAssetDetails(ctx, assetId)
AssetStore-->>Client: AssetInfo
sequenceDiagram
participant Client
participant SDK as SDK SendOffChain
participant Options as Options Handler
participant ARK as ARK Protocol
Client->>SDK: SendOffChain(ctx, receivers, opts...)
SDK->>Options: applySendOffChainOptions(opts)
loop For each option
Options->>Options: Validate non-nil option
Options->>Options: Apply to sendOffChainOptions
end
Options-->>SDK: Error or success
alt Option error
SDK-->>Client: Return error immediately
else Success
SDK->>SDK: Build clientOpts with WithVtxos
alt Extension packets provided
SDK->>SDK: Append WithExtraCustomPacket
end
SDK->>ARK: Call ArkClient.SendOffChain(clientOpts)
ARK-->>SDK: Transaction result
SDK-->>Client: Return txid
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/e2e/utils_test.go (1)
198-217: Consider making the sleep configurable or adding retry logic.The hardcoded 2-second sleep could cause test flakiness under load or slow CI environments. For E2E tests this is often acceptable, but if flakiness occurs, consider a polling approach with timeout.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/utils_test.go` around lines 198 - 217, Replace the hardcoded time.Sleep in fetchExtensionFromVirtualTx with a polling/retry loop (or make the wait duration configurable) that repeatedly calls idxr.GetVirtualTxs until the tx appears or a timeout is reached; specifically, poll GetVirtualTxs for txid with a short interval and overall timeout, returning an error (or failing the test via require) if the timeout elapses before extension.NewExtensionFromTx succeeds, so tests are resilient in slow CI or under load.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client_internal_test.go`:
- Around line 136-137: Remove the extra consecutive blank line in
client_internal_test.go that causes gofmt to fail (the blank lines around lines
136-137); open the file and delete one of the two adjacent empty lines so the
file is formatted correctly and the gofmt/golangci-lint check will pass.
In `@client.go`:
- Around line 355-366: There is trailing whitespace on the blank line inside the
block that builds the result slice (around where order, result,
clientTypes.Asset and sums are used); open the function that constructs and
returns result, remove the trailing spaces on the empty line between the
len(order) check and the result := line so the file is gofmt-compliant, then
re-run formatting/linting.
In `@sdk.go`:
- Around line 18-19: The file sdk.go has consecutive blank lines (around lines
18-19) causing gofmt to fail; remove the extra blank line so there are no
duplicate blank lines in that area, then run gofmt/goimports (or go fmt) to
ensure formatting passes CI; update the commit with the cleaned sdk.go file.
---
Nitpick comments:
In `@test/e2e/utils_test.go`:
- Around line 198-217: Replace the hardcoded time.Sleep in
fetchExtensionFromVirtualTx with a polling/retry loop (or make the wait duration
configurable) that repeatedly calls idxr.GetVirtualTxs until the tx appears or a
timeout is reached; specifically, poll GetVirtualTxs for txid with a short
interval and overall timeout, returning an error (or failing the test via
require) if the timeout elapses before extension.NewExtensionFromTx succeeds, so
tests are resilient in slow CI or under load.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 099eee11-a8de-417d-9141-aa3c0e4d05c1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
asset.goclient.goclient_internal_test.gogo.modsdk.gosend.gotest/e2e/extension_test.gotest/e2e/utils_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client_internal_test.go (2)
212-214: Helper comment is slightly inaccurate vs implementation.Line 213-Line 214 says it constructs a “single fake non-nil AssetInput,” but
mustBuildPacketcreates one input per output and only for non-issuance groups. Consider updating the comment to match the actual behavior.Suggested wording tweak
-// It constructs groups using a single fake non-nil AssetInput so that the -// reissuance/burn invariants in asset.NewAssetGroup don't reject them. +// For non-issuance groups, it constructs matching inputs per output so that +// asset.NewAssetGroup input/output invariants are satisfied.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client_internal_test.go` around lines 212 - 214, Update the doc comment for mustBuildPacket to accurately describe its behavior: explain that it assembles an asset.Packet from group descriptions by creating one fake non-nil AssetInput per output (not a single input), and that these fake inputs are only created for non-issuance groups so asset.NewAssetGroup invariants aren’t rejected; reference mustBuildPacket and the behavior around "non-issuance groups" and "one input per output" so the comment matches the implementation.
189-203: Add a subtest for invalidtxidderivation fallback.
deriveAssetsFromPacketskips groups whenasset.NewAssetId(txid, groupIndex)fails (seeclient.go:347-353). Please add one test that passes an invalid txid and asserts the issuance-only packet yieldsnil, so this behavior is explicitly pinned.Proposed test addition
t.Run("issuance group derives id from txid and group index", func(t *testing.T) { t.Parallel() @@ require.Equal(t, uint64(20), got[0].Amount) }) + + t.Run("issuance group with invalid txid is skipped", func(t *testing.T) { + t.Parallel() + pkt := mustBuildPacket(t, []testGroup{ + {id: nil, outs: []uint64{5}}, + }) + require.Nil(t, deriveAssetsFromPacket(pkt, "not-a-valid-txid")) + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client_internal_test.go` around lines 189 - 203, Add a subtest to client_internal_test.go that verifies deriveAssetsFromPacket returns nil for issuance-only packets when asset.NewAssetId fails for an invalid txid: create an invalid txid (so asset.NewAssetId(txid, 0) will error), build an issuance-only packet with mustBuildPacket (one group with nil id and outputs), call deriveAssetsFromPacket(pkt, invalidTxid) and assert the result is nil/empty; this pins the fallback behavior in deriveAssetsFromPacket and references deriveAssetsFromPacket and asset.NewAssetId so reviewers can locate the relevant logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk.go`:
- Around line 51-53: The public SDK should not alias client.SendOption directly;
replace the SendOffChain signature to use a new SDK-owned type
SendOffChainOption (similar to InitOption/BatchSessionOption), remove the type
alias that exposes client.WithVtxos, and implement only SDK-supported helpers
(e.g., WithExtension) for SendOffChainOption so callers cannot pass unsupported
options like client.WithVtxos; update all call sites and the interface
declaration for SendOffChain(ctx context.Context, receivers
[]clientTypes.Receiver, opts ...SendOffChainOption) to accept the new type and
ensure send.go (where client.WithVtxos(vtxos) is prepended) still prepends vtxos
internally rather than relying on caller-provided vtxos.
---
Nitpick comments:
In `@client_internal_test.go`:
- Around line 212-214: Update the doc comment for mustBuildPacket to accurately
describe its behavior: explain that it assembles an asset.Packet from group
descriptions by creating one fake non-nil AssetInput per output (not a single
input), and that these fake inputs are only created for non-issuance groups so
asset.NewAssetGroup invariants aren’t rejected; reference mustBuildPacket and
the behavior around "non-issuance groups" and "one input per output" so the
comment matches the implementation.
- Around line 189-203: Add a subtest to client_internal_test.go that verifies
deriveAssetsFromPacket returns nil for issuance-only packets when
asset.NewAssetId fails for an invalid txid: create an invalid txid (so
asset.NewAssetId(txid, 0) will error), build an issuance-only packet with
mustBuildPacket (one group with nil id and outputs), call
deriveAssetsFromPacket(pkt, invalidTxid) and assert the result is nil/empty;
this pins the fallback behavior in deriveAssetsFromPacket and references
deriveAssetsFromPacket and asset.NewAssetId so reviewers can locate the relevant
logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80f200d8-6192-46d4-8b1a-7545661baf15
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
client.goclient_internal_test.gogo.modsdk.gostore/service_test.gostore/sql/migration/20260408120000_add_tx_assets_column.down.sqlstore/sql/migration/20260408120000_add_tx_assets_column.up.sqlstore/sql/sqlc/queries/models.gostore/sql/sqlc/queries/query.sql.gostore/sql/sqlc/query.sqlstore/sql/tx_store.gotest/e2e/extension_test.gotest/e2e/utils_test.go
✅ Files skipped from review due to trivial changes (4)
- store/sql/migration/20260408120000_add_tx_assets_column.up.sql
- store/sql/migration/20260408120000_add_tx_assets_column.down.sql
- store/sql/sqlc/queries/models.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (3)
- test/e2e/extension_test.go
- test/e2e/utils_test.go
- client.go
|
related to arkade-os/arkd#1003 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client.go`:
- Around line 317-329: The current fallback uses deriveAssetsFromPacket on
history[i].AssetPacket which sums all outputs and overstates assets when the
packet contains outputs not owned by this wallet; change the logic so the
fallback is only used when ownership is unambiguous (e.g., the packet's outputs
can be proven to belong exclusively to this wallet) or, preferably, stop relying
on this read-side reconstruction and ensure wallet-scoped Assets are populated
on write paths that persist Transaction.Assets; update the loop that touches
history[i].Assets (and any similar code between lines ~333-375) to skip
deriveAssetsFromPacket unless a clear ownership check succeeds (or add a TODO
and wire up the write-paths to set history[i].Assets during persistence) and
update any tests to cover a send-with-change case so the behavior is validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc7cef5e-8af4-4a96-ad93-35cec82f4427
📒 Files selected for processing (3)
client.gosdk.gostore/service_test.go
✅ Files skipped from review due to trivial changes (2)
- store/service_test.go
- sdk.go
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Arkana Code Review — arkade-os/go-sdk#139
Reviewed the full diff plus surrounding file context. This PR adds asset persistence at issuance, transaction history asset enrichment, a variadic options pattern for SendOffChain, GetAssetDetails, and supporting DB migrations. Solid work overall — detailed findings below.
🟡 Issues to address
1. Potential uint64 underflow removed, but zero-amount edge case introduced
client.go — handleArkTx (around the old line 1465, new switch block)
The old code had inAmount - outAmount which would panic/underflow if outAmount > inAmount. The new switch-based logic fixes that — good. However, when inAmount == outAmount, the result is amount = 0 with txType = TxSent. A zero-amount "sent" transaction will show up in the user's history. Consider either:
- Skipping the tx entirely when the delta is zero (it's a wash), or
- Adding a comment explaining why a zero-amount entry is intentional.
2. Redundant length check in persistIssuedAssets
asset.go:76 — if len(assetIds) > 0 is always true because line 63 already returns early when len(assetIds) == 0. Harmless but noisy — remove it or add a comment for the reader.
3. deriveAssetsFromPacket: uint16 cast of groupIndex
client.go (new deriveAssetsFromPacket function, around line 162):
derived, err := asset.NewAssetId(txid, uint16(groupIndex))groupIndex is an int from range pkt. If a packet ever had >65535 groups, this silently truncates. Extremely unlikely in practice, but a bounds check or a comment noting the protocol cap would harden this.
4. deriveAssetsFromPacket: uint64 addition without overflow check
client.go (line ~175):
sums[id] += out.AmountIf a malicious or buggy packet has amounts that sum past math.MaxUint64, this wraps silently. This is display-only (not protocol-critical), but could show wildly incorrect balances. A saturating add or overflow check would be defensive.
5. No upfront validation of reserved packet types in WithExtension
send_opts.go:23-26 — The docstring says type 0x00 is reserved and rejected by the underlying client, but the SDK layer doesn't validate this. The error only surfaces deep in the call stack. Consider adding an early check in WithExtension:
for _, p := range packets {
if p.Type() == asset.PacketType {
return fmt.Errorf("packet type 0x%02x is reserved for the asset packet", asset.PacketType)
}
}This gives SDK users a clear error at the call site, not buried in the protocol layer. The e2e test confirms the current behavior works, but fail-fast is better UX.
🔵 Cross-repo impact (important)
Breaking interface change: Adding GetAssetDetails and changing SendOffChain(ctx, receivers) → SendOffChain(ctx, receivers, opts...) on the ArkClient interface (sdk.go) is a breaking change for any code that provides its own implementation of ArkClient.
Known consumers:
- fulmine (
internal/core/application/service.go:89,pkg/swap/swap.go:40,pkg/swap/batch_handler.go:41) — embeds the concretearksdk.ArkClientreturned byNewArkClient/LoadArkClient, so it inherits the new methods automatically. No breakage expected unless fulmine has mock implementations in tests. - asset-demos/golang (
main.go:44,99) — uses the interface as a parameter type but only calls existing methods.SendOffChaincallers with no opts are backward-compatible. No breakage unless it implements the interface. - ark-simulator, cli — same pattern, should be fine.
Recommendation: Coordinate the go-sdk release with downstream consumers. If any repo has test mocks implementing ArkClient, they'll need updating.
🟢 Things done well
- Fail-safe persistence:
persistIssuedAssetslogs and swallows errors so a successful on-chain issuance isn't masked by a local store failure. Correct design. - Self-loop prevention: Clearing
ControlAssetIdon the first issued asset forNewControlAssetis clean and well-documented. - Backward-compatible enrichment: The
GetTransactionHistoryfallback toderiveAssetsFromPacketwhenAssetsis empty handles legacy rows gracefully. - Migration: Clean up/down migration for the
assetscolumn. Nullable TEXT is correct for backward compat. - Test coverage: Three new e2e tests (
TestSendWithExtraCustomPacket,TestGetAssetDetails,TestTransactionHistoryAssets) cover the happy paths, negative cases, and control-asset linkage.testTxsfixture updated for unit tests too. - Options pattern:
sendOffChainOptionswith nil-check and error return is idiomatic Go.
⚪ Protocol-critical assessment
This PR touches asset handling (issuance persistence, transaction history enrichment, extension packets) but does not modify VTXO signing, forfeit paths, round lifecycle, connector trees, or exit paths. The handleArkTx change to determine tx direction is the closest to protocol-adjacent code, and the new logic is strictly more correct than the old inAmount - outAmount (which could underflow).
Verdict: Not protocol-critical. No mandatory human sign-off required for this specific PR, though the interface change warrants coordination with downstream repos.
🤖 Reviewed by Arkana · #139
There was a problem hiding this comment.
LGTM with minor suggestions.
The five items flagged (zero-amount tx edge case, redundant len check, uint16 cast, uint64 overflow, early packet-type validation) are all non-blocking — none are protocol-critical or security issues. The cross-repo interface breakage is manageable since downstream consumers embed the concrete type.
Ship it once the minor feedback is addressed or explicitly deferred. 🚢
🤖 Arkana
1 participant
@altafan please review
Summary by CodeRabbit
Release Notes
New Features
GetAssetDetailsmethodTests