Skip to content

Conversation

gfyrag
Copy link
Collaborator

@gfyrag gfyrag commented Jun 30, 2025

Fixes LX-65

@gfyrag gfyrag requested a review from a team as a code owner June 30, 2025 13:16
Copy link

coderabbitai bot commented Jun 30, 2025

Walkthrough

This 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

File(s) Change Summary
docs/api/README.md, pkg/client/docs/models/components/v2reverttransactionrequest.md, pkg/client/docs/models/operations/v2reverttransactionrequest.md API and client documentation updated to describe new optional metadata in revert transaction requests.
internal/api/v2/controllers_transactions_revert.go Handler updated to accept and decode optional metadata from JSON request body.
internal/controller/ledger/controller.go RevertTransaction struct extended with a Metadata field.
internal/controller/ledger/controller_default.go Revert logic modified to use input metadata when creating the reversed transaction.
pkg/client/models/components/v2reverttransactionrequest.go New struct V2RevertTransactionRequest with Metadata field and getter method added.
pkg/client/models/operations/v2reverttransaction.go Struct updated to include pointer to V2RevertTransactionRequest and getter method.
pkg/client/v2.go Client method updated to serialize and send request body with metadata and set content-type header.
test/e2e/api_transactions_revert_test.go Tests updated and extended to verify revert with additional metadata.

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)
Loading

Assessment against linked issues

Objective Addressed Explanation
Modify the revertTransaction endpoint to accept an optional JSON request body (LX-65)
Merge user-provided metadata into resulting transaction metadata, excluding override of revert key (LX-65)
If request body omitted or empty, endpoint behavior remains unchanged (LX-65)
Document updated request format and behavior in API reference (LX-65)
Add test coverage validating metadata storage, revert metadata integrity, and backward compatibility (LX-65)

Assessment against linked issues: Out-of-scope changes

No out-of-scope changes detected related to the objectives from the linked issues.

Possibly related PRs

  • fix: missing metadata on reverts #861: Adds helper functions for metadata handling and sets default revert metadata on reversed transactions, which relates to this PR's extension of metadata support in the revert flow.

Suggested reviewers

  • paul-nicolas

Poem

In the ledger’s gentle warren,
A new field hops in with glee—
Metadata for every revert,
Custom notes for all to see.
Now every bunny’s transaction,
Can carry a story anew,
With carrots, code, and context too! 🥕

✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/revert-transaction-add-metadata

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai auto-generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a 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:

  1. an empty object {} or
  2. 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

📥 Commits

Reviewing files that changed from the base of the PR and between db77af0 and 075881f.

⛔ 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 through ledger.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:

  1. Request body serialization: Properly added utils.SerializeRequestBody call with the new V2RevertTransactionRequest type
  2. HTTP semantics: Correctly passes bodyReader to the POST request and sets Content-Type header when needed
  3. Consistency: Follows the same pattern as other methods in the file that accept request bodies
  4. Backward compatibility: The true parameter suggests this is optional, maintaining compatibility with existing clients

The 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 |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

@gfyrag gfyrag force-pushed the feat/revert-transaction-add-metadata branch from 075881f to 8a4d82a Compare June 30, 2025 13:52
Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 075881f and 8a4d82a.

⛔ 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 correct

The example payload and the new body parameter row correctly describe an optional metadata 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)));
Copy link

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.

Comment on lines 5 to 12
create function compute_transactions_amounts(postings jsonb)
returns numeric[]
language sql
immutable
as
$$
select '{1}';
$$;
Copy link

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 a RAISE 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.

@gfyrag gfyrag force-pushed the feat/revert-transaction-add-metadata branch from 8a4d82a to 9e6c0e7 Compare June 30, 2025 14:07
Copy link

@coderabbitai coderabbitai bot left a 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 is none, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8a4d82a and 9e6c0e7.

⛔ 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 adding Content-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.

Copy link

codecov bot commented Jun 30, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.

Project coverage is 82.81%. Comparing base (37b70b5) to head (cb23ff8).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/api/v2/controllers_transactions_revert.go 63.63% 3 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@@ -22,12 +24,26 @@ func revertTransaction(w http.ResponseWriter, r *http.Request) {
return
}

type X struct {
Copy link
Contributor

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 ?

@gfyrag gfyrag force-pushed the feat/revert-transaction-add-metadata branch from 9e6c0e7 to ff33d7e Compare July 16, 2025 12:37
Copy link

@coderabbitai coderabbitai bot left a 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 like property1 / 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 a metadata object identical to the existing V2Metadata 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9e6c0e7 and ff33d7e.

⛔ 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 stating Content-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 the body as optional (Required = false). Ensure the backend handler tolerates:

  1. Completely absent request bodies, and
  2. Content-Type: application/json with an empty body ({ }).

Otherwise, clients following the documentation will receive 4xx errors.

@gfyrag gfyrag force-pushed the feat/revert-transaction-add-metadata branch from ff33d7e to cb23ff8 Compare July 16, 2025 22:34
@gfyrag gfyrag enabled auto-merge July 16, 2025 22:34
Copy link

@coderabbitai coderabbitai bot left a 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 → optional Content-Type header

The 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:

  1. Adding a comment that the header must be supplied only when a body is present, or
  2. 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 polish

The description column for the new body parameter currently shows none.
Replacing it with a short sentence like “Optional request payload allowing extra metadata during revert” would remove ambiguity.


4598-4621: Schema DRYness: reuse V2Metadata instead of re-declaring a free-form object

V2RevertTransactionRequest.metadata is effectively the same structure as V2Metadata 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

📥 Commits

Reviewing files that changed from the base of the PR and between ff33d7e and cb23ff8.

⛔ 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

@gfyrag gfyrag added this pull request to the merge queue Jul 16, 2025
Merged via the queue into main with commit 4c01edc Jul 16, 2025
9 of 11 checks passed
@gfyrag gfyrag deleted the feat/revert-transaction-add-metadata branch July 16, 2025 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants