Add ignore_missing_asset_packets to SubmitTx#952
Open
sekulicd wants to merge 5 commits intoarkade-os:masterfrom
Open
Add ignore_missing_asset_packets to SubmitTx#952sekulicd wants to merge 5 commits intoarkade-os:masterfrom
sekulicd wants to merge 5 commits intoarkade-os:masterfrom
Conversation
Contributor
WalkthroughAdds an optional boolean flag to SubmitTx/SubmitOffchainTx that, when true, causes missing OP_RETURN asset packets to be silently ignored during asset validation; the flag is added to API specs, threaded through service and validator layers, and covered by unit and e2e tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as gRPC Handler
participant Service as Application Service
participant Validator as Asset Validator
Client->>Handler: SubmitTx(tx, ignoreMissingAssetPackets)
Handler->>Service: SubmitOffchainTx(checkpoints, signedArkTx, ignoreMissingAssetPackets)
Service->>Validator: validateAssetTransaction(tx, ext, inputAssets, ignoreMissingAssetPackets)
alt Asset packet present
Validator->>Validator: validate asset metadata
Validator-->>Service: success
else Asset packet missing & ignoreMissingAssetPackets=true
Validator->>Validator: detect "asset packet not found" -> suppress
Validator-->>Service: success (suppressed)
else Asset packet missing & ignoreMissingAssetPackets=false
Validator-->>Service: ASSET_VALIDATION_FAILED
else Other validation error
Validator-->>Service: ASSET_VALIDATION_FAILED
end
Service-->>Handler: AcceptedOffchainTx or error
Handler-->>Client: SubmitTxResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/core/application/asset_validation.go (1)
30-35: Avoid coupling suppression to raw error text.This branch currently relies on
strings.Contains(err.Error(), "asset packet not found"), which is fragile to upstream message changes and can accidentally broaden/narrow suppression behavior. Prefer a structured discriminator (typed error, metadata key, or dedicated predicate from the asset validation layer).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/asset_validation.go` around lines 30 - 35, The suppression branch currently matches on raw error text; replace that fragile check with a structured predicate or typed error from the asset validation layer (e.g. add and use an exported function like assetvalidation.IsAssetPacketNotFound(err) or an exported sentinel/type such as assetvalidation.ErrAssetPacketNotFound). Update the condition in the ignoreMissingAssetPackets block to call that predicate (instead of strings.Contains(err.Error(), "...")) while keeping the other checks (err.CodeName() == "ASSET_VALIDATION_FAILED"); ensure the asset validation package exports the predicate/type and import it where needed so the suppression relies on a stable API rather than raw message text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/application/asset_validation_test.go`:
- Line 93: The test currently ignores the error from hex.DecodeString when
creating malformedScript; change the call to capture and assert the error is nil
(e.g., check err and fail the test with t.Fatalf/t.Fatal or use require.NoError)
so a bad fixture hex string fails the test with a clear message; update the
hex.DecodeString invocation that assigns malformedScript to also capture err and
assert success before using malformedScript.
In `@internal/core/application/asset_validation.go`:
- Around line 40-42: Update the misleading comment for getAssetsFromTx: remove
the reference to an “early return above” and instead state succinctly that
getAssetsFromTx may return nil when no asset packet is present (for example,
when ignoreMissingAssetPackets is set), and that the subsequent loop that
consumes the result handles a nil map gracefully by not iterating. Refer to
getAssetsFromTx and the consumer loop so future readers understand the
nil-return behavior without implying unreachable control flow.
In `@internal/test/e2e/e2e_test.go`:
- Around line 5124-5126: The helper setupArkSDKwithPublicKey creates an SDK
client instance (returned as alice) but only the transport is closed; add an
explicit cleanup calling alice.Stop() (e.g., in the same t.Cleanup block where
grpcTransport.Close() is called) to ensure the SDK client is torn down between
subtests; update the test where alice, aliceWallet, _, grpcTransport :=
setupArkSDKwithPublicKey(t) is used to call t.Cleanup(func() { alice.Stop();
grpcTransport.Close() }) so alice.Stop() runs after the test.
---
Nitpick comments:
In `@internal/core/application/asset_validation.go`:
- Around line 30-35: The suppression branch currently matches on raw error text;
replace that fragile check with a structured predicate or typed error from the
asset validation layer (e.g. add and use an exported function like
assetvalidation.IsAssetPacketNotFound(err) or an exported sentinel/type such as
assetvalidation.ErrAssetPacketNotFound). Update the condition in the
ignoreMissingAssetPackets block to call that predicate (instead of
strings.Contains(err.Error(), "...")) while keeping the other checks
(err.CodeName() == "ASSET_VALIDATION_FAILED"); ensure the asset validation
package exports the predicate/type and import it where needed so the suppression
relies on a stable API rather than raw message text.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api-spec/protobuf/gen/ark/v1/indexer.pb.rgw.gois excluded by!**/gen/**api-spec/protobuf/gen/ark/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (8)
api-spec/openapi/swagger/ark/v1/service.openapi.jsonapi-spec/protobuf/ark/v1/service.protointernal/core/application/asset_validation.gointernal/core/application/asset_validation_test.gointernal/core/application/service.gointernal/core/application/types.gointernal/interface/grpc/handlers/arkservice.gointernal/test/e2e/e2e_test.go
| // pkg/ark-lib/asset/testdata/packet_fixtures.json. | ||
| // Decodes to: OP_RETURN <ARK 0x00 <1 group with out-of-bounds control | ||
| // asset group index>>. | ||
| malformedScript, _ := hex.DecodeString("6a0f41524b000102020100000101e80301") |
Contributor
There was a problem hiding this comment.
Don’t ignore fixture decode errors in tests.
If this hex string is accidentally edited into invalid data, the test may fail for the wrong reason. Assert decode success explicitly.
Suggested test hardening
- malformedScript, _ := hex.DecodeString("6a0f41524b000102020100000101e80301")
+ malformedScript, err := hex.DecodeString("6a0f41524b000102020100000101e80301")
+ require.NoError(t, 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.
Suggested change
| malformedScript, _ := hex.DecodeString("6a0f41524b000102020100000101e80301") | |
| malformedScript, err := hex.DecodeString("6a0f41524b000102020100000101e80301") | |
| require.NoError(t, err) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/application/asset_validation_test.go` at line 93, The test
currently ignores the error from hex.DecodeString when creating malformedScript;
change the call to capture and assert the error is nil (e.g., check err and fail
the test with t.Fatalf/t.Fatal or use require.NoError) so a bad fixture hex
string fails the test with a clear message; update the hex.DecodeString
invocation that assigns malformedScript to also capture err and assert success
before using malformedScript.
Comment on lines
+5124
to
+5126
| alice, aliceWallet, _, grpcTransport := setupArkSDKwithPublicKey(t) | ||
| t.Cleanup(func() { grpcTransport.Close() }) | ||
|
|
Contributor
There was a problem hiding this comment.
Add explicit SDK client teardown in the helper.
Line 5124 creates an SDK client, but only the transport is closed. Add alice.Stop() cleanup to prevent resource leakage between subtests.
💡 Proposed fix
- alice, aliceWallet, _, grpcTransport := setupArkSDKwithPublicKey(t)
- t.Cleanup(func() { grpcTransport.Close() })
+ alice, aliceWallet, _, grpcTransport := setupArkSDKwithPublicKey(t)
+ t.Cleanup(func() { alice.Stop() })
+ t.Cleanup(func() { grpcTransport.Close() })📝 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.
Suggested change
| alice, aliceWallet, _, grpcTransport := setupArkSDKwithPublicKey(t) | |
| t.Cleanup(func() { grpcTransport.Close() }) | |
| alice, aliceWallet, _, grpcTransport := setupArkSDKwithPublicKey(t) | |
| t.Cleanup(func() { alice.Stop() }) | |
| t.Cleanup(func() { grpcTransport.Close() }) | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/test/e2e/e2e_test.go` around lines 5124 - 5126, The helper
setupArkSDKwithPublicKey creates an SDK client instance (returned as alice) but
only the transport is closed; add an explicit cleanup calling alice.Stop()
(e.g., in the same t.Cleanup block where grpcTransport.Close() is called) to
ensure the SDK client is torn down between subtests; update the test where
alice, aliceWallet, _, grpcTransport := setupArkSDKwithPublicKey(t) is used to
call t.Cleanup(func() { alice.Stop(); grpcTransport.Close() }) so alice.Stop()
runs after the test.
Contributor
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 `@internal/core/application/service.go`:
- Line 1965: The call to s.validateAssetTransaction is written as a long
one-liner and fails the golines formatter; reformat the call to wrap arguments
over multiple lines for readability and linelength compliance by placing the
arguments (ctx, proof.UnsignedTx, ext, assetInputs, false) each on their own
line (or break across two lines) and put the closing parenthesis on its own line
so the call to s.validateAssetTransaction(ctx, proof.UnsignedTx, ext,
assetInputs, false) retains the same behavior but satisfies formatting checks.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/core/application/asset_validation.gointernal/core/application/service.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/core/application/asset_validation.go
internal/core/application/service.go
Outdated
|
|
||
| // validate assets | ||
| if err := s.validateAssetTransaction(ctx, proof.UnsignedTx, ext, assetInputs); err != nil { | ||
| if err := s.validateAssetTransaction(ctx, proof.UnsignedTx, ext, assetInputs, false); err != nil { |
Contributor
There was a problem hiding this comment.
Line 1965: Reformat this call to fix the current golines CI failure.
Functionally this is correct (false keeps intent registration strict), but the one-liner is failing formatting checks.
Proposed formatting fix
- if err := s.validateAssetTransaction(ctx, proof.UnsignedTx, ext, assetInputs, false); err != nil {
+ if err := s.validateAssetTransaction(
+ ctx, proof.UnsignedTx, ext, assetInputs, false,
+ ); err != nil {
return "", 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.
Suggested change
| if err := s.validateAssetTransaction(ctx, proof.UnsignedTx, ext, assetInputs, false); err != nil { | |
| if err := s.validateAssetTransaction( | |
| ctx, proof.UnsignedTx, ext, assetInputs, false, | |
| ); err != nil { | |
| return "", err | |
| } |
🧰 Tools
🪛 GitHub Actions: ci_unit
[error] 1965-1965: File is not properly formatted (golines)
🪛 GitHub Check: unit tests
[failure] 1965-1965:
File is not properly formatted (golines)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/application/service.go` at line 1965, The call to
s.validateAssetTransaction is written as a long one-liner and fails the golines
formatter; reformat the call to wrap arguments over multiple lines for
readability and linelength compliance by placing the arguments (ctx,
proof.UnsignedTx, ext, assetInputs, false) each on their own line (or break
across two lines) and put the closing parenthesis on its own line so the call to
s.validateAssetTransaction(ctx, proof.UnsignedTx, ext, assetInputs, false)
retains the same behavior but satisfies formatting checks.
After merging master, validateAssetTransaction gained an extension.Extension parameter (from PR arkade-os#946). Update all 3 test calls to pass extension.Extension{} and redesign the "other asset error" test to work with the extension-based architecture instead of the old OP_RETURN parsing approach. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/core/application/asset_validation_test.go (1)
76-111: Consider deduplicating the repeated dummy P2TR script fixture.The same long byte slice appears twice; extracting it into a small helper/const would reduce noise and avoid accidental drift between fixtures.
Also applies to: 151-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/application/asset_validation_test.go` around lines 76 - 111, Extract the repeated long PkScript byte slice into a single shared constant or small helper so both fixtures reuse it (e.g., define dummyP2TRScript = []byte{...} or func makeDummyP2TR() []byte) and replace the two inline occurrences in the tests with that symbol; update references where the same slice appears (the PkScript fields around the shown block and the other occurrence at lines ~151-157) to use the new constant/helper to avoid duplication and drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/application/asset_validation_test.go`:
- Around line 116-120: The test helper txWithInvalidAssetPacket and its comment
indicate the case should trigger ASSET_OUTPUT_INVALID when the extension
references a non-existent vout, but the assertions currently check for an
input-asset mismatch message; update the test to assert the ASSET_OUTPUT_INVALID
condition instead: locate the test that calls validateAssetTransaction (and uses
ignoreMissingAssetPackets) and change the expectation to look for the
ASSET_OUTPUT_INVALID error (or the specific ErrAssetOutputInvalid sentinel/type
or its canonical message) rather than "not present in the packet", or
alternatively adjust the helper/comment if you intended to test the
input-asset-mismatch branch; ensure the assertion references
txWithInvalidAssetPacket and validateAssetTransaction so the fixture, comment,
and assertion are consistent.
---
Nitpick comments:
In `@internal/core/application/asset_validation_test.go`:
- Around line 76-111: Extract the repeated long PkScript byte slice into a
single shared constant or small helper so both fixtures reuse it (e.g., define
dummyP2TRScript = []byte{...} or func makeDummyP2TR() []byte) and replace the
two inline occurrences in the tests with that symbol; update references where
the same slice appears (the PkScript fields around the shown block and the other
occurrence at lines ~151-157) to use the new constant/helper to avoid
duplication and drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f179dff-eb99-45f9-a9df-537e96f198f0
📒 Files selected for processing (1)
internal/core/application/asset_validation_test.go
Comment on lines
+116
to
+120
| // txWithInvalidAssetPacket builds a wire.MsgTx together with an | ||
| // extension.Extension whose asset packet references an output index (vout=99) | ||
| // that does not exist in the transaction. This triggers an | ||
| // ASSET_OUTPUT_INVALID error during validateAssetTransaction, which must NOT | ||
| // be suppressed by the ignoreMissingAssetPackets flag. |
Contributor
There was a problem hiding this comment.
Test intent and assertions are currently misaligned.
The helper comment says this path should trigger ASSET_OUTPUT_INVALID, but the assertion checks for "not present in the packet" (input-asset mismatch). Please align either the fixture intent/comments or the assertions so the test states exactly what branch it validates.
Suggested alignment (comment + stronger assertion)
-// extension.Extension whose asset packet references an output index (vout=99)
-// that does not exist in the transaction. This triggers an
-// ASSET_OUTPUT_INVALID error during validateAssetTransaction, which must NOT
-// be suppressed by the ignoreMissingAssetPackets flag.
+// extension.Extension carrying a syntactically valid packet used to trigger a
+// non-"asset packet not found" validation error, which must NOT be suppressed
+// by the ignoreMissingAssetPackets flag.
@@
require.NotContains(t, err.Error(), "asset packet not found",
"error should be a different asset validation error, not 'not found'")
+ require.Equal(t, "ASSET_INPUT_INVALID", err.CodeName(),
+ "error should come from non-missing-packet validation path")
require.Contains(t, err.Error(), "not present in the packet",
"error should indicate asset input mismatch with the packet")Also applies to: 209-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/application/asset_validation_test.go` around lines 116 - 120,
The test helper txWithInvalidAssetPacket and its comment indicate the case
should trigger ASSET_OUTPUT_INVALID when the extension references a non-existent
vout, but the assertions currently check for an input-asset mismatch message;
update the test to assert the ASSET_OUTPUT_INVALID condition instead: locate the
test that calls validateAssetTransaction (and uses ignoreMissingAssetPackets)
and change the expectation to look for the ASSET_OUTPUT_INVALID error (or the
specific ErrAssetOutputInvalid sentinel/type or its canonical message) rather
than "not present in the packet", or alternatively adjust the helper/comment if
you intended to test the input-asset-mismatch branch; ensure the assertion
references txWithInvalidAssetPacket and validateAssetTransaction so the fixture,
comment, and assertion are consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This closes #942
@altafan @Kukks please review.
Summary by CodeRabbit
New Features
Behavior Changes
Tests