feat: add capability to specify 'force' parameter on bulkd endpoint when creating transactions #939
Conversation
…hen creating transactions
WalkthroughThe changes introduce a new optional boolean property, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Controller
participant Bulker
participant Elements
participant LedgerController
Client->>API_Controller: POST /transactions (with force in body or query)
API_Controller->>API_Controller: Merge force from body/query
API_Controller->>Bulker: Create transaction with payload (force set)
Bulker->>Elements: TransactionRequest.ToCore()
Elements->>LedgerController: TxToScriptData(..., allowUnboundedOverdrafts=Force)
LedgerController-->>Elements: ScriptData
Elements-->>Bulker: CreateTransaction
Bulker-->>API_Controller: Result
API_Controller-->>Client: Response
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Fixes LX-51 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
pkg/client/models/operations/v2createtransaction.go (1)
17-18: Enhance the deprecation notice with migration guidance.The deprecation notice correctly indicates that the
Forcefield will be removed, but it doesn't specify what users should migrate to. Consider adding specific guidance about using theforcefield in the request body instead of as a query parameter.// - // Deprecated: This will be removed in a future release, please migrate away from it as soon as possible. + // Deprecated: This will be removed in a future release, please migrate to using the `force` field in the request body (V2PostTransaction.Force) instead of as a query parameter.pkg/client/docs/models/components/v2posttransaction.md (1)
15-15: Add description and example for theForcefield.The newly added
Forcefield lacks a description explaining its purpose and usage. Consider adding a clear description of what this field does (e.g., "Disable balance checks when creating transactions") and provide an example value (e.g.,true).- | `Force` | **bool* | :heavy_minus_sign: | N/A | | + | `Force` | **bool* | :heavy_minus_sign: | Disable balance checks when creating transactions. When set to true, transactions can bring accounts below zero. | true |pkg/client/docs/models/operations/v2createtransactionrequest.md (1)
11-11: Deprecation notice is clear but has minor formatting issues.The Markdown formatting for the deprecated
Forcefield correctly uses strikethrough and provides a clear warning notice, which helps users understand that they should migrate away from it. However, there are spaces inside the emphasis markers which may cause rendering issues in some Markdown parsers.-| ~~`Force`~~ | **bool* | :heavy_minus_sign: | : warning: ** DEPRECATED **: This will be removed in a future release, please migrate away from it as soon as possible.<br/><br/>Disable balance checks when passing postings | true | +| ~~`Force`~~ | **bool* | :heavy_minus_sign: | :warning: **DEPRECATED**: This will be removed in a future release, please migrate away from it as soon as possible.<br/><br/>Disable balance checks when passing postings | true |🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
11-11: Spaces inside emphasis markers
null(MD037, no-space-in-emphasis)
11-11: Spaces inside emphasis markers
null(MD037, no-space-in-emphasis)
docs/api/README.md (3)
517-518: Clarifyforcesupport in Bulk request parameters
The JSON example correctly shows the newforceflag at the element level, but the Bulk create endpoint’s parameters table (lines 524–532) does not mentionforce. If a top-levelforcequery parameter is supported, it should be documented here; otherwise add a note clarifying thatforceis only scoped per-element in the request body.
1333-1334: Demonstrateforceas a query parameter in code sample
The payload example includesforcein the body, but the endpoint also acceptsforcevia query string. Consider updating the first line of the HTTP request toPOST http://localhost:8080/v2/{ledger}/transactions?force=true HTTP/1.1to illustrate this alternative usage.
2726-2727: Add description for theforceproperty in the PostTransaction schema
The new table row forforcelacks a descriptive entry. Populate the Description column (e.g. “Disable balance checks when passing postings”) to match the query parameter’s explanation and improve discoverability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
openapi.yamlis excluded by!**/*.yamlopenapi/v2.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (9)
docs/api/README.md(7 hunks)internal/api/bulking/bulker.go(1 hunks)internal/api/bulking/elements.go(2 hunks)internal/api/v2/controllers_transactions_create.go(1 hunks)pkg/client/docs/models/components/v2posttransaction.md(1 hunks)pkg/client/docs/models/operations/v2createtransactionrequest.md(1 hunks)pkg/client/models/components/v2posttransaction.go(2 hunks)pkg/client/models/operations/v2createtransaction.go(1 hunks)test/e2e/api_bulk_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
internal/api/bulking/bulker.go (1)
internal/api/bulking/elements.go (1)
TransactionRequest(97-106)
internal/api/bulking/elements.go (2)
internal/controller/ledger/controller.go (1)
CreateTransaction(93-97)internal/controller/ledger/numscript.go (1)
TxToScriptData(17-124)
🪛 markdownlint-cli2 (0.17.2)
pkg/client/docs/models/operations/v2createtransactionrequest.md
11-11: Spaces inside emphasis markers
null
(MD037, no-space-in-emphasis)
11-11: Spaces inside emphasis markers
null
(MD037, no-space-in-emphasis)
🔇 Additional comments (9)
internal/api/bulking/bulker.go (1)
130-130:✅ Verification successful
Verify that the behavior remains consistent with the previous implementation.
The change from
ToCore(false)toToCore()reflects the updated method signature that now uses theForcefield from theTransactionRequeststruct instead of a parameter. This is a good refactoring that encapsulates the force flag within the request struct.
🏁 Script executed:
#!/bin/bash # Description: Verify the default value of the Force field in TransactionRequest struct # Let's check the implementation of ToCore() method to understand how the Force field is used rg -A 10 "func \(.*TransactionRequest\) ToCore" internal/api/bulking/elements.go # Also check if there are any tests that verify the behavior of this method rg "Test.*TransactionRequest.*ToCore" -g "*.go"Length of output: 456
I want to confirm exactly how the
Forcefield is defined and used inToCore()before wrapping up.
🏁 Script executed:
#!/bin/bash # Show the TransactionRequest struct definition (including json tags) rg -A 5 "type TransactionRequest" internal/api/bulking/elements.go # Show where and how req.Force is referenced inside ToCore() rg -A 5 "req\.Force" internal/api/bulking/elements.goLength of output: 708
I’d like to see the full
TransactionRequeststruct to confirm whereForceis declared and its default.
🏁 Script executed:
#!/bin/bash # Show the full TransactionRequest struct definition rg -A20 "type TransactionRequest" internal/api/bulking/elements.goLength of output: 911
Confirm default force behavior remains unchanged
The refactor from
ToCore(false)toToCore()now readsreq.Force(defaulting to Go’s zero‐valuefalsewhen omitted), so behavior with noforceflag set is identical to before.• internal/api/bulking/elements.go:
TransactionRequestdeclaresForce bool \json:"force"``ToCore()callsledgercontroller.TxToScriptData(txData, req.Force), preserving the previous hard-codedfalse.No further action required.
pkg/client/models/components/v2posttransaction.go (1)
67-67: LGTM! The field and getter implementation are well-structured.The new
Forcefield and its getter method follow the established patterns in the codebase:
- Uses pointer type for optional boolean
- Includes
omitemptyin the JSON tag- Implements a proper nil check in the getter
Also applies to: 129-135
test/e2e/api_bulk_test.go (1)
63-85: LGTM! Good test coverage for the new force parameter feature.The new test case correctly validates that bulk transaction creation works when the
forceparameter is set to true. It follows the existing test patterns and properly uses the pointer helper function for the boolean value.internal/api/v2/controllers_transactions_create.go (1)
37-44: Good implementation for backward compatibility.The implementation correctly handles both the query parameter and request payload versions of the
forceparameter, which provides backward compatibility while supporting the new bulk endpoint capability. The code comments clearly explain the rationale.Just a minor note: You might want to consider adding a deprecation notice in logs or response headers for the query parameter usage to encourage migration to the request payload approach, similar to the documentation changes.
internal/api/bulking/elements.go (2)
105-105: LGTM! Adding Force field to TransactionRequest struct.The new
Forceboolean field is properly added to the struct, allowing it to be parsed from JSON request bodies.
108-124: Simplification of method signature enhances API consistency.Removing the explicit parameter and using the struct field simplifies the API and makes it more consistent with how other parameters are handled. The implementation correctly passes the
Forcefield value toTxToScriptData.This change might require updates to any code that calls
ToCore()with an explicit parameter, but I can see from the other file changes that this has been addressed in the controllers.docs/api/README.md (3)
3450-3451: Bulk JSON schema example correctly showsforce
The snippet properly positions the newforceflag afteraccountMetadata, and the JSON example is valid and well-formatted.
3525-3526: BulkElement example updated withforce
The JSON example cleanly adds theforceflag within each element’s data block with correct indentation and no trailing commas.
3597-3598: BulkElementCreateTransaction example integratedforce
The snippet accurately includesforcein the transaction element’s data, consistent with other examples.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #939 +/- ##
==========================================
- Coverage 82.25% 82.21% -0.04%
==========================================
Files 141 141
Lines 7786 7788 +2
==========================================
- Hits 6404 6403 -1
- Misses 1057 1059 +2
- Partials 325 326 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -2077,6 +2077,7 @@ paths: | |||
| - name: force | |||
| in: query | |||
| description: Disable balance checks when passing postings | |||
| deprecated: true | |||
There was a problem hiding this comment.
Add in the description what replaces it :)
| @@ -818,6 +818,7 @@ paths: | |||
| - name: force | |||
| in: query | |||
| description: Disable balance checks when passing postings | |||
| deprecated: true | |||
| @@ -514,7 +514,8 @@ Accept: application/json | |||
| "property2": { | |||
| "admin": "true" | |||
| } | |||
| } | |||
| }, | |||
| "force": true | |||
There was a problem hiding this comment.
not sure it's worth the change, but I'd put it to false here to show it's the default (and as forcing something is dangerous, you don't want people to copy/paste this without knowing what that param does)
Fixes LX-51