fix: improve search of utxo by full asset & fail on missing policy id#992
Merged
Conversation
📝 WalkthroughWalkthroughThis PR updates the gRPC v1alpha and v1beta query implementations to unify how ChangesAssetPattern Query Index Logic
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
🧹 Nitpick comments (2)
src/serve/grpc/v1beta/query.rs (2)
170-178: 💤 Low valueRedundant intermediate wrappers in the combined-asset arm.
by_policyandby_assetare cloned into wrapper structs solely to enable the non-empty check viamaybe_from, but in the(Some(_), Some(_))arm both wrappers are discarded with_andself.policy_id/self.asset_nameare read again directly. Callingis_empty()directly avoids the allocations and makes the intent clearer.♻️ Proposed refactor
impl IntoSet for u5c::cardano::AssetPattern { fn into_set<S: CardanoIndexExt>(self, indexes: &S) -> Result<HashSet<TxoRef>, Status> { - let by_policy = ByPolicyQuery::maybe_from(self.policy_id.clone()); - let by_asset = ByAssetQuery::maybe_from(self.asset_name.clone()); - - match (by_policy, by_asset) { - (Some(_), Some(_)) => { - let mut subject = self.policy_id.to_vec(); - subject.extend_from_slice(&self.asset_name); - ByAssetQuery(bytes::Bytes::from(subject)).into_set(indexes) - } - (Some(x), None) => x.into_set(indexes), - (None, Some(_)) => Err(Status::invalid_argument( - "asset name query requires a policy_id", - )), - (None, None) => Ok(HashSet::default()), + match (!self.policy_id.is_empty(), !self.asset_name.is_empty()) { + (true, true) => { + let mut subject = self.policy_id.to_vec(); + subject.extend_from_slice(&self.asset_name); + ByAssetQuery(bytes::Bytes::from(subject)).into_set(indexes) + } + (true, false) => ByPolicyQuery(self.policy_id).into_set(indexes), + (false, true) => Err(Status::invalid_argument( + "asset name query requires a policy_id", + )), + (false, false) => Ok(HashSet::default()), } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/serve/grpc/v1beta/query.rs` around lines 170 - 178, The current code constructs ByPolicyQuery::maybe_from and ByAssetQuery::maybe_from only to check non-emptiness and then ignores those wrappers in the (Some(_), Some(_)) arm; instead, avoid allocations by checking the raw fields directly (use self.policy_id.is_empty() and self.asset_name.is_empty()) and only build the combined subject if both are non-empty, then create the bytes vector and call ByAssetQuery(bytes::Bytes::from(subject)).into_set(indexes); remove the initial ByPolicyQuery/ByAssetQuery wrapper variables and their maybe_from calls (refer to ByPolicyQuery, ByAssetQuery, maybe_from, self.policy_id, self.asset_name, and the final ByAssetQuery(...).into_set(indexes)).
168-186: ⚡ Quick winNo unit tests cover the two new
AssetPatterncode paths.The
(Some(_), Some(_))combined-query arm and the(None, Some(_))error arm both lack test coverage. Given that this is the primary functional change in the PR, tests for these paths would prevent regressions.Suggested cases:
policy_idnon-empty +asset_namenon-empty →utxos_by_assetis called with the concatenated subject.policy_idempty +asset_namenon-empty →Status::invalid_argument("asset name query requires a policy_id").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/serve/grpc/v1beta/query.rs` around lines 168 - 186, Add unit tests for the IntoSet impl for u5c::cardano::AssetPattern covering the two missing branches: create a test that constructs an AssetPattern with non-empty policy_id and non-empty asset_name and assert that ByAssetQuery is invoked (i.e., the resulting utxo set equals the expected result from ByAssetQuery when called with the concatenated subject) and another test that constructs an AssetPattern with empty policy_id and non-empty asset_name and asserts the function returns Err(Status::invalid_argument("asset name query requires a policy_id")). Locate the IntoSet implementation for u5c::cardano::AssetPattern and write tests that exercise AssetPattern.into_set<S: CardanoIndexExt>(indexes) using a mock or test implementation of CardanoIndexExt that lets you verify ByAssetQuery(bytes::Bytes::from(subject)).into_set(indexes) was used and that the error arm is returned for the empty-policy case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/serve/grpc/v1beta/query.rs`:
- Around line 170-178: The current code constructs ByPolicyQuery::maybe_from and
ByAssetQuery::maybe_from only to check non-emptiness and then ignores those
wrappers in the (Some(_), Some(_)) arm; instead, avoid allocations by checking
the raw fields directly (use self.policy_id.is_empty() and
self.asset_name.is_empty()) and only build the combined subject if both are
non-empty, then create the bytes vector and call
ByAssetQuery(bytes::Bytes::from(subject)).into_set(indexes); remove the initial
ByPolicyQuery/ByAssetQuery wrapper variables and their maybe_from calls (refer
to ByPolicyQuery, ByAssetQuery, maybe_from, self.policy_id, self.asset_name, and
the final ByAssetQuery(...).into_set(indexes)).
- Around line 168-186: Add unit tests for the IntoSet impl for
u5c::cardano::AssetPattern covering the two missing branches: create a test that
constructs an AssetPattern with non-empty policy_id and non-empty asset_name and
assert that ByAssetQuery is invoked (i.e., the resulting utxo set equals the
expected result from ByAssetQuery when called with the concatenated subject) and
another test that constructs an AssetPattern with empty policy_id and non-empty
asset_name and asserts the function returns Err(Status::invalid_argument("asset
name query requires a policy_id")). Locate the IntoSet implementation for
u5c::cardano::AssetPattern and write tests that exercise
AssetPattern.into_set<S: CardanoIndexExt>(indexes) using a mock or test
implementation of CardanoIndexExt that lets you verify
ByAssetQuery(bytes::Bytes::from(subject)).into_set(indexes) was used and that
the error arm is returned for the empty-policy case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7db97c6e-619b-4fba-bd05-24b2d8b18031
📒 Files selected for processing (2)
src/serve/grpc/v1alpha/query.rssrc/serve/grpc/v1beta/query.rs
scarmuega
approved these changes
May 14, 2026
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
2 participants
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.
Summary by CodeRabbit