feat: Create new data access to allow for in-memory transactions. #1386
Conversation
MantisClone
left a comment
MantisClone
left a comment
There was a problem hiding this comment.
This looks like the right approach to me! Please add some unit tests in the transaction-manager to show that it works as intended.
packages/transaction-manager/src/inmemory-transaction-manager.ts
Outdated
Show resolved
Hide resolved
packages/transaction-manager/src/inmemory-transaction-manager.ts
Outdated
Show resolved
Hide resolved
packages/transaction-manager/src/in-memory-transaction-manager.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@aimensahnoun I think I lead you to the wrong implementation.
I now believe we should make the InMemoryTransactionManager functions available on the regular TransactionManager.
Reason: I want to persistInMemoryTransaction() from the RequestClient layer.
transaction-manager.ts pseudocode
public async persistTransaction() {
processTransaction()
persistTransactionToDataAccess()
}
public async createInMemoryTransaction() {
processTransaction()
}
public async persistInMemoryTransaction() {
persistTransactionToDataAccess()
} Then, we can make these functions available on the layers above.
request-client.ts & request-logic pseudocode
public async createRequest(bool inMemory?) {
if inMemory {
eventually calls createInMemoryTransaction()
}
else {
eventually calls persistTransaction()
}
}
public async _createEncryptedRequest(bool inMemory?) {
if inMemory {
eventually calls createInMemoryTransaction()
}
else {
eventually calls persistTransaction()
}
}
// Should handle persisting cleartext and encrypted requests
// Should throw error if Request is already persisted.
public async persistInMemoryRequest() {
eventually calls persistInMemoryTransaction()
}Context
Originally, I proposed we create an InMemoryTransactionManagerwith a persistTransaction() function that it returns the request, in-memory, without actually persisting it on-chain.
Why not inject InMemoryTransactionManager?
Injecting InMemoryTransactionManager is great for creating an in-memory request because it doesn't require any new functions. But we still must create a new function to persist an in-memory Request. This new function should be available at the request-client.js layer.
We could make the developer interact with the InMemoryTransactionManager instance, but that deviates from convention - where creating and retrieving requests is available at the request-client.js layer without interacting with lower layers.
What happens if someone calls RequestNetwork.persistInMemoryRequest(Request) with a regular TransactionManager instead of an InMemoryTransactionManager? It would fail as "Not Implemented".
In the injected design, the InMemoryTransactionManager.persistTransaction() doesn't actually persist the transaction. This is misleading.
I thought we were moving away from runtime config and towards static composition and dependency injection to reduce download time #1202
The ability to create a Request on the frontend and persist it from the backend is a common request from our builders so enabling it on the normal TransactionManager class makes sense. 🙆
This change won't bloat the download size because the RequestLogic is extracted to utility functions to avoid code duplication.
Update an In-Memory Request?
Should we make it possible to update an in-memory request? Such that when we persist it persists the creation AND subsequent updates?
I'm leaning towards no because I can't think of a use case and I'd rather not bother with that complexity if we don't need to.
MantisClone
requested review from
alexandre-abrioux,
benjlevesque,
kevindavee and
yomarion
alexandre-abrioux
left a comment
alexandre-abrioux
left a comment
There was a problem hiding this comment.
Hi, the idea is great! I haven't read through the code yet, but could you explain the need to inject the whole TransactionManager, and not just a custom DataAccess? At Request Finance we handle #1380 and #1331 by injecting our own DataAccess implementation, which does the "write" part asynchronously. Users can pay their request before it exists on the network.
Here is pseudo-code:
/**
* A DataAccess Writer that publishes transactions in the database
* It does not wait for them to succeed
*/
export class DatabaseDataWrite implements DataAccessTypes.IDataWrite {
constructor(
private dbConnection: Connection,
private pendingStore: DataAccessTypes.IPendingStore,
) {}
async initialize(): Promise<void> {
return Promise.resolve();
}
async close(): Promise<void> {
return Promise.resolve();
}
async persistTransaction(
transaction: DataAccessTypes.ITransaction,
channelId: string,
topics?: string[],
): Promise<DataAccessTypes.IReturnPersistTransaction> {
const id = uuid.v4();
// "Fire and forget" the transaction: it will be saved in DB and handled asynchronously
// We don't listen to the confirmation
await this.dbConnection.save({
transaction,
channelId,
topics,
});
logger.info(`Published message ${id} for ${channelId}`);
// Return a dummy transaction
// in order for the transaction-manager to continue processing successfully
const transactionRaw: DataAccessTypes.IReturnPersistTransactionRaw = {
meta: {
topics,
// a temporary location ID, to help with debugging
transactionStorageLocation: id,
storageMeta: {
state: StorageTypes.ContentState.PENDING,
timestamp: Utils.getCurrentTimestampInSecond(),
ipfs: {
size: 1,
},
storageType: StorageTypes.StorageSystemType.LOCAL,
},
},
result: {},
};
// We don't need to confirm the transaction here.
const storageResult: StorageTypes.IAppendResult = Object.assign(
new EventEmitter(),
{
id,
content: "",
meta: transactionRaw.meta.storageMeta,
} as StorageTypes.IEntry,
);
this.pendingStore.add(channelId, {
topics,
transaction,
storageResult,
});
return Object.assign(new EventEmitter(), transactionRaw);
}
}
/** A Data Access that uses a database to create transactions, and a Graph Node to read them */
export class DatabaseDataAccess extends CombinedDataAccess {
constructor(
subgraphUrl: string,
pubsubTopic: string,
chainName: string,
dbConnection: Connection,
) {
// this is useful only at creation; the Request Client needs to read
// the request right after creating it, which would fail or take a long time
// if using the subgraph.
const pendingStore = new PendingStore();
super(
new TheGraphDataAccess({
graphql: {
url: subgraphUrl,
},
network: chainName,
pendingStore,
logger,
}),
new DatabaseDataWrite(dbConnection, pendingStore),
);
}
_getStatus(): Promise<DataAccessTypes.IDataAccessStatus> {
throw new Error("Method not implemented.");
}
}Then, when it's time to persist it, use any IPFS client to submit the transaction. and the EthereumTransactionSubmitter to add it on-chain (no need to install the whole Request Network library for that part, nor instantiate the whole client).
|
@alexandre-abrioux your comment convinced me we should inject this feature at the DataAccess layer, instead of the TransactionManager layer. My Goal
I want a good developer experience for (1) and (2). In my opinion, this means it's accessible via the Proposed SolutionImplement (1) by injecting a not-yet-implemented The The The proposed FYI @aimensahnoun |
aimensahnoun
force-pushed
the
1380-pay-before-persist
branch
from
9b911d4 to
776bb7d
Compare
…o 1380-pay-before-persist
aimensahnoun
changed the title
InMemoryTransactionManager class for in memory transac…
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (5)
- packages/request-client.js/src/api/request-network.ts (7 hunks)
- packages/request-client.js/src/http-request-network.ts (4 hunks)
- packages/request-client.js/src/no-persist-http-data-access.ts (1 hunks)
- packages/request-client.js/test/http-request-network.test.ts (1 hunks)
- packages/request-client.js/test/in-memory-request.test.ts (1 hunks)
Additional context used
Biome
packages/request-client.js/src/no-persist-http-data-access.ts
[error] 3-3: A Node.js builtin module should be imported with the node: protocol.
Using the node: protocol is more explicit and signals that the imported module belongs to Node.js.
Unsafe fix: Add the node: protocol.(lint/style/useNodejsImportProtocol)
Additional comments not posted (13)
packages/request-client.js/src/no-persist-http-data-access.ts (2)
5-19: Constructor Implementation ReviewThe constructor of
NoPersistHttpDataAccessproperly delegates initialization to the superclassHttpDataAccessusing the provided configurations. This ensures that any setup done by the superclass is preserved.
21-47: Review ofpersistTransactionMethodThe
persistTransactionmethod is designed to simulate transaction persistence by immediately emitting a 'confirmed' event without actually persisting the data. This fits the requirement for in-memory transaction handling. However, ensure that this behavior is clearly documented to avoid confusion, as it deviates from typical persistence expectations.packages/request-client.js/test/in-memory-request.test.ts (2)
8-38: Review of Test Setup and TeardownThe setup and teardown methods are appropriately configured to ensure a clean testing environment. The use of
mockServerto simulate network responses is a good practice. Additionally, thespyPersistTransactionmock function setup is crucial for verifying that thepersistTransactionmethod is not called unexpectedly.
46-80: Test Coverage for In-Memory Request HandlingThe tests cover the creation of a request without persisting it and the expected behavior when attempting to persist such a request. These tests are crucial for validating the new functionality introduced in the PR. Ensure that these tests are included in the continuous integration process to maintain coverage.
packages/request-client.js/test/http-request-network.test.ts (1)
21-21: Review of Test Handler ResetReplacing
mockServer.restoreHandlers()withmockServer.resetHandlers()in theafterEachhook is a good practice. It ensures that each test starts with a clean state, which is crucial for test isolation and reliability.packages/request-client.js/src/api/request-network.ts (5)
7-7: Addition ofClientTypes:
The addition ofClientTypesappears to be necessary for the new functionality related to handling client-specific data types. Ensure that all new types are properly documented and used consistently throughout the codebase.
12-12: Addition ofExtensionTypes:
The addition ofExtensionTypesis essential for managing different extensions, particularly in the context of payment requests. It's important to ensure that these types are integrated seamlessly with the existing codebase and that they are used correctly in the new methods introduced.
25-25: Import ofNoPersistHttpDataAccess:
The import ofNoPersistHttpDataAccessis crucial for the new feature allowing requests to be handled in memory. This class should be thoroughly reviewed to ensure it adheres to the expected behaviors of not persisting data.
37-37: Modification ofdataAccessdeclaration:
The modification to makedataAccessa private member is a good practice, enhancing encapsulation. This change should be accompanied by appropriate access methods if external access is required.
194-224: MethodpersistRequestReview:
ThepersistRequestmethod is used to persist in-memory requests. It properly throws errors if the request was not created withskipPersistenceor if the data access layer does not support persistence. This is a critical method for the new functionality, ensuring that requests can be transitioned from in-memory to persisted state securely.The error messages are clear and informative, which is good for user experience. However, ensure that these errors are handled gracefully in the calling code to prevent crashes or unintended behavior.
Verification successful
Error Handling for
persistRequestVerified:
ThepersistRequestmethod's error handling is verified through tests inin-memory-request.test.ts. These tests ensure that the errors thrown bypersistRequestare correctly handled.
- Tests check for errors using
expect(...).rejects.toThrow(...).Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify error handling in calling code for `persistRequest`. # Test: Search for calls to `persistRequest` and check for error handling. rg --type python -A 5 $'persistRequest'Length of output: 69
Script:
#!/bin/bash # Description: Verify error handling in calling code for `persistRequest`. # Test: Search for calls to `persistRequest` and check for error handling. rg --type ts -A 5 'persistRequest'Length of output: 2355
packages/request-client.js/src/http-request-network.ts (3)
14-14: Import statement forNoPersistHttpDataAccessis correctly placed.
Line range hint
40-49: Review of new parameterskipPersistencein the constructor.The addition of the
skipPersistenceparameter is correctly implemented. It's well integrated into the options object with a default value.Also applies to: 53-53
58-62: Conditional instantiation based onskipPersistence.The logic to conditionally instantiate
NoPersistHttpDataAccessbased on theskipPersistenceflag is correctly implemented. This allows for flexibility in transaction handling based on the persistence requirement.
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range comments (1)
packages/request-client.js/src/api/request-network.ts (1)
Line range hint
146-184: Correct spelling and consider refactoringcreateRequestmethod.There's a minor spelling mistake in
isSkipingPersistence. It should beisSkippingPersistence. Also, the method's complexity has increased with the new functionality. Consider refactoring to separate the concerns more clearly, perhaps by extracting parts of the logic into smaller, more focused methods.- const isSkipingPersistence = this.dataAccess instanceof NoPersistHttpDataAccess; + const isSkippingPersistence = this.dataAccess instanceof NoPersistHttpDataAccess;Refactoring suggestion: Extract the logic related to handling in-memory transactions into a separate method to improve readability and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- packages/request-client.js/src/api/request-network.ts (7 hunks)
Additional comments not posted (2)
packages/request-client.js/src/api/request-network.ts (2)
Line range hint
225-266: Refer tocreateRequestmethod review.The considerations for error handling, spelling corrections, and potential refactoring in the
createRequestmethod apply here as well. Please refer to the comments made on that method for applicable improvements.
Line range hint
267-317: MethodfromRequestIdlooks good.This method handles the retrieval of requests by ID effectively, including handling both confirmed and pending states. The implementation is clear and adheres to coding standards.
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- packages/request-client.js/src/api/request-network.ts (7 hunks)
Additional context used
Learnings (1)
packages/request-client.js/src/api/request-network.ts (2)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:74-145 Timestamp: 2024-07-17T13:57:39.144Z Learning: The data passed to the `preparePaymentRequest` method in the `RequestNetwork` class is reliably formatted by the transaction manager, negating the need for additional error handling for data formatting within this method.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:74-145 Timestamp: 2024-07-17T13:50:20.447Z Learning: The transaction manager in the Request Network SDK formats the data ensuring its integrity before it reaches methods like `preparePaymentRequest`.
Additional comments not posted (5)
packages/request-client.js/src/api/request-network.ts (5)
7-7: New import added:ClientTypesThis import is necessary for the new features introduced in this PR, specifically for handling client-specific types in the
RequestNetworkclass.
12-12: New import added:ExtensionTypesThis import is crucial for handling extension-specific types used in the new
preparePaymentRequestmethod.
37-37: Modification todataAccesspropertyThe
dataAccessproperty has been modified to accommodate instances of bothDataAccessTypes.IDataAccessandNoPersistHttpDataDataAccess. This change is essential for the new feature that allows skipping persistence.
62-62: Assignment ofdataAccesswithin constructorThis line is crucial as it ensures that the
dataAccessinstance passed to the constructor is used throughout the class. This change is aligned with the new feature requirements.
74-145: New method:preparePaymentRequestThis method prepares a payment request structure from transaction data, which is a key part of the new feature allowing payments before persistence. The method appears to handle the data correctly and efficiently, creating a structured request object.
However, there is no error handling for JSON parsing, which could lead to runtime errors if the data is malformed. Given the previous discussions and learnings, it seems the transaction manager ensures data integrity before it reaches this method, making additional error handling redundant.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (2)
- packages/request-client.js/src/http-request-network.ts (4 hunks)
- packages/request-client.js/test/in-memory-request.test.ts (1 hunks)
Additional comments not posted (4)
packages/request-client.js/src/http-request-network.ts (3)
29-29: Clarification ofskipPersistenceoption in constructor documentation.The documentation for the
skipPersistenceoption is clear and aligns with the PR objectives to allow transactions without immediate persistence. This is a good practice as it enhances the understanding for future developers.
49-49: Implementation of conditional data access instantiation.The implementation correctly handles the
skipPersistenceflag to conditionally instantiateNoPersistHttpDataDataAccessorHttpDataAccess. This is a crucial feature for enabling transactions without immediate persistence and aligns well with the PR objectives.Also applies to: 53-53, 58-62
14-14: Consider removing unused import if not needed.The import of
NoPersistHttpDataAccessappears to be used conditionally based on theskipPersistenceflag. Ensure that it is indeed utilized and not just imported without a purpose.Verification successful
The import of
NoPersistHttpDataAccessis used and necessary.The
NoPersistHttpDataAccessis utilized in conditional statements within the codebase, confirming that the import is required and should not be removed.
packages/request-client.js/src/http-request-network.tspackages/request-client.js/src/api/request-network.tsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `NoPersistHttpDataAccess` across the codebase. # Test: Search for the usage of `NoPersistHttpDataAccess`. Expect: Usages in conditional statements. rg --type python -A 5 $'NoPersistHttpDataAccess'Length of output: 78
Script:
#!/bin/bash # Description: Verify the usage of `NoPersistHttpDataAccess` across the codebase. # Test: Search for the usage of `NoPersistHttpDataAccess`. Expect: Usages in conditional statements. rg --type typescript -A 5 'NoPersistHttpDataAccess'Length of output: 86
Script:
#!/bin/bash # Description: Verify the usage of `NoPersistHttpDataAccess` across the codebase. # Test: Search for the usage of `NoPersistHttpDataAccess`. Expect: Usages in conditional statements. rg --type js -A 5 'NoPersistHttpDataAccess'Length of output: 43
Script:
#!/bin/bash # Description: Verify the usage of `NoPersistHttpDataAccess` across the codebase. # Test: Search for the usage of `NoPersistHttpDataAccess` across all file types. rg -A 5 'NoPersistHttpDataAccess'Length of output: 4634
packages/request-client.js/test/in-memory-request.test.ts (1)
8-120: Comprehensive test suite for in-memory request handling.The test suite thoroughly covers the functionalities introduced in the PR, including creating requests without persistence, handling errors when persistence is attempted incorrectly, and persisting in-memory requests properly. This ensures robust handling and error management for in-memory requests, aligning with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range comments (4)
packages/request-client.js/src/api/request-network.ts (4)
Line range hint
146-186: Review ofcreateRequestMethod:The modifications to accommodate in-memory requests are well-implemented. However, the method could benefit from clearer documentation, especially regarding the conditions under which persistence is skipped. Additionally, consider optimizing the creation of the
Requestobject to avoid redundancy in the conditional logic.+ // Check if persistence should be skipped and handle accordingly + const isSkippingPersistence = this.dataAccess instanceof NoPersistHttpDataAccess; + if (isSkippingPersistence) { + // Handle in-memory request creation + // Ensure all necessary data is present + }
Line range hint
225-268: Review of_createEncryptedRequestMethod:This method handles the creation of encrypted requests effectively. However, similar to
createRequest, it could benefit from clearer documentation and potential optimization in the handling of in-memory requests.+ // Check if persistence should be skipped and handle accordingly + const isSkippingPersistence = this.dataAccess instanceof NoPersistHttpDataAccess; + if (isSkippingPersistence) { + // Handle in-memory request creation + // Ensure all necessary data is present + }
Line range hint
293-324: Review offromRequestIdMethod:This method effectively retrieves a request based on its ID. The error handling for non-existent requests is appropriate. Consider adding more detailed logging for debugging purposes, especially when a request is not found.
+ // Log the error for better debugging + if (!requestAndMeta.result.request && !requestAndMeta.result.pending) { + console.error(localUtils.formatGetRequestFromIdError(requestAndMeta)); + throw new Error(localUtils.formatGetRequestFromIdError(requestAndMeta)); + }
Line range hint
350-386: Review offromMultipleTopicsMethod:This method efficiently retrieves an array of request instances based on multiple topics. It handles the creation and refresh of request objects well. Consider adding more detailed logging for debugging purposes, especially when requests are not found.
+ // Log the error for better debugging + if (!requestsAndMeta.result.requests.length) { + console.error('No requests found for the provided topics'); + }
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- packages/request-client.js/src/api/request-network.ts (7 hunks)
Additional context used
Learnings (1)
packages/request-client.js/src/api/request-network.ts (2)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:74-145 Timestamp: 2024-07-17T13:57:39.144Z Learning: The data passed to the `preparePaymentRequest` method in the `RequestNetwork` class is reliably formatted by the transaction manager, negating the need for additional error handling for data formatting within this method.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:74-145 Timestamp: 2024-07-17T13:50:20.447Z Learning: The transaction manager in the Request Network SDK formats the data ensuring its integrity before it reaches methods like `preparePaymentRequest`.
Additional comments not posted (3)
packages/request-client.js/src/api/request-network.ts (3)
Line range hint
269-292: Review ofcomputeRequestIdMethod:This method correctly computes the ID of a request. It is straightforward and does not appear to have any issues. Ensure that the parameters are validated before processing to avoid potential errors.
Line range hint
325-349: Review offromIdentityMethod:This method correctly retrieves an array of request instances based on an identity. Ensure that the identity type is supported before processing to avoid unnecessary errors.
74-145: Review ofpreparePaymentRequestMethod:This method efficiently structures payment requests from transaction data. However, there are a few improvements and checks needed:
Error Handling: The method assumes the transaction data is always correctly formatted. Considering the transaction data comes from external sources, it would be safer to add error handling around JSON parsing and data manipulation to prevent runtime errors.
Data Integrity: Ensure that all necessary fields in
transactionDataare present before accessing them. This can prevent potentialundefinederrors.Performance: The method iterates over
originalExtensionsDatato buildnewExtensions. Consider optimizing this iforiginalExtensionsDatacan be large.- const requestData = JSON.parse(transactionData.data as string).data; + try { + const requestData = JSON.parse(transactionData.data as string).data; + } catch (error) { + throw new Error('Invalid transaction data format'); + } - const newExtensions: RequestLogicTypes.IExtensionStates = {}; + const newExtensions: RequestLogicTypes.IExtensionStates = originalExtensionsData.reduce((acc, extension) => { + if (extension.id !== ExtensionTypes.OTHER_ID.CONTENT_DATA) { + acc[extension.id] = { + events: [{ + name: extension.action, + parameters: { + paymentAddress: extension.parameters.paymentAddress, + salt: extension.parameters.salt, + }, + timestamp: requestData.parameters.timestamp, + }], + id: extension.id, + type: ExtensionTypes.TYPE.PAYMENT_NETWORK, + values: { + salt: extension.parameters.salt, + receivedPaymentAmount: '0', + receivedRefundancy: '0', + sentPaymentAmount: '0', + sentRefundAmount: '0', + paymentAddress: extension.parameters.paymentAddress, + }, + version: extension.version, + }; + } + return acc; + }, {});Skipped due to learnings
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:74-145 Timestamp: 2024-07-17T13:57:39.144Z Learning: The data passed to the `preparePaymentRequest` method in the `RequestNetwork` class is reliably formatted by the transaction manager, negating the need for additional error handling for data formatting within this method.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:74-145 Timestamp: 2024-07-17T13:50:20.447Z Learning: The transaction manager in the Request Network SDK formats the data ensuring its integrity before it reaches methods like `preparePaymentRequest`.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- packages/request-client.js/src/api/request-network.ts (7 hunks)
Additional context used
Learnings (1)
packages/request-client.js/src/api/request-network.ts (2)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:74-145 Timestamp: 2024-07-17T13:57:39.144Z Learning: The data passed to the `preparePaymentRequest` method in the `RequestNetwork` class is reliably formatted by the transaction manager, negating the need for additional error handling for data formatting within this method.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:74-145 Timestamp: 2024-07-17T13:50:20.447Z Learning: The transaction manager in the Request Network SDK formats the data ensuring its integrity before it reaches methods like `preparePaymentRequest`.
Additional comments not posted (3)
packages/request-client.js/src/api/request-network.ts (3)
121-151: Review ofpersistRequestMethod:The method correctly handles the persistence of in-memory requests. The error handling for missing
inMemoryInfoand unsupported data access instances is appropriate. However, consider enhancing the error messages to provide more context to developers:- throw new Error('Cannot persist request without inMemoryInfo.'); + throw new Error('Cannot persist request without inMemoryInfo. Ensure the request was created with skipPersistence enabled.'); - throw new Error( - 'Cannot persist request when skipPersistence is enabled. To persist the request, create a new instance of RequestNetwork without skipPersistence being set to true.', - ); + throw new Error( + 'Cannot persist request: the current data access instance does not support persistence. Please use a compatible data access instance.', + );
489-561: Review ofpreparePaymentRequestMethod:This method effectively prepares a structured request for payment purposes. It's essential to ensure that all fields are correctly extracted and that error handling is robust, especially given the potential complexity of transaction data parsing:
- const requestData = JSON.parse(transactionData.data as string).data; + try { + const requestData = JSON.parse(transactionData.data as string).data; + } catch (error) { + throw new Error('Invalid transaction data format'); + }Consider adding validations for
transactionDataandoriginalExtensionsDatabefore processing them.
Line range hint
37-62: Review of Constructor Initialization:The constructor correctly initializes various components and handles optional parameters well. However, it's crucial to ensure that all dependencies like
currencyManagerandpaymentOptionsare adequately tested, especially since they have default behaviors and conditional initializations.
MantisClone
left a comment
MantisClone
left a comment
There was a problem hiding this comment.
Partial Review, about half way through.
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (4)
- packages/request-client.js/src/api/request-network.ts (7 hunks)
- packages/request-client.js/src/api/request.ts (3 hunks)
- packages/request-client.js/test/in-memory-request.test.ts (1 hunks)
- packages/types/src/request-logic-types.ts (2 hunks)
Additional context used
Learnings (2)
packages/request-client.js/test/in-memory-request.test.ts (1)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:25-25 Timestamp: 2024-07-17T15:06:14.563Z Learning: The `persistTransaction()` function in the `no-persist-http-data-access.ts` file is tested in the `in-memory-request.test.ts` file, which is separate from the typical `index.test.ts` file to avoid test interference.packages/request-client.js/src/api/request-network.ts (3)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:25-25 Timestamp: 2024-07-17T15:06:14.563Z Learning: The `persistTransaction()` function in the `no-persist-http-data-access.ts` file is tested in the `in-memory-request.test.ts` file, which is separate from the typical `index.test.ts` file to avoid test interference.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:74-145 Timestamp: 2024-07-17T13:57:39.144Z Learning: The data passed to the `preparePaymentRequest` method in the `RequestNetwork` class is reliably formatted by the transaction manager, negating the need for additional error handling for data formatting within this method.Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:74-145 Timestamp: 2024-07-17T13:50:20.447Z Learning: The transaction manager in the Request Network SDK formats the data ensuring its integrity before it reaches methods like `preparePaymentRequest`.
Additional comments not posted (7)
packages/request-client.js/test/in-memory-request.test.ts (2)
46-60: Comprehensive test for creating a request without persistence.This test effectively verifies that a request can be created with
skipPersistenceenabled, and it checks all relevant properties of the request to ensure they are populated correctly. Additionally, it confirms thatpersistTransactionis not called, which is crucial for the in-memory functionality.
96-119: Validates the persistence of an in-memory request.This test effectively verifies that a request initially created with
skipPersistencecan be persisted later using a different instance ofRequestNetwork. It correctly asserts thatpersistTransactionis called and checks the result of the persistence operation.packages/request-client.js/src/api/request.ts (2)
82-90: Review of the newinMemoryInfoproperty addition.The addition of the
inMemoryInfoproperty is well-documented and clearly describes its purpose and usage. This change aligns with the PR's goal to handle requests in memory before persistence, allowing operations like payments to be processed beforehand.
- Correctness: The property is correctly typed as
RequestLogicTypes.IInMemoryInfo | null, which allows for proper handling of both scenarios where the in-memory data is available or not.- Documentation: The property is well-documented, with clear descriptions of each sub-property (
transactionData,topics,requestData). This helps in understanding the role of each in the context of in-memory requests.Overall, this addition appears to be well-implemented and serves its intended purpose effectively.
Line range hint
112-123: Review of constructor modifications to handleinMemoryInfo.The constructor has been updated to accept an optional
inMemoryInfoparameter. This is a crucial change as it integrates the in-memory request handling directly into the instantiation process of theRequestobject.
- Correctness: The
inMemoryInfois correctly initialized withoptions?.inMemoryInfo || null, ensuring that it falls back tonullif not provided. This is important for backward compatibility and cases where in-memory handling is not required.- Integration: The integration of
inMemoryInfointo the constructor is seamless and does not interfere with existing parameters or the initialization of other properties.This change is well-executed and necessary for the feature introduced by this PR. It allows the
Requestobject to be instantiated with in-memory data when needed, supporting the new workflow described in the PR.packages/types/src/request-logic-types.ts (1)
217-222: Review of theIInMemoryInfoInterfaceThe new interface
IInMemoryInfois well-defined and aligns with the PR objectives to manage in-memory transactions. It includes necessary fields liketransactionData,topics, andrequestDatawhich are crucial for handling requests that are not immediately persisted.However, it would be beneficial to add documentation comments to this interface to explain the purpose of each field and how it is intended to be used. This will improve maintainability and understandability for other developers.
packages/request-client.js/src/api/request-network.ts (2)
25-25: Review ofNoPersistHttpDataAccessImportThe import of
NoPersistHttpDataAccessis crucial for the new feature of handling in-memory transactions. This class allows theRequestNetworkto handle requests that are not immediately persisted.
121-151: Review ofpersistRequestMethodThe new
persistRequestmethod is crucial for persisting in-memory requests. It correctly checks if the request was created withskipPersistenceenabled and throws appropriate errors if conditions are not met.The method's implementation is clear and aligns with the functionality described in the PR. The error messages are specific and informative, which is good for debugging and developer experience.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (1)
- packages/request-client.js/src/http-request-network.ts (4 hunks)
Additional context used
Learnings (2)
Common learnings
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:74-145 Timestamp: 2024-07-17T13:57:39.144Z Learning: The data passed to the `preparePaymentRequest` method in the `RequestNetwork` class is reliably formatted by the transaction manager, negating the need for additional error handling for data formatting within this method.packages/request-client.js/src/http-request-network.ts (1)
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:25-25 Timestamp: 2024-07-17T15:06:14.563Z Learning: The `persistTransaction()` function in the `no-persist-http-data-access.ts` file is tested in the `in-memory-request.test.ts` file, which is separate from the typical `index.test.ts` file to avoid test interference.
Additional comments not posted (2)
packages/request-client.js/src/http-request-network.ts (2)
14-14: Consider removing unused import.The import of
NoPersistHttpDataAccessis commented out. If this class is not used, it should be removed to keep the code clean. If it's intended for future use, a comment explaining its purpose would be helpful.Skipped due to learnings
Learnt from: MantisClone PR: RequestNetwork/requestNetwork#1386 File: packages/request-client.js/src/api/request-network.ts:25-25 Timestamp: 2024-07-17T15:06:14.563Z Learning: The `persistTransaction()` function in the `no-persist-http-data-access.ts` file is tested in the `in-memory-request.test.ts` file, which is separate from the typical `index.test.ts` file to avoid test interference.
Line range hint
40-62: Review of conditional instantiation logic for data access.The logic for instantiating
NoPersistHttpDataAccessorHttpDataAccessbased on theskipPersistenceflag is implemented correctly. However, ensure that theNoPersistHttpDataAccessclass is fully compatible and tested, as it plays a crucial role in the new functionality.Also, consider adding error handling or a fallback mechanism if neither
useMockStoragenorskipPersistenceconditions are met but the required configurations (httpConfig,nodeConnectionConfig) are incomplete or invalid.
Resolves #1380, Resolves #1331
Description:
NoPersistHttpDataAccessin order to bypass the persistence of a request while it's being created.skipPersistenceboolean tohttp-request-networkto useNoPersistHttpDataAccessinstead of the defaultHttpDataAccess.persistTransactionmethod toRequestNetwork.prepareRequestDataForPaymentmethod in order to create a structured request object that can be used for paying the request.Summary by CodeRabbit
New Features
skipPersistenceoption in transaction handling, allowing creation without immediate persistence.inMemoryInfoproperty for handling in-memory requests and pre-persistence operations.Improvements
skipPersistence.Walkthrough by CodeRabbit (copied from comment that got buried in the comments below)