feat: add capability to specify account metadata outside ns #835
Conversation
WalkthroughThe pull request introduces a new optional JSON structure, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant TxRequest
participant Controller
participant Ledger
Client->>API: POST Transaction Request (with accountMetadata)
API->>TxRequest: Parse payload & invoke ToCore (build CreateTransaction)
TxRequest->>Controller: Pass CreateTransaction parameters
Controller->>Ledger: Call CreateTransaction with parameters
Ledger-->>Controller: Return Log & CreatedTransaction
Controller-->>API: Send success response
API-->>Client: Return HTTP Response
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
✨ 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:
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
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #835 +/- ##
==========================================
- Coverage 82.00% 81.94% -0.06%
==========================================
Files 137 137
Lines 7512 7532 +20
==========================================
+ Hits 6160 6172 +12
- Misses 1036 1042 +6
- Partials 316 318 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🔭 Outside diff range comments (1)
internal/api/v1/controllers_transactions_create.go (1)
61-144: 💡 Verification agent🧩 Analysis chain
Verify that account metadata is properly handled
While the code changes look good, I don't see any specific handling for the "account metadata" mentioned in the PR objective. The PR summary mentions adding this capability, but it's not immediately clear from this file how account metadata is processed.
🏁 Script executed:
#!/bin/bash # Check the definition of CreateTransaction struct to verify account metadata handling rg -A 10 "type CreateTransaction struct" --type go # Look for any AccountMetadata references in the codebase rg "AccountMetadata" --type goLength of output: 33818
Action required: Propagate Account Metadata in Transaction Creation
The endpoint in
internal/api/v1/controllers_transactions_create.goisn’t mapping any account metadata from the incoming request—even though theledgercontroller.CreateTransactionstruct (defined ininternal/controller/ledger/controller.go) includes anAccountMetadatafield. This omission means that, despite the PR’s objective to add account metadata capability, the feature isn’t being exercised here.
- In both branches (postings and script), the command is constructed only with a
RunScriptvalue.- If the request is meant to include account metadata (e.g. as a separate field like
payload.AccountMetadata), it should be assigned to theAccountMetadatafield when callingl.CreateTransaction.Please confirm how the payload should supply account metadata and update the mapping accordingly.
🧹 Nitpick comments (3)
internal/controller/ledger/controller_default_test.go (1)
106-106: Potential inconsistency with metadata handlingWhile the
CreateTransactiontest on line 55 now uses an empty map for account metadata, theRevertTransactiontest on line 106 still usesnilfor the metadata parameter inCommitTransaction. For consistency, consider updating this to also use an empty map.- CommitTransaction(gomock.Any(), gomock.Any(), nil). + CommitTransaction(gomock.Any(), gomock.Any(), map[string]metadata.Metadata{}).docs/api/README.md (2)
497-509: Add “accountMetadata” to Bulk Request JSON Example
A new JSON field,accountMetadata, has been added immediately after themetadatafield in the bulk request example. It defines two properties (property1andproperty2), each with an embeddedadminkey. This is clear and consistent with the intended update. However, consider adding a brief explanatory note in the surrounding documentation (or in a dedicated parameters table) to clarify that this field is optional and describe its expected usage.
1293-1321: Include “accountMetadata” in Transaction Creation JSON Example
In the transaction creation example, the update introduces a newaccountMetadatafield (following the same structure as in the bulk request) in addition to the existingmetadatafield. The nested structure—withproperty1andproperty2each holding anadminflag—is clear and consistent. For clarity, it might be helpful to add a brief description in the accompanying documentation regarding the purpose ofaccountMetadataand how it differs (or complements) the standardmetadata.
📜 Review details
Configuration used: CodeRabbit UI
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 (26)
docs/api/README.md(7 hunks)internal/api/bulking/bulker.go(1 hunks)internal/api/bulking/bulker_test.go(1 hunks)internal/api/bulking/elements.go(2 hunks)internal/api/bulking/mocks_ledger_controller_test.go(1 hunks)internal/api/common/mocks_ledger_controller_test.go(1 hunks)internal/api/v1/controllers_transactions_create.go(2 hunks)internal/api/v1/controllers_transactions_create_test.go(1 hunks)internal/api/v1/mocks_ledger_controller_test.go(1 hunks)internal/api/v2/controllers_bulk_test.go(2 hunks)internal/api/v2/controllers_transactions_create.go(1 hunks)internal/api/v2/controllers_transactions_create_test.go(1 hunks)internal/api/v2/mocks_ledger_controller_test.go(1 hunks)internal/controller/ledger/controller.go(2 hunks)internal/controller/ledger/controller_default.go(4 hunks)internal/controller/ledger/controller_default_test.go(2 hunks)internal/controller/ledger/controller_generated_test.go(1 hunks)internal/controller/ledger/controller_with_events.go(1 hunks)internal/controller/ledger/controller_with_too_many_client_handling.go(1 hunks)internal/controller/ledger/controller_with_too_many_client_handling_test.go(2 hunks)internal/controller/ledger/controller_with_traces.go(1 hunks)internal/controller/system/state_tracker.go(1 hunks)pkg/client/docs/models/components/v2posttransaction.md(1 hunks)pkg/client/docs/sdks/v2/README.md(1 hunks)pkg/client/models/components/v2posttransaction.go(2 hunks)test/e2e/api_transactions_create_test.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (12)
internal/controller/ledger/controller_with_too_many_client_handling.go (2)
internal/controller/ledger/controller.go (1)
CreateTransaction(83-86)internal/storage/ledger/logs.go (1)
Log(20-26)
internal/api/v2/controllers_transactions_create_test.go (4)
internal/controller/ledger/controller.go (2)
CreateTransaction(83-86)RunScript(79-79)pkg/testserver/api.go (1)
CreateTransaction(37-45)internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/machine/vm/run.go (1)
RunScript(14-19)
internal/controller/ledger/controller.go (4)
internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/log.go (3)
Log(84-96)CreatedTransaction(193-196)AccountMetadata(191-191)internal/storage/ledger/logs.go (1)
Log(20-26)internal/machine/vm/run.go (1)
RunScript(14-19)
internal/api/v1/controllers_transactions_create_test.go (3)
internal/controller/ledger/controller.go (2)
CreateTransaction(83-86)RunScript(79-79)internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/machine/vm/run.go (1)
RunScript(14-19)
internal/controller/system/state_tracker.go (1)
internal/controller/ledger/controller.go (1)
CreateTransaction(83-86)
internal/controller/ledger/controller_generated_test.go (2)
internal/controller/ledger/controller.go (1)
CreateTransaction(83-86)internal/storage/ledger/logs.go (1)
Log(20-26)
internal/controller/ledger/controller_default_test.go (4)
internal/controller/ledger/controller.go (2)
CreateTransaction(83-86)RunScript(79-79)pkg/testserver/api.go (1)
CreateTransaction(37-45)internal/controller/ledger/parameters.go (1)
Parameters(3-7)internal/machine/vm/run.go (1)
RunScript(14-19)
internal/controller/ledger/controller_with_too_many_client_handling_test.go (1)
internal/controller/ledger/controller.go (1)
CreateTransaction(83-86)
internal/api/v2/mocks_ledger_controller_test.go (5)
internal/api/common/mocks_ledger_controller_test.go (1)
LedgerController(24-28)internal/api/v1/mocks_ledger_controller_test.go (1)
LedgerController(24-28)internal/api/bulking/mocks_ledger_controller_test.go (1)
LedgerController(24-28)internal/controller/ledger/controller.go (1)
CreateTransaction(83-86)internal/storage/ledger/logs.go (1)
Log(20-26)
internal/api/bulking/bulker_test.go (1)
internal/controller/ledger/controller.go (2)
CreateTransaction(83-86)RunScript(79-79)
internal/api/bulking/elements.go (2)
internal/controller/ledger/controller.go (3)
CreateTransaction(83-86)RunScript(79-79)Script(80-80)internal/api/v1/controllers_transactions_create.go (1)
Script(19-22)
internal/api/bulking/mocks_ledger_controller_test.go (5)
internal/api/common/mocks_ledger_controller_test.go (1)
LedgerController(24-28)internal/api/v1/mocks_ledger_controller_test.go (1)
LedgerController(24-28)internal/api/v2/mocks_ledger_controller_test.go (1)
LedgerController(24-28)internal/controller/ledger/controller.go (1)
CreateTransaction(83-86)internal/storage/ledger/logs.go (1)
Log(20-26)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (42)
internal/controller/ledger/controller_with_too_many_client_handling_test.go (2)
26-26: Parameter type updated to reflect new CreateTransaction typeThe parameter type has been changed from
Parameters[RunScript]toParameters[CreateTransaction], which is consistent with the introduction of the newCreateTransactionstruct that includes support for account metadata outside of namespaces.
60-60: Parameter type updated to reflect new CreateTransaction typeSimilar to line 26, this change updates the parameter type from
Parameters[RunScript]toParameters[CreateTransaction]to be consistent with the new parameter structure in the controller interface.internal/api/bulking/mocks_ledger_controller_test.go (1)
108-108: Mock method signature updated to use CreateTransaction parameter typeThe signature of the
CreateTransactionmock method has been updated to useledger0.Parameters[ledger0.CreateTransaction]instead ofledger0.Parameters[ledger0.RunScript]. This change is necessary to align the mock implementation with the actual interface that now uses the enhancedCreateTransactiontype which supports account metadata.pkg/client/docs/models/components/v2posttransaction.md (1)
12-13: Added AccountMetadata field for specifying metadata outside namespaceThe documentation has been enhanced with a new optional
AccountMetadatafield of typemap[string]map[string]*string*to support the capability of specifying account metadata outside of the namespace. This field allows associating metadata with specific accounts directly during transaction creation, which addresses the PR objective (LX-25).pkg/client/models/components/v2posttransaction.go (2)
35-35: Added AccountMetadata field to V2PostTransaction structA new field
AccountMetadataof typemap[string]map[string]stringhas been added to theV2PostTransactionstruct. This implementation allows clients to provide account-specific metadata during transaction creation, which is consistent with the documentation changes and the PR objective.
84-89: Added accessor method for the new AccountMetadata fieldThe
GetAccountMetadata()method provides a null-safe way to access the newAccountMetadatafield, consistent with the pattern used for other fields in the struct. This ensures API consumers can safely interact with the new field even when the object is nil.pkg/client/docs/sdks/v2/README.md (1)
1031-1038: New account metadata field looks good.This addition implements a new field
AccountMetadatawhich enables the specification of metadata for accounts outside the namespace (addressing LX-25). The structure is well-formed, with account keys mapping to metadata key-value pairs.internal/controller/ledger/controller.go (2)
46-46: Good signature update to support new functionality.The method signature has been correctly updated to accept
Parameters[CreateTransaction]instead of the previousParameters[RunScript], which aligns with the PR objective to support account metadata outside namespace.
83-86: Well-structured new type definition.The new
CreateTransactiontype appropriately embedsRunScriptto maintain backward compatibility while adding theAccountMetadatafield to support the new functionality. The use of the existingmetadata.Metadatatype ensures consistency with the rest of the codebase.internal/controller/ledger/controller_with_events.go (1)
41-41: Method signature correctly updated.The
CreateTransactionmethod signature has been properly updated to useParameters[CreateTransaction]instead of the previousParameters[RunScript], ensuring consistent implementation of the interface across the codebase.internal/controller/ledger/controller_with_traces.go (1)
352-352: Method signature correctly updated for tracing.The
CreateTransactionmethod signature inControllerWithTraceshas been properly updated to useParameters[CreateTransaction], maintaining consistency with the interface definition and other implementations.internal/api/v2/controllers_transactions_create_test.go (1)
404-408: Test updated to correctly use the CreateTransaction structThe change appropriately updates the test to use the new
CreateTransactionstruct which wraps the existingRunScriptstructure, aligning with the PR's goal of supporting account metadata specification outside the namespace.internal/controller/ledger/controller_with_too_many_client_handling.go (1)
43-43: Method signature updated to support account metadataThe method signature has been properly updated to use
Parameters[CreateTransaction]instead ofParameters[RunScript], maintaining consistency with the interface changes across the codebase. This change enables the handling of account metadata during transaction creation while preserving the retry logic for handling "too many clients" errors.internal/controller/system/state_tracker.go (1)
49-49: Method signature updated for account metadata supportThe signature change correctly updates the parameter type to use
ledgercontroller.Parameters[ledgercontroller.CreateTransaction], aligning with the broader refactoring in the codebase. The state tracking logic remains unchanged, ensuring proper state management while supporting the new account metadata feature.test/e2e/api_transactions_create_test.go (1)
120-163: Well-structured test for the new account metadata featureThe new test case properly validates that account metadata can be specified outside of the namespace. The test creates a transaction with postings and adds metadata to two accounts ("world" and "alice"), then verifies that the metadata is correctly associated with each account after the transaction is created.
This test effectively covers the core functionality introduced in this PR and ensures that the account metadata is properly persisted in the database.
internal/api/v1/controllers_transactions_create_test.go (1)
230-234: Update to CreateTransaction parameters reflects new account metadata capabilityThe parameter type for
CreateTransactionhas been changed fromParameters[RunScript]toParameters[CreateTransaction]to accommodate the new account metadata functionality. This change aligns with the PR objective to enable specifying account metadata outside the namespace.internal/api/bulking/bulker_test.go (1)
59-65: LGTM: Transaction creation parameters updated correctlyThe test correctly updates the parameters passed to the
CreateTransactionmethod, wrapping theRunScriptin aCreateTransactionstruct to support the new account metadata functionality.internal/controller/ledger/controller_generated_test.go (1)
107-107: Parameter type change in mock implementationThe mock correctly reflects the updated parameter type for the
CreateTransactionmethod, changing fromParameters[RunScript]toParameters[CreateTransaction]to support the new account metadata functionality.internal/api/bulking/bulker.go (2)
129-129: Method rename from ToRunScript to ToCoreThe method call has been updated from
ToRunScript(false)toToCore(false)to better represent its purpose of converting to a core transaction structure rather than just a run script.
134-138: CreateTransaction parameter type updated for account metadata supportThe parameter type has been changed from
Parameters[RunScript]toParameters[CreateTransaction]to accommodate the new account metadata capability, aligning with the broader API changes.internal/api/v2/mocks_ledger_controller_test.go (1)
108-108: Updated method signature to support account metadataThe
CreateTransactionmethod signature has been updated to useledger0.Parameters[ledger0.CreateTransaction]instead ofledger0.Parameters[ledger0.RunScript]. This change correctly aligns with the new feature that allows specifying account metadata outside of the namespace.internal/api/v1/mocks_ledger_controller_test.go (1)
108-108: Updated method signature to support account metadataThe signature change for
CreateTransactionfrom usingledger0.Parameters[ledger0.RunScript]toledger0.Parameters[ledger0.CreateTransaction]is consistent with the new feature implementation and matches the changes made in other mock controllers.internal/api/common/mocks_ledger_controller_test.go (1)
108-108: Updated method signature to support account metadataThe
CreateTransactionmethod signature has been consistently updated across all mock controllers to use the newledger0.Parameters[ledger0.CreateTransaction]type, which now includes account metadata capabilities.internal/controller/ledger/controller_default_test.go (2)
55-55: Initialized account metadata map in CommitTransaction callThe test has been updated to pass an empty metadata map (
map[string]metadata.Metadata{}) instead ofnil, ensuring consistent behavior when account metadata is present but empty.
67-70: Updated test to use new CreateTransaction structureThe test now correctly uses the new
Parameters[CreateTransaction]structure with the embeddedRunScriptfield, aligning with the interface changes in the controller. This change supports the new capability to specify account metadata outside the namespace.internal/api/v1/controllers_transactions_create.go (2)
86-88: Properly adapted the function call to use the new CreateTransaction structureThe code has been updated to use the new
ledgercontroller.CreateTransactionstructure that wraps the previously usedRunScriptparameter. This change aligns with the PR objective of adding capability to specify account metadata outside namespace.
117-124: Implementation consistently applies the new structure patternThis section correctly implements the same pattern as the previous modification, encapsulating the
RunScriptdata within the newCreateTransactionstructure. The code maintains proper handling of script, timestamp, reference, and metadata.internal/api/v2/controllers_transactions_create.go (2)
35-35: Method call renamed from ToRunScript to ToCoreThe change from
payload.ToRunScript()topayload.ToCore()aligns with the PR objective to enhance account metadata handling. This refactoring suggests a shift from treating the transaction as a script to run, to creating a core transaction representation.
41-41: Variable renamed and type changedThe variable has been renamed from
runScripttocreateTransactionto better reflect its purpose, and the type is nowCreateTransactioninstead ofRunScript. This change maintains consistency with the updated method signature.internal/api/v2/controllers_bulk_test.go (2)
67-73: Test updated to use the new CreateTransaction structureThe mock expectation has been updated to use
ledgercontroller.Parameters[ledgercontroller.CreateTransaction]instead ofledgercontroller.Parameters[ledgercontroller.RunScript]. The transaction data is now correctly wrapped in aCreateTransactionstruct, withRunScriptas a field.
477-483: Consistent use of new CreateTransaction structureThis change mirrors the earlier update to properly use the new
CreateTransactionstructure in test expectations, maintaining consistency throughout the test file.internal/api/bulking/elements.go (4)
102-102: New field added for account metadataAdded a new field
AccountMetadataof typemap[string]metadata.Metadatato theTransactionRequeststruct. This implements the core feature of the PR - allowing account metadata to be specified outside the namespace.
105-105: Method renamed with updated return typeThe method has been renamed from
ToRunScripttoToCoreand its return type changed from*ledgercontroller.RunScriptto*ledgercontroller.CreateTransaction. This reflects the shift towards a more general transaction creation approach rather than just script execution.
111-128: Refactored logic to handle both postings and script casesThe implementation now creates a
runScriptvariable that's properly populated based on whether postings exist or script is provided. This maintains the existing logic while adapting to the new structure.
130-133: Return updated to include account metadataThe method now returns a
CreateTransactionstruct that includes both theRunScriptand the newAccountMetadatafield. This ensures that account metadata is properly passed through the transaction creation flow.internal/controller/ledger/controller_default.go (7)
47-47: Updated type parameter for createTransactionLpThe type parameter for
createTransactionLphas been changed fromRunScripttoCreateTransaction, reflecting the updated transaction creation flow.
103-103: Updated log processor initializationThe initialization of
createTransactionLpnow usesCreateTransactionas its type parameter, consistent with the type change.
287-287: Updated method signature for createTransactionThe method signature now accepts
Parameters[CreateTransaction]instead ofParameters[RunScript], reflecting the updated transaction creation flow.
325-338: New logic added to process account metadataThis new implementation properly handles account metadata:
- It initializes
accountMetadatafrom the script execution result- Creates an empty map if none exists
- Merges in any account metadata provided in the input parameters
This is a key part of implementing the PR's objective to allow specifying account metadata outside the namespace.
345-345: Updated CommitTransaction call to include account metadataThe
CommitTransactioncall now includes the processed account metadata, ensuring it's properly stored with the transaction.
352-352: Updated return value to include account metadataThe
CreatedTransactionreturn value now includes the account metadata, ensuring it's available to callers.
356-356: Updated method signature for CreateTransactionThe public API method has been updated to use
Parameters[CreateTransaction]instead ofParameters[RunScript], maintaining consistency with the internal implementation changes.
Fixes LX-25