-
Notifications
You must be signed in to change notification settings - Fork 123
feat: allow to specify additional metadata when reverting transactions #990
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
WalkthroughThis change extends the transaction revert functionality to accept optional user-provided metadata in the API request. The metadata is propagated through the controller and client layers, included in the reversed transaction, and documented in the OpenAPI and client documentation. Corresponding tests are updated to verify metadata handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Controller
participant Ledger
Client->>API: POST /v2/{ledger}/transactions/{id}/revert (with optional metadata)
API->>Controller: Call RevertTransaction (pass metadata)
Controller->>Ledger: Create reversed transaction (include metadata)
Ledger-->>Controller: Revert transaction result
Controller-->>API: Response
API-->>Client: Response (reverted transaction with metadata)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected related to the objectives from the linked issues. Possibly related PRs
Suggested reviewers
Poem
✨ 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. 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
|
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: 1
🧹 Nitpick comments (4)
internal/api/v2/controllers_transactions_revert.go (2)
27-29
: Improve struct naming for better readability.The struct name
X
is not descriptive. Consider using a more meaningful name to improve code readability.- type X struct { + type RevertTransactionRequestBody struct { Metadata metadata.Metadata `json:"metadata,omitempty"` }
31-37
: Clarify error message for optional metadata.The error message "expected JSON body with metadata" might be misleading as it suggests metadata is required, when it's actually optional. Consider making the message clearer.
- api.BadRequest(w, common.ErrValidation, errors.New("expected JSON body with metadata")) + api.BadRequest(w, common.ErrValidation, errors.New("invalid JSON body format"))docs/api/README.md (2)
1663-1672
: Example payload could mention emptiness to stress “optional”.Because the body is truly optional (
required = false
further down), it may help future readers if the example shows either:
- an empty object
{}
or- a comment/note right above the snippet clarifying that the body can be omitted or left empty.
This avoids the impression that
"metadata"
must always be supplied.
1674-1684
: Document the body parameter description.The parameters table lists
body
with an empty description (“none”). Add something like “Optional object allowing additional metadata for the reversed transaction.” to make the purpose self-contained.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
openapi.yaml
is excluded by!**/*.yaml
openapi/v2.yaml
is excluded by!**/*.yaml
pkg/client/.speakeasy/gen.lock
is excluded by!**/*.lock
,!**/*.lock
pkg/client/.speakeasy/logs/naming.log
is excluded by!**/*.log
,!**/*.log
pkg/client/speakeasyusagegen/.speakeasy/logs/naming.log
is excluded by!**/*.log
,!**/*.log
📒 Files selected for processing (10)
docs/api/README.md
(3 hunks)internal/api/v2/controllers_transactions_revert.go
(2 hunks)internal/controller/ledger/controller.go
(1 hunks)internal/controller/ledger/controller_default.go
(1 hunks)pkg/client/docs/models/components/v2reverttransactionrequest.md
(1 hunks)pkg/client/docs/models/operations/v2reverttransactionrequest.md
(1 hunks)pkg/client/models/components/v2reverttransactionrequest.go
(1 hunks)pkg/client/models/operations/v2reverttransaction.go
(2 hunks)pkg/client/v2.go
(2 hunks)test/e2e/api_transactions_revert_test.go
(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
internal/controller/ledger/controller_default.go (2)
internal/machine/vm/machine.go (1)
Metadata
(51-51)internal/metadata.go (1)
MarkReverts
(20-22)
internal/controller/ledger/controller.go (1)
internal/machine/vm/machine.go (1)
Metadata
(51-51)
internal/api/v2/controllers_transactions_revert.go (3)
internal/machine/vm/machine.go (1)
Metadata
(51-51)internal/api/common/errors.go (1)
ErrValidation
(17-17)internal/controller/ledger/controller.go (1)
RevertTransaction
(99-104)
pkg/client/models/components/v2reverttransactionrequest.go (1)
pkg/client/models/operations/v2reverttransaction.go (1)
V2RevertTransactionRequest
(11-23)
pkg/client/models/operations/v2reverttransaction.go (1)
pkg/client/models/components/v2reverttransactionrequest.go (1)
V2RevertTransactionRequest
(5-7)
test/e2e/api_transactions_revert_test.go (6)
pkg/client/models/operations/v2reverttransaction.go (2)
V2RevertTransactionResponse
(78-82)V2RevertTransactionRequest
(11-23)pkg/client/models/components/v2reverttransactionrequest.go (1)
V2RevertTransactionRequest
(5-7)pkg/client/ledger.go (1)
Ledger
(20-27)pkg/client/v2.go (1)
V2
(20-24)pkg/client/models/operations/v2createtransaction.go (1)
V2CreateTransactionResponse
(62-66)pkg/client/models/operations/v2gettransaction.go (1)
V2GetTransactionRequest
(12-19)
🪛 markdownlint-cli2 (0.17.2)
docs/api/README.md
4562-4562: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
🔇 Additional comments (14)
pkg/client/docs/models/operations/v2reverttransactionrequest.md (1)
12-13
: Documentation looks good.The addition of the
V2RevertTransactionRequest
field properly documents the new request body parameter for the revert transaction operation.internal/controller/ledger/controller.go (1)
103-103
: Good addition of metadata support.The Metadata field addition is consistent with the existing pattern in the codebase and properly typed as
metadata.Metadata
.internal/controller/ledger/controller_default.go (1)
447-447
: Correct implementation of metadata propagation.The change properly uses the input metadata (
parameters.Input.Metadata
) while preserving the revert marking functionality throughledger.MarkReverts()
. This enables users to provide additional metadata when reverting transactions.pkg/client/models/components/v2reverttransactionrequest.go (2)
5-7
: Well-structured request model.The struct definition is clean and follows Go conventions with proper JSON tags and the
omitempty
option for the optional metadata field.
9-14
: Proper nil-safe getter implementation.The
GetMetadata()
method correctly handles nil receiver cases and provides safe access to the metadata field.pkg/client/models/operations/v2reverttransaction.go (2)
21-23
: LGTM - Generated code follows correct patterns.The new field is properly typed with the correct request tag for JSON media type and follows the established struct field patterns.
71-76
: LGTM - Getter method follows established pattern.The getter method implementation is consistent with other getter methods in the file, properly handling nil cases.
internal/api/v2/controllers_transactions_revert.go (1)
40-47
: LGTM - Metadata properly passed to controller.The metadata is correctly extracted from the request body and passed to the RevertTransaction command parameters.
test/e2e/api_transactions_revert_test.go (3)
136-142
: LGTM - Proper test setup for metadata functionality.The introduction of the
request
variable allows for flexible configuration of the revert transaction request, enabling testing of the new metadata feature.
143-152
: LGTM - Request structure properly updated.The revert transaction call is correctly updated to include the request body, maintaining backward compatibility while enabling the new metadata functionality.
252-272
: LGTM - Comprehensive test coverage for additional metadata.The test properly verifies that user-provided metadata is merged with system metadata (the revert reference) in the resulting transaction. This ensures the feature works as expected.
pkg/client/v2.go (1)
4022-4046
: LGTM! Well-implemented metadata support for revert transactions.The changes correctly implement the new metadata functionality:
- Request body serialization: Properly added
utils.SerializeRequestBody
call with the newV2RevertTransactionRequest
type- HTTP semantics: Correctly passes
bodyReader
to the POST request and sets Content-Type header when needed- Consistency: Follows the same pattern as other methods in the file that accept request bodies
- Backward compatibility: The
true
parameter suggests this is optional, maintaining compatibility with existing clientsThe implementation aligns well with the PR objective to allow additional metadata when reverting transactions.
docs/api/README.md (2)
1654-1659
: Good catch –Content-Type
header is now explicit.Explicitly declaring
Content-Type: application/json
for the revert endpoint brings it in line with the other JSON-payload POST requests documented in this file. No action required.
4545-4568
: Duplicate heading triggers MD024; rename or fold into existing section.
### V2RevertTransactionRequest
appears twice (once via the TOC boilerplate, once as the actual heading).
Markdown-lint (MD024
) flags this as a duplicate heading. Either:-### Properties +### Properties <!-- V2RevertTransactionRequest properties -->or collapse the auto-generated duplicate if your doc-generation tool allows it.
|
||
| Field | Type | Required | Description | | ||
| ------------------- | ------------------- | ------------------- | ------------------- | | ||
| `Metadata` | map[string]*string* | :heavy_minus_sign: | N/A | |
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.
Fix type documentation inconsistency.
The documentation shows the Metadata field type as map[string]*string*
but the actual Go code defines it as map[string]string
. The documentation should be corrected to match the implementation.
🤖 Prompt for AI Agents
In pkg/client/docs/models/components/v2reverttransactionrequest.md at line 8,
the type for the Metadata field is incorrectly documented as
map[string]*string*. Update this to map[string]string to match the actual Go
code implementation and ensure consistency between the documentation and code.
075881f
to
8a4d82a
Compare
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: 2
🧹 Nitpick comments (2)
docs/api/README.md (2)
1668-1674
: Clarify ‘Content-Type’ header necessity
Content-Type: application/json
is now hard-coded in the sample request even though the request body is optional. Readers might assume the header is mandatory. Consider adding a short note (or conditionally showing the header) to avoid confusion when no body is sent.
4571-4594
: Rename duplicate “### Properties” heading to silence MD024
markdownlint
flags a duplicate heading at line 4588. Renaming this local “Properties” heading avoids the clash without altering meaning.-### Properties +### Properties (V2RevertTransactionRequest)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
internal/storage/bucket/migrations/41-index-transactions-amounts/notes.yaml
is excluded by!**/*.yaml
openapi.yaml
is excluded by!**/*.yaml
openapi/v2.yaml
is excluded by!**/*.yaml
pkg/client/.speakeasy/gen.lock
is excluded by!**/*.lock
,!**/*.lock
pkg/client/.speakeasy/logs/naming.log
is excluded by!**/*.log
,!**/*.log
pkg/client/speakeasyusagegen/.speakeasy/logs/naming.log
is excluded by!**/*.log
,!**/*.log
📒 Files selected for processing (11)
docs/api/README.md
(3 hunks)internal/api/v2/controllers_transactions_revert.go
(2 hunks)internal/controller/ledger/controller.go
(1 hunks)internal/controller/ledger/controller_default.go
(1 hunks)internal/storage/bucket/migrations/41-index-transactions-amounts/up.sql
(1 hunks)pkg/client/docs/models/components/v2reverttransactionrequest.md
(1 hunks)pkg/client/docs/models/operations/v2reverttransactionrequest.md
(1 hunks)pkg/client/models/components/v2reverttransactionrequest.go
(1 hunks)pkg/client/models/operations/v2reverttransaction.go
(2 hunks)pkg/client/v2.go
(2 hunks)test/e2e/api_transactions_revert_test.go
(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/client/models/components/v2reverttransactionrequest.go
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/client/docs/models/components/v2reverttransactionrequest.md
- internal/controller/ledger/controller.go
- pkg/client/docs/models/operations/v2reverttransactionrequest.md
- internal/controller/ledger/controller_default.go
- internal/api/v2/controllers_transactions_revert.go
- test/e2e/api_transactions_revert_test.go
- pkg/client/models/operations/v2reverttransaction.go
- pkg/client/v2.go
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/api/README.md
4588-4588: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (1)
docs/api/README.md (1)
1677-1698
: 👍 Body parameter definition looks correctThe example payload and the new
body
parameter row correctly describe an optionalmetadata
object and reference the new schema. No issues spotted here.
select '{1}'; | ||
$$; | ||
|
||
create index transactions_amounts on transactions using gin ((compute_transactions_amounts(postings::jsonb))); |
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.
🛠️ Refactor suggestion
Index creation should be postponed until the function is useful
transactions_amounts
is a GIN index over the constant result of the above stub. It will never be used by the planner, wastes disk, and slows down INSERT/UPDATE on transactions
. Recommend removing or deferring this statement until compute_transactions_amounts
produces discriminative keys.
🤖 Prompt for AI Agents
In internal/storage/bucket/migrations/41-index-transactions-amounts/up.sql at
line 14, the GIN index creation on transactions_amounts is premature because the
compute_transactions_amounts function currently returns a constant stub, making
the index unused and inefficient. Remove or comment out this index creation
statement for now, and only add it back once compute_transactions_amounts
produces meaningful, discriminative keys that justify the index.
create function compute_transactions_amounts(postings jsonb) | ||
returns numeric[] | ||
language sql | ||
immutable | ||
as | ||
$$ | ||
select '{1}'; | ||
$$; |
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.
💡 Verification agent
🧩 Analysis chain
compute_transactions_amounts
is a no-op stub; index will be meaningless
The function is declared IMMUTABLE
yet returns the hard-coded array {1}
for every call.
Because the output is identical for every row, any index that depends on it will collapse to a single key and provide no selectivity, while still incurring write-time and storage overhead.
Either implement the real computation here, or create the function now and add the index only in the migration that introduces the proper logic. If this is an intentional placeholder, add a clear TODO or RAISE NOTICE
so it isn’t forgotten.
🏁 Script executed:
#!/bin/bash
# Inspect usage of compute_transactions_amounts and index creation in the migration
rg -n "compute_transactions_amounts" -A5 internal/storage/bucket/migrations/41-index-transactions-amounts/up.sql
rg -n "create\s\+index" -n internal/storage/bucket/migrations/41-index-transactions-amounts/up.sql
Length of output: 483
compute_transactions_amounts stub makes GIN index non-selective
The migration’s compute_transactions_amounts
function is marked IMMUTABLE but always returns the constant '{1}'
. As a result, the GIN index you create will collapse to a single key for every row—incurring storage and write overhead with no lookup benefit.
Please update this migration to either:
- Implement the real computation in
compute_transactions_amounts
before creating the index - Split into two migrations: first define the real function, then in a subsequent migration create the index
- If this is a deliberate placeholder, add a clear
TODO
and aRAISE NOTICE
in the function body so it isn’t forgotten
Locations to address:
- internal/storage/bucket/migrations/41-index-transactions-amounts/up.sql:
- Lines 5–12 (function stub)
- Line 14 (index creation)
🤖 Prompt for AI Agents
In internal/storage/bucket/migrations/41-index-transactions-amounts/up.sql lines
5 to 12, the compute_transactions_amounts function is a stub returning a
constant, making the GIN index non-selective and ineffective. Fix this by
implementing the actual computation logic inside the function before creating
the index at line 14. Alternatively, split the migration into two: one defining
the real function and another creating the index. If keeping the stub
temporarily, add a clear TODO comment and a RAISE NOTICE inside the function to
ensure it is not forgotten.
8a4d82a
to
9e6c0e7
Compare
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)
docs/api/README.md (2)
1677-1686
: Consider adding an empty-body example
Because the body is optional (required: false
in the parameters table), it may be useful to show a second snippet illustrating an empty JSON object{}
or no body at all. This prevents downstream users from assuming metadata is mandatory.
1697-1697
: Body parameter description missing context
The “Description” column isnone
, yet other optional bodies state the intent (e.g. metadata that will be set on the reversal). A short description improves generated SDK docs.-|body|body|[V2RevertTransactionRequest](#schemav2reverttransactionrequest)|false|none| +|body|body|[V2RevertTransactionRequest](#schemav2reverttransactionrequest)|false|Additional metadata to attach to the reversal|
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
openapi.yaml
is excluded by!**/*.yaml
openapi/v2.yaml
is excluded by!**/*.yaml
pkg/client/.speakeasy/gen.lock
is excluded by!**/*.lock
,!**/*.lock
pkg/client/.speakeasy/logs/naming.log
is excluded by!**/*.log
,!**/*.log
pkg/client/speakeasyusagegen/.speakeasy/logs/naming.log
is excluded by!**/*.log
,!**/*.log
📒 Files selected for processing (10)
docs/api/README.md
(3 hunks)internal/api/v2/controllers_transactions_revert.go
(2 hunks)internal/controller/ledger/controller.go
(1 hunks)internal/controller/ledger/controller_default.go
(1 hunks)pkg/client/docs/models/components/v2reverttransactionrequest.md
(1 hunks)pkg/client/docs/models/operations/v2reverttransactionrequest.md
(1 hunks)pkg/client/models/components/v2reverttransactionrequest.go
(1 hunks)pkg/client/models/operations/v2reverttransaction.go
(2 hunks)pkg/client/v2.go
(2 hunks)test/e2e/api_transactions_revert_test.go
(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/client/models/components/v2reverttransactionrequest.go
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/client/docs/models/operations/v2reverttransactionrequest.md
- pkg/client/docs/models/components/v2reverttransactionrequest.md
- internal/controller/ledger/controller.go
- internal/controller/ledger/controller_default.go
- internal/api/v2/controllers_transactions_revert.go
- pkg/client/models/operations/v2reverttransaction.go
- test/e2e/api_transactions_revert_test.go
- pkg/client/v2.go
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/api/README.md
4588-4588: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Tests
- GitHub Check: Dirty
🔇 Additional comments (2)
docs/api/README.md (2)
1668-1673
: Good call addingContent-Type: application/json
The revert endpoint now explicitly documents the content-type header, bringing it in line with the other POST routes.
4571-4594
: Schema name duplicates heading id but that’s unavoidable
Markdown-lint flags the repeated “Properties” headings (MD024). Given the file is generated by Widdershins, fighting the linter isn’t worthwhile unless you plan to post-process the output. Safe to ignore.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #990 +/- ##
==========================================
- Coverage 82.90% 82.81% -0.09%
==========================================
Files 145 145
Lines 8229 8240 +11
==========================================
+ Hits 6822 6824 +2
- Misses 1081 1088 +7
- Partials 326 328 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -22,12 +24,26 @@ func revertTransaction(w http.ResponseWriter, r *http.Request) { | |||
return | |||
} | |||
|
|||
type X struct { |
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.
is X a good name ? Is it not possible to name it request instead ?
9e6c0e7
to
ff33d7e
Compare
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)
docs/api/README.md (2)
1677-1686
: Tweak the sample payload for stronger guidance
Using placeholder names likeproperty1
/property2
doesn’t convey typical usage. Consider switching to meaningful keys ("reason": "duplicate charge"
) so readers immediately understand the intent.- "metadata": { - "property1": "string", - "property2": "string" - } + "metadata": { + "reason": "duplicate charge", + "initiated_by": "support-user-123" + }
4571-4594
: Avoid redefining the metadata schema
V2RevertTransactionRequest
inlines ametadata
object identical to the existingV2Metadata
schema (see lines ~2476). Redefining it introduces redundancy and future drift risk.- |metadata|object|false|none|none| - |» **additionalProperties**|string|false|none|none| + |metadata|[V2Metadata](#schemav2metadata)|false|none|none|Applying this keeps one authoritative definition for free-form metadata across the API.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
openapi.yaml
is excluded by!**/*.yaml
openapi/v2.yaml
is excluded by!**/*.yaml
pkg/client/.speakeasy/gen.lock
is excluded by!**/*.lock
,!**/*.lock
pkg/client/.speakeasy/logs/naming.log
is excluded by!**/*.log
,!**/*.log
pkg/client/speakeasyusagegen/.speakeasy/logs/naming.log
is excluded by!**/*.log
,!**/*.log
📒 Files selected for processing (10)
docs/api/README.md
(3 hunks)internal/api/v2/controllers_transactions_revert.go
(2 hunks)internal/controller/ledger/controller.go
(1 hunks)internal/controller/ledger/controller_default.go
(1 hunks)pkg/client/docs/models/components/v2reverttransactionrequest.md
(1 hunks)pkg/client/docs/models/operations/v2reverttransactionrequest.md
(1 hunks)pkg/client/models/components/v2reverttransactionrequest.go
(1 hunks)pkg/client/models/operations/v2reverttransaction.go
(2 hunks)pkg/client/v2.go
(2 hunks)test/e2e/api_transactions_revert_test.go
(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/client/models/components/v2reverttransactionrequest.go
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/client/docs/models/operations/v2reverttransactionrequest.md
- internal/controller/ledger/controller.go
- internal/controller/ledger/controller_default.go
- internal/api/v2/controllers_transactions_revert.go
- pkg/client/models/operations/v2reverttransaction.go
- test/e2e/api_transactions_revert_test.go
- pkg/client/v2.go
- pkg/client/docs/models/components/v2reverttransactionrequest.md
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/api/README.md
4588-4588: Multiple headings with the same content
(MD024, no-duplicate-heading)
⏰ 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). (3)
- GitHub Check: Tests
- GitHub Check: Dirty
- GitHub Check: Cursor BugBot
🔇 Additional comments (2)
docs/api/README.md (2)
1670-1671
: Content-Type header addition looks correct
Explicitly statingContent-Type: application/json
in the example request removes any ambiguity for integrators.
1695-1698
: Double-check “Required” flag for the body parameter
The parameter table now marks thebody
as optional (Required = false
). Ensure the backend handler tolerates:
- Completely absent request bodies, and
Content-Type: application/json
with an empty body ({ }
).Otherwise, clients following the documentation will receive 4xx errors.
ff33d7e
to
cb23ff8
Compare
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 (5)
docs/api/README.md (5)
1676-1681
: Optional body → optionalContent-Type
headerThe
Content-Type: application/json
header was added to the example request even though the body parameter is optional (required=false
).
Including this header when no body is sent is misleading and may confuse API consumers and some tooling.-Content-Type: application/json
Consider either:
- Adding a comment that the header must be supplied only when a body is present, or
- Moving the header into a separate example that actually includes the JSON body.
1685-1695
: Use meaningful metadata keys in the example body
"property1"
/"property2"
are placeholder-like and don’t illustrate real-world usage.
Using something closer to production (e.g."reason": "fraud-detection"
or"initiator": "system"
) makes the docs more self-explanatory.
1705-1705
: Minor doc table polishThe description column for the new
body
parameter currently showsnone
.
Replacing it with a short sentence like “Optional request payload allowing extra metadata during revert” would remove ambiguity.
4598-4621
: Schema DRYness: reuseV2Metadata
instead of re-declaring a free-form object
V2RevertTransactionRequest.metadata
is effectively the same structure asV2Metadata
defined earlier.
Defining it twice increases maintenance overhead and may create divergence later.- |metadata|object|false|none|none| - |» **additionalProperties**|string|false|none|none| + |metadata|[V2Metadata](#schemav2metadata)|false|none|none|If the generator allows
$ref
, referencing the existing schema keeps the spec concise.
4615-4615
: Duplicate heading reported by markdownlint
MD024
flags a duplicate heading at this line.
It’s harmless but fixing it silences lint noise for downstream consumers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
openapi.yaml
is excluded by!**/*.yaml
openapi/v2.yaml
is excluded by!**/*.yaml
pkg/client/.speakeasy/gen.lock
is excluded by!**/*.lock
,!**/*.lock
pkg/client/.speakeasy/logs/naming.log
is excluded by!**/*.log
,!**/*.log
pkg/client/speakeasyusagegen/.speakeasy/logs/naming.log
is excluded by!**/*.log
,!**/*.log
📒 Files selected for processing (10)
docs/api/README.md
(3 hunks)internal/api/v2/controllers_transactions_revert.go
(2 hunks)internal/controller/ledger/controller.go
(1 hunks)internal/controller/ledger/controller_default.go
(1 hunks)pkg/client/docs/models/components/v2reverttransactionrequest.md
(1 hunks)pkg/client/docs/models/operations/v2reverttransactionrequest.md
(1 hunks)pkg/client/models/components/v2reverttransactionrequest.go
(1 hunks)pkg/client/models/operations/v2reverttransaction.go
(2 hunks)pkg/client/v2.go
(2 hunks)test/e2e/api_transactions_revert_test.go
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/controller/ledger/controller.go
- pkg/client/docs/models/components/v2reverttransactionrequest.md
- pkg/client/docs/models/operations/v2reverttransactionrequest.md
- internal/controller/ledger/controller_default.go
- pkg/client/models/components/v2reverttransactionrequest.go
- test/e2e/api_transactions_revert_test.go
- internal/api/v2/controllers_transactions_revert.go
- pkg/client/models/operations/v2reverttransaction.go
- pkg/client/v2.go
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/api/README.md
4615-4615: Multiple headings with the same content
(MD024, no-duplicate-heading)
⏰ 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). (3)
- GitHub Check: Cursor BugBot
- GitHub Check: Tests
- GitHub Check: Dirty
Fixes LX-65