-
Notifications
You must be signed in to change notification settings - Fork 111
Add type TradeAsset to compatible Asset #1586
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughThis PR introduces a changeset entry for a patch release of Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/wise-rocks-peel.md (1)
1-5: Align changeset wording (and double‑check semver level)The changeset correctly targets
@xchainjs/xchain-mayachain-query, but you might want to mirror the actual alias name for clarity:-Add type TradeAsset to Compatible Asset types +Add type TradeAsset to the `CompatibleAsset` unionAlso, since this widens a public union type, it can introduce new TypeScript compile‑time errors for consumers that use exhaustive checks over
CompatibleAsset. If your project’s policy treats that as non‑breaking,patchis fine; otherwise consider whetherminorwould better match your semver conventions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/wise-rocks-peel.md(1 hunks)packages/xchain-mayachain-query/src/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/xchain-mayachain-query/src/types.ts (1)
packages/xchain-util/src/types/asset.ts (4)
Asset(27-32)TokenAsset(37-42)SynthAsset(47-52)TradeAsset(57-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
packages/xchain-mayachain-query/src/types.ts (1)
15-15: ExtendingCompatibleAssetwithTradeAssetis consistent and type-safeIncluding
TradeAssetinCompatibleAssetaligns with the rest of this file (trade-related types) and@xchainjs/xchain-util’s asset family, so the change looks correct and should let swap/fees/history handling accept trade assets where appropriate.One thing to keep in mind: widening a public union type can surface new TypeScript compile errors for consumers that rely on exhaustive
asset.typechecks (e.g.assertNeverpatterns). If your semver policy treats that as non‑breaking, this is fine as a patch; otherwise, you may want to confirm that a minor bump is acceptable for downstreams.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.