-
Notifications
You must be signed in to change notification settings - Fork 559
feat: add batch transaction exports to public API #3064
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds new public re-exports: exposes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
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 (2)
packages/xrpl/src/index.ts (1)
20-20
: Prefer named re-exports to avoid leaking internals and to keep a stable public surfaceUsing a star export from './Wallet/batchSigner' may unintentionally re-export future helpers/types added to that module. Consider explicitly re-exporting only the intended public utilities.
Apply this diff:
-export * from './Wallet/batchSigner' +export { combineBatchSigners, signMultiBatch } from './Wallet/batchSigner'packages/xrpl/src/models/transactions/index.ts (1)
42-48
: Consider adding minimal API smoke tests for the new exportsTo guard against regressions in future refactors of barrels, add a test that imports these from the public entry and asserts their presence/types. Keeps the contract visible.
I can draft a lightweight test that imports from
packages/xrpl/src
(or the built package) and assertsBatch
,BatchFlags
,BatchInnerTransaction
, and the walletsignMultiBatch
/combineBatchSigners
are available. Want me to add it?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/xrpl/src/index.ts
(1 hunks)packages/xrpl/src/models/transactions/index.ts
(2 hunks)
🔇 Additional comments (3)
packages/xrpl/src/index.ts (1)
20-20
: Sanity-check complete: noBatchSigner
name collisions
- Inspected
packages/xrpl/src/Wallet/batchSigner.ts
exports via ripgrep: onlysignMultiBatch
andcombineBatchSigners
are exported.- No
BatchSigner
(type or value) is exported from that module, so it cannot collide with theBatchSigner
type re-exported bymodels/transactions
.packages/xrpl/src/models/transactions/index.ts (2)
1-7
: No Action Required: Symbol Kinds VerifiedBoth GlobalFlags and GlobalFlagsInterface are declared with the correct symbol kinds—GlobalFlags as an enum (value + type) and GlobalFlagsInterface as an interface (type only)—so they’ll behave correctly under verbatimModuleSyntax. All good here.
42-48
: BatchSigner naming overlap check
The initialrg
search inpackages/xrpl/src
for runtime exports ofBatchSigner
returned no matches, indicating no obvious class, const, function, or enum namedBatchSigner
is being exported elsewhere. However, absence of output isn’t definitive proof for default exports or more obscure patterns.Please manually confirm in
packages/xrpl/src/Wallet/batchSigner.ts
that:
- It does not default-export a runtime symbol called
BatchSigner
.- Any exported values (functions, constants, classes) use a different name or are type-only.
Once verified, there should be no naming conflict at the top-level barrel exports.
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.
Please update HISTORY.md
@mvadari Added. Please review. |
Thanks for your contribution @nabe3m - greatly appreciated! |
@LJ-XRPL Thanks so much for your comment! I’ll do my best to keep contributing, even in small ways. |
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.
LGTM
High Level Overview of Change
Add Batch transaction types and utilities to public API exports
Batch
,BatchFlags
,BatchFlagsInterface
,BatchInnerTransaction
, andBatchSigner
from transactions indexGlobalFlags
from common transaction types for use in batch inner transactionsbatchSigner
utilities (signMultiBatch
,combineBatchSigners
) in main package exportsContext of Change
This change addresses the need for external packages and applications to access batch transaction types, global transaction flags, and signing utilities that were previously internal to the library. Currently, developers trying to implement batch transaction handling must either:
By exposing these types and utilities through the public API, we enable:
GlobalFlags
enum for setting flags on batch inner transactions (e.g.,GlobalFlags.tfInnerBatchTxn
)This architecture was chosen because:
GlobalFlags
is essential for proper batch inner transaction constructionType of Change
Did you update HISTORY.md?
Test Plan
Existing Tests:
signMultiBatch
andcombineBatchSigners
verify functionalityvalidateBatch
ensure type safetyNew Test Coverage:
GlobalFlags
enum values are accessible for batch inner transaction flagsManual Testing:
Verification Steps:
npm test
to ensure all existing tests passnpm run build
GlobalFlags.tfInnerBatchTxn
is accessible for batch inner transactions