feat: allow no persist for Custom Data Access #1541
Conversation
WalkthroughThis pull request introduces significant architectural changes to the data access layer across multiple packages. The primary focus is on enhancing the flexibility and modularity of data persistence mechanisms. Key modifications include adding a Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
🧰 Additional context used📓 Learnings (1)packages/request-client.js/src/http-data-access.ts (2)⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (5)
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
github-actions
bot
requested review from
alexandre-abrioux,
leoslr,
sstefdev and
yomarion
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/request-client.js/src/http-metamask-data-access.ts (1)
45-45: LGTM! Explicit persistence setting aligns with PR objectives.The addition of
persist: truemaintains the existing behavior while making the persistence configuration explicit. This change supports the PR's goal of allowing customDataAccessimplementations to control persistence behavior.Consider documenting this persistence behavior in the class's JSDoc, especially since this class handles both IPFS and Ethereum storage. This would help implementers understand the implications of the
persistflag in the context of MetaMask-based storage.packages/request-client.js/test/in-memory-request.test.ts (1)
66-84: Consider enhancing test coverage.While the test case correctly verifies basic functionality, consider adding:
- Verification that the mock storage wasn't called
- Assertions for the actual value of
mockDataAccess.persist- Test case for persisting a request created with custom data access
it('creates a request without persisting it with custom data access', async () => { const mockStorage = new MockStorage(); const mockDataAccess = new MockDataAccess(mockStorage, { persist: false }); + const mockStorageSpy = jest.spyOn(mockStorage, 'save'); requestNetwork = new RequestNetworkBase({ signatureProvider: TestData.fakeSignatureProvider, dataAccess: mockDataAccess, }); const request = await requestNetwork.createRequest(requestCreationParams); expect(request).toBeDefined(); expect(request.requestId).toBeDefined(); expect(request.inMemoryInfo).toBeDefined(); expect(request.inMemoryInfo?.requestData).toBeDefined(); expect(request.inMemoryInfo?.topics).toBeDefined(); expect(request.inMemoryInfo?.transactionData).toBeDefined(); expect(spyPersistTransaction).not.toHaveBeenCalled(); + expect(mockStorageSpy).not.toHaveBeenCalled(); + expect(mockDataAccess.persist).toBe(false); });packages/request-client.js/src/http-data-access.ts (1)
17-17: LGTM! Consider enhancing type safety.The
persistproperty implementation looks good with proper readonly modifier and default value.Consider using a type alias for the constructor parameters to improve maintainability:
type HttpDataAccessConfig = { httpConfig?: Partial<ClientTypes.IHttpDataAccessConfig>; nodeConnectionConfig?: Partial<NodeConnectionConfig>; persist: boolean; };Also applies to: 38-38, 42-42, 46-46, 51-51
packages/request-client.js/src/api/request-network.ts (1)
151-151: Enhance error message clarity.The error handling is correct, but the error message could be more specific about the current state.
Consider this improved error message:
- if (!this.dataAccess.persist) { - 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.', - ); - } + if (!this.dataAccess.persist) { + throw new Error( + `Cannot persist request: current DataAccess instance (${this.dataAccess.constructor.name}) has persistence disabled. To persist the request, create a new instance of RequestNetwork with a DataAccess that has persistence enabled.`, + ); + }Also applies to: 160-163
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/data-access/src/combined-data-access.ts(1 hunks)packages/data-access/src/data-write.ts(1 hunks)packages/data-access/src/mock-data-access.ts(1 hunks)packages/request-client.js/src/api/request-network.ts(4 hunks)packages/request-client.js/src/http-data-access.ts(2 hunks)packages/request-client.js/src/http-metamask-data-access.ts(1 hunks)packages/request-client.js/src/http-request-network.ts(1 hunks)packages/request-client.js/src/no-persist-http-data-access.ts(1 hunks)packages/request-client.js/test/in-memory-request.test.ts(2 hunks)packages/thegraph-data-access/src/NoopDataWrite.ts(1 hunks)packages/types/src/data-access-types.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
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-11-12T14:52:33.204Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (11)
packages/request-client.js/src/http-metamask-data-access.ts (1)
45-45: Verify persistence behavior across storage layers.Since this class handles both IPFS and Ethereum storage with caching, let's verify the impact of the persistence flag:
✅ Verification successful
Persistence flag is a safe configuration setting
The persist flag is a constructor parameter passed to the parent class that doesn't impact the broader persistence behavior. The codebase shows consistent transaction persistence patterns through well-defined interfaces, with no conditional logic depending on this flag.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any derived classes and persistence-related code # Check for classes extending HttpMetaMaskDataAccess echo "Checking for derived implementations..." rg "extends HttpMetaMaskDataAccess" # Check for persistence-related code in the codebase echo "Checking persistence-related code..." rg "persist.*DataAccess" --type ts # Check for IPFS/Ethereum storage patterns echo "Checking storage patterns..." rg "persistTransaction|getTransactionsByChannelId" --type ts -A 5Length of output: 68901
packages/thegraph-data-access/src/NoopDataWrite.ts (1)
4-4: LGTM! Clean implementation of the persist parameter.The addition of the
persistparameter with a default value oftruemaintains backward compatibility while enabling the new functionality.packages/request-client.js/src/no-persist-http-data-access.ts (1)
18-18: LGTM! Consistent implementation of no-persist behavior.The explicit
persist: falsein the super call maintains the existing no-persist behavior while utilizing the new persistence control mechanism.packages/data-access/src/mock-data-access.ts (1)
16-22: LGTM! Well-structured implementation of persistence control.The changes properly integrate the persist option while maintaining backward compatibility through default values.
packages/data-access/src/data-write.ts (1)
9-9: Verify test coverage for persistence control.Ensure comprehensive test coverage exists for the new persistence control functionality across different scenarios.
Let's check the test coverage:
✅ Verification successful
Test coverage for persistence control is comprehensive.
The codebase includes extensive test coverage across multiple packages, covering:
- Core persistence functionality for both clear and encrypted transactions
- Error handling and edge cases
- In-memory and HTTP data access scenarios
- API endpoint integration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases related to persistence control rg -l 'persist.*test|test.*persist' --type ts # Look for specific test scenarios rg -A 5 'describe.*persist|test.*persist|it.*persist' --type tsLength of output: 26762
packages/data-access/src/combined-data-access.ts (1)
4-11: LGTM! Clean implementation of persistence state propagation.The readonly modifier ensures immutability, and initialization from the writer's persist value maintains consistency.
packages/request-client.js/src/http-request-network.ts (1)
60-68: Verify persistence behavior across all code paths.The implementation looks correct, but let's verify the persistence behavior is consistent across all code paths:
- Mock storage with skipPersistence: persist=false
- Mock storage without skipPersistence: persist=true
- NoPersistHttpDataAccess: persist=false (implicit)
- HttpDataAccess: persist=true (explicit)
✅ Verification successful
Persistence behavior is correctly implemented across all code paths
The implementation correctly handles persistence configuration:
- MockDataAccess: Properly sets persist based on skipPersistence flag
- NoPersistHttpDataAccess: Implicitly no persistence by design
- HttpDataAccess: Explicitly sets persist=true
The persistence behavior is consistent with the IDataWrite interface requirements and follows a clear pattern across all implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify persistence configuration is consistent # Search for instantiations of data access classes with persist parameter ast-grep --pattern 'new $CLASS($_, { persist: $_})'Length of output: 464
Script:
#!/bin/bash # Search for class definitions and persistence-related implementations ast-grep --pattern 'class $CLASS extends $_ { constructor($_) { $$$ } }' # Also search for any persistence-related method calls rg "persist" -A 3 -B 3Length of output: 68940
packages/types/src/data-access-types.ts (1)
28-28: LGTM! Well-placed interface addition.The
persistproperty is appropriately added to theIDataWriteinterface, establishing a clear contract for persistence behavior.packages/request-client.js/src/api/request-network.ts (3)
117-117: LGTM! Good decoupling.Replacing the instanceof check with a property check improves extensibility by allowing custom DataAccess implementations to control persistence behavior.
200-200: LGTM! Consistent implementation.The change maintains consistency with the
createRequestmethod implementation.
Line range hint
17-17: Verify all DataAccess implementations.Let's ensure all DataAccess implementations have been updated with the persist property.
✅ Verification successful
All DataAccess implementations correctly include the persist property
All implementations of IDataWrite and IDataAccess properly declare the readonly persist property with consistent default values. The inheritance chain is complete and no implementations are missing the required property.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all DataAccess class implementations ast-grep --pattern 'class $_ implements DataAccessTypes.IDataAccess { $$$ }' # Search for persist property declarations rg -l "persist.*:.*boolean" packages/Length of output: 25932
Script:
#!/bin/bash # Check interface definition echo "=== IDataAccess interface definition ===" rg -A 5 "interface IDataAccess" packages/types/src/data-access-types.ts # Look for alternative implementations echo -e "\n=== Alternative DataAccess implementations ===" ast-grep --pattern 'class $_ { $$$ persist: boolean; $$$ }' # Check content of files with persist declarations echo -e "\n=== Content of NoopDataWrite.ts ===" cat packages/thegraph-data-access/src/NoopDataWrite.ts echo -e "\n=== Content of data-write.ts ===" cat packages/data-access/src/data-write.ts echo -e "\n=== Content of combined-data-access.ts ===" cat packages/data-access/src/combined-data-access.ts echo -e "\n=== Content of mock-data-access.ts ===" cat packages/data-access/src/mock-data-access.tsLength of output: 6687
Script:
#!/bin/bash # Check IDataWrite interface definition echo "=== IDataWrite interface definition ===" rg -A 5 "interface IDataWrite" packages/types/src/data-access-types.ts # Look for any missed implementations echo -e "\n=== All files implementing IDataWrite or IDataAccess ===" rg -l "implements (DataAccessTypes\.)?(IDataWrite|IDataAccess)" packages/ # Verify no other persist property implementations exist echo -e "\n=== Any other persist property declarations ===" rg "persist\s*:\s*boolean" packages/Length of output: 1490
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/request-client.js/src/http-data-access.ts (1)
97-108: Enhance type safety in result object constructionWhile the implementation is functionally correct, using
Object.assigndirectly with the event emitter might not guarantee type safety. Consider creating a type guard or using a more type-safe approach.Consider this alternative implementation:
- const result: DataAccessTypes.IReturnPersistTransaction = Object.assign( - eventEmitter, - getNoPersistTransactionRawData(topics), - ); + const noPersistData = getNoPersistTransactionRawData(topics); + const result: DataAccessTypes.IReturnPersistTransaction = { + ...noPersistData, + on: eventEmitter.on.bind(eventEmitter), + once: eventEmitter.once.bind(eventEmitter), + emit: eventEmitter.emit.bind(eventEmitter), + };This approach:
- Explicitly defines the shape of the result object
- Properly binds event emitter methods
- Ensures all required properties are present
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/request-client.js/src/http-data-access.ts(5 hunks)packages/request-client.js/src/http-metamask-data-access.ts(3 hunks)packages/request-client.js/src/http-request-network.ts(1 hunks)packages/request-client.js/src/no-persist-http-data-access.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/request-client.js/src/no-persist-http-data-access.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/request-client.js/src/http-request-network.ts
- packages/request-client.js/src/http-metamask-data-access.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (3)
packages/request-client.js/src/http-data-access.ts (3)
7-7: LGTM!The import of
getNoPersistTransactionRawDatais correctly added to support the new no-persist functionality.
18-18: LGTM!The
persistproperty is correctly declared as a public readonly boolean, which ensures immutability and proper type safety.
52-52: LGTM!The
persistproperty is correctly initialized in the constructor.
kevindavee
force-pushed
the
feat/allow-no-persist-for-custom-data-access
branch
from
ff2d53d to
952184d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/data-access/src/no-persist-transaction-raw-data.ts (1)
12-12: Consider using more precise timestamp.The current implementation uses
Date.now() / 1000which loses precision due to floating-point division. Consider usingMath.floor(Date.now() / 1000)for a more precise Unix timestamp.- timestamp: Date.now() / 1000, + timestamp: Math.floor(Date.now() / 1000),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/data-access/src/data-write.ts(2 hunks)packages/data-access/src/index.ts(1 hunks)packages/data-access/src/no-persist-transaction-raw-data.ts(1 hunks)packages/request-client.js/src/http-data-access.ts(5 hunks)packages/request-client.js/src/http-metamask-data-access.ts(3 hunks)packages/request-client.js/src/http-request-network.ts(1 hunks)packages/request-client.js/src/no-persist-http-data-access.ts(0 hunks)packages/request-client.js/test/api/request-network.test.ts(1 hunks)packages/transaction-manager/test/index.test.ts(19 hunks)
💤 Files with no reviewable changes (1)
- packages/request-client.js/src/no-persist-http-data-access.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/request-client.js/src/http-data-access.ts
- packages/request-client.js/src/http-request-network.ts
🧰 Additional context used
📓 Learnings (1)
packages/request-client.js/test/api/request-network.test.ts (1)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1386
File: packages/request-client.js/src/api/request-network.ts:25-25
Timestamp: 2024-11-12T14:52:33.204Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (7)
packages/data-access/src/index.ts (1)
8-8: LGTM! Clean export addition.The new export follows the established pattern in the barrel file.
packages/data-access/src/no-persist-transaction-raw-data.ts (1)
3-17: LGTM! Well-structured utility function.The function provides a clean implementation for generating non-persisted transaction data with proper typing.
packages/request-client.js/test/api/request-network.test.ts (1)
19-19: LGTM! Consistent mock data access configuration.The
persist: trueproperty has been consistently added to all mock data access objects throughout the test file, maintaining test consistency.Also applies to: 230-230, 277-277, 321-321, 398-398, 450-450, 508-508, 567-567, 641-641, 729-729, 806-806, 885-885, 963-963, 1112-1112, 1187-1187, 1256-1256, 1336-1336, 1406-1406, 1486-1486
packages/transaction-manager/test/index.test.ts (1)
64-64: LGTM! Consistent mock data access configuration.The
persist: trueproperty has been consistently added to all mock data access objects throughout the test file, maintaining test consistency.Also applies to: 230-230, 277-277, 321-321, 398-398, 450-450, 508-508, 567-567, 641-641, 729-729, 806-806, 885-885, 963-963, 1112-1112, 1187-1187, 1256-1256, 1336-1336, 1406-1406, 1486-1486
packages/data-access/src/data-write.ts (2)
10-10: LGTM! Good default value choice.The
persistparameter with a default value oftruemaintains backward compatibility while allowing opt-in control of persistence.
28-39: LGTM! Clean implementation of the no-persist flow.The implementation correctly:
- Creates the event emitter early
- Handles the no-persist case with proper event emission
- Returns immediately without unnecessary storage operations
packages/request-client.js/src/http-metamask-data-access.ts (1)
76-87: LGTM! Consistent implementation with DataAccessWrite.The no-persist flow implementation matches the pattern in DataAccessWrite, ensuring consistent behavior across different DataAccess implementations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/request-client.js/src/http-metamask-data-access.ts (1)
81-90: LGTM: No-persist logic implementationThe implementation correctly:
- Checks the persist flag
- Uses getNoPersistTransactionRawData for consistent behavior
- Emits confirmation immediately for no-persist cases
However, consider adding a debug log to help with troubleshooting:
if (!this.persist) { + console.debug('Skipping persistence as persist is set to false'); const result: DataAccessTypes.IReturnPersistTransaction = Object.assign( eventEmitter, getNoPersistTransactionRawData(topics), );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/request-client.js/src/http-data-access.ts(5 hunks)packages/request-client.js/src/http-metamask-data-access.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/request-client.js/src/http-data-access.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (4)
packages/request-client.js/src/http-metamask-data-access.ts (4)
1-1: LGTM: Import addition aligns with new functionalityThe addition of
getNoPersistTransactionRawDataimport is appropriate for implementing the no-persist feature.
36-48: Consider deriving persist value from httpConfigWhile the implementation is functional, it would be more consistent to derive the
persistvalue fromhttpConfigsince it's a data access configuration. This aligns with the existing pattern where HTTP-related configurations are passed throughhttpConfig.- persist, }: { httpConfig?: Partial<ClientTypes.IHttpDataAccessConfig>; nodeConnectionConfig?: NodeConnectionConfig; web3?: any; ethereumProviderUrl?: string; - persist?: boolean; } = { httpConfig: {}, - persist: true, }, ) { - super({ httpConfig, nodeConnectionConfig, persist }); + super({ httpConfig, nodeConnectionConfig, persist: httpConfig?.persist ?? true });
79-80: LGTM: Event emitter initializationThe EventEmitter is correctly initialized and typed as PersistTransactionEmitter.
81-90: Verify impact on transaction trackingSince MetaMask transactions involve blockchain interactions, ensure that downstream components correctly handle non-persisted transactions and don't assume persistence.
Run this script to check for potential assumptions about persistence in dependent components:
✅ Verification successful
Non-persisted transactions are handled safely
The implementation correctly handles non-persisted transactions by immediately emitting confirmation events and maintaining consistent behavior with persisted transactions. Downstream components are designed to work with both scenarios, as evidenced by extensive test coverage and proper handling in the transaction manager.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for code that might assume transaction persistence echo "Checking for persistence assumptions in transaction handling..." # Look for direct references to transaction storage or confirmation rg -l 'transaction.*storage|confirmation.*status' --type ts # Look for MetaMask transaction handling ast-grep --pattern 'class $_ { $$$ transaction($$$) { $$$ storage.$$$ $$$ } }'Length of output: 536
Script:
#!/bin/bash # Check transaction manager implementation echo "=== Transaction Manager Implementation ===" rg "class.*TransactionManager" -A 10 packages/transaction-manager/src/transaction-manager.ts echo -e "\n=== Persistence Flag Usage ===" rg "persist|isPersisted" --type ts -A 3 echo -e "\n=== Integration Test Cases ===" rg "describe.*transaction" -A 5 packages/integration-test/test/layers.test.tsLength of output: 71631
There was a problem hiding this comment.
I think we can have the same outcome without needing a new property persist = true | false in each class.
What I have in mind is the following:
- Turn
HttpDataAccessinto aCombinedDataAccessby splitting it into 2 classes:HttpDataReadandHttpDataWrite - Export a new writer class in the
data-accesspackage:NoPersistDataWrite - Depending on if the user wants to persist data or not, they can use the
HttpDataWriteor theNoPersistDataWrite - Builders that have their own custom
CombinedDataAccesscan also use the newNoPersistDataWriteif they need to
┌────────────────┐
│ HttpDataAccess │
└────────┬───────┘
│
│
┌──────────▼─────────┐
│ CombinedDataAccess │
└┬────────────────┬──┘
│ │
│ │
┌──────────▼─────┐ ┌────▼───────────┐
│ HttpDataRead │ │ HttpDataWrite │
└────────────────┘ └────────────────┘
or
┌────────────────────┐
│ NoPersistDataWrite │
└────────────────────┘
|
@kevindavee How urgently do you need this Pull Request merged? @aimensahnoun please review, given that you are most familiar with the original feature. |
I agree with this approach, despite this being a breaking change to the I don't think we need to maintain backward compatibility as this InMemory Request feature is still rather new. I'll make sure that our other app builders are informed of the breaking change and can migrate if needed. |
|
@alexandre-abrioux , thanks for the suggestion. I think your approach is cleaner. @MantisClone , not that urgent. This change is needed for better performance in Request Finance. But nothing is blocking us right now.
|
|
@alexandre-abrioux, I've tried to implement your approach. It could manage the dependency injection for a persistent or non-persistent Examples: We can't rely on the class type since we don't know what class type is used during run-time. We still need a property that we can read during run-time. Even though we can check |
|
@kevindavee What about keeping the |
|
For dependency injection, that works. But how do we know from |
|
|
|
Never mind my answer above; you're right. It does not account for builders using customer DataAccess implementations. I think it could work if you make the if (this.dataAccess instanceof CombinedDataAccess && this.dataAccess.writer instanceof NoPersistDataWrite) {
...
} |
|
I think we're almost there.
Maybe instead of using |
|
@kevindavee Looks good 🙂 |
kevindavee
force-pushed
the
feat/allow-no-persist-for-custom-data-access
branch
from
970bba8 to
dcf76d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (8)
packages/data-access/src/no-persist-data-write.ts (1)
25-27: Consider using more precise timestamp.Using
Date.now()/1000might lose precision. Consider using a more precise method or storing the full millisecond timestamp.- timestamp: Date.now() / 1000, + timestamp: Math.floor(Date.now() / 1000), // Ensure consistent integer timestampspackages/request-client.js/src/http-transaction.ts (1)
17-30: Document retry behavior in method JSDoc.The method uses retry logic with configurable parameters, but this behavior isn't documented in the method's JSDoc.
/** * Gets a transaction from the node through HTTP. * @param transactionData The transaction data + * @throws {Error} If hash normalization fails + * @remarks This method implements exponential backoff retry logic with the following parameters: + * - Maximum retries: Configured via httpConfig.getConfirmationMaxRetry + * - Initial retry delay: Configured via httpConfig.getConfirmationRetryDelay + * - Exponential backoff: Configured via httpConfig.getConfirmationExponentialBackoffDelay + * - Maximum backoff delay: Configured via httpConfig.getConfirmationMaxExponentialBackoffDelay */packages/request-client.js/src/http-data-read.ts (1)
61-66: Extract pagination parameter handling to a utility function.The pagination parameter handling logic is duplicated across methods. Consider extracting it to a reusable utility function.
+private buildPaginationParams(page?: number, pageSize?: number) { + return { + ...(page !== undefined && { page }), + ...(pageSize !== undefined && { pageSize }), + }; +} const params = { topic, updatedBetween, - ...(page !== undefined && { page }), - ...(pageSize !== undefined && { pageSize }), + ...this.buildPaginationParams(page, pageSize), };packages/request-client.js/src/http-data-write.ts (1)
48-53: Add request timeout configuration.The fetch request doesn't have a timeout configuration, which could lead to hanging requests in case of network issues.
const data = await this.dataAccessConfig.fetch<DataAccessTypes.IReturnPersistTransactionRaw>( 'POST', '/persistTransaction', - undefined, + { + timeout: this.dataAccessConfig.httpConfig.requestTimeout || 30000, // Default 30s timeout + }, { channelId, topics, transactionData }, );packages/request-client.js/src/http-data-access-config.ts (1)
85-93: Consider using URLSearchParams for parameter handlingThe current implementation manually processes parameters and uses the
qslibrary. Consider using the built-inURLSearchParamsfor better maintainability and native browser compatibility.- // qs.parse doesn't handle well mixes of string and object params - for (const [key, value] of Object.entries(params)) { - if (typeof value === 'object') { - params[key] = JSON.stringify(value); - } - } - url.search = stringify(params); + const searchParams = new URLSearchParams(); + for (const [key, value] of Object.entries(params)) { + searchParams.append(key, typeof value === 'object' ? JSON.stringify(value) : String(value)); + } + url.search = searchParams.toString();packages/request-client.js/src/http-data-access.ts (1)
35-42: Consider extracting initialization logic to a separate methodThe constructor contains complex initialization logic. Consider extracting it to a private method for better readability and testability.
constructor(...) { - const dataAccessConfig = new HttpDataAccessConfig({ httpConfig, nodeConnectionConfig }); - const transaction = new HttpTransaction(dataAccessConfig); - const reader = new HttpDataRead(dataAccessConfig); - const writer = persist - ? new HttpDataWrite(dataAccessConfig, transaction) - : new NoPersistDataWrite(); - - super(reader, writer); + const { dataAccessConfig, transaction, reader, writer } = this.initializeComponents( + httpConfig, + nodeConnectionConfig, + persist + ); + super(reader, writer); this.dataAccessConfig = dataAccessConfig; this.transaction = transaction; } + private initializeComponents( + httpConfig?: Partial<ClientTypes.IHttpDataAccessConfig>, + nodeConnectionConfig?: Partial<NodeConnectionConfig>, + persist: boolean = true + ) { + const dataAccessConfig = new HttpDataAccessConfig({ httpConfig, nodeConnectionConfig }); + const transaction = new HttpTransaction(dataAccessConfig); + const reader = new HttpDataRead(dataAccessConfig); + const writer = persist + ? new HttpDataWrite(dataAccessConfig, transaction) + : new NoPersistDataWrite(); + return { dataAccessConfig, transaction, reader, writer }; + }packages/request-client.js/src/http-metamask-data-write.ts (2)
16-16: Avoid using 'any' type forstorageMetaIn the cache definition,
storageMetais typed asany. Consider defining a more specific type forstorageMetato enhance type safety and maintainability.
59-64: Verify the necessity of the network name checkThe condition
if (!this.networkName)may always be false sincethis.networkNameis initialized with a default value'private'in the constructor. Consider removing this condition or adjusting the logic if necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
packages/data-access/src/combined-data-access.ts(2 hunks)packages/data-access/src/index.ts(1 hunks)packages/data-access/src/no-persist-data-write.ts(1 hunks)packages/request-client.js/src/api/request-network.ts(4 hunks)packages/request-client.js/src/http-data-access-config.ts(1 hunks)packages/request-client.js/src/http-data-access.ts(8 hunks)packages/request-client.js/src/http-data-read.ts(1 hunks)packages/request-client.js/src/http-data-write.ts(1 hunks)packages/request-client.js/src/http-metamask-data-access.ts(5 hunks)packages/request-client.js/src/http-metamask-data-write.ts(1 hunks)packages/request-client.js/src/http-request-network.ts(2 hunks)packages/request-client.js/src/http-transaction.ts(1 hunks)packages/request-client.js/src/index.ts(1 hunks)packages/request-client.js/test/in-memory-request.test.ts(2 hunks)packages/transaction-manager/test/index.test.ts(19 hunks)packages/types/src/data-access-types.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/request-client.js/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/data-access/src/index.ts
- packages/data-access/src/combined-data-access.ts
- packages/transaction-manager/test/index.test.ts
- packages/types/src/data-access-types.ts
- packages/request-client.js/src/api/request-network.ts
🧰 Additional context used
📓 Learnings (1)
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-11-12T14:52:33.204Z
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.
🪛 Biome (1.9.4)
packages/request-client.js/test/in-memory-request.test.ts
[error] 22-24: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (5)
packages/request-client.js/src/http-request-network.ts (1)
60-60: LGTM! Clean refactoring of persistence logicThe changes simplify the code by removing the conditional instantiation of
NoPersistHttpDataAccessand using a singleHttpDataAccessclass with apersistparameter. This improves maintainability and reduces code complexity.packages/request-client.js/src/http-data-access.ts (1)
12-14: LGTM! Good use of inheritance and compositionThe class now extends
CombinedDataAccessand uses composition withdataAccessConfigandtransactionproperties, improving modularity and separation of concerns.packages/request-client.js/test/in-memory-request.test.ts (1)
81-98: LGTM! Good test coverage for custom data accessThe test case thoroughly verifies the creation of requests using custom data access implementation.
packages/request-client.js/src/http-metamask-data-access.ts (2)
1-11: LGTM! Good architectural improvement.The transition to extend
CombinedDataAccessand the introduction of composition-based imports align well with the PR's objective of enabling custom DataAccess implementations.
87-87: LGTM! Good implementation of the new architecture.The refactored methods properly delegate to the appropriate components while maintaining error handling:
persistTransactiondelegates to the writer componentgetTransactionsByChannelIdusesdataAccessConfigfor fetching with proper retry configurationAlso applies to: 100-107
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
packages/request-client.js/test/api/request-network.test.ts (2)
19-19: LGTM! Consider adding test cases for persistence behavior.The addition of
isPersistingmock aligns with the interface changes. However, the test coverage could be enhanced.Consider adding test cases to verify:
- Behavior when
isPersistingreturnsfalse- Interaction between persistence state and request creation/updates
- Edge cases where persistence state changes during operations
Example test structure:
describe('persistence behavior', () => { it('should handle non-persisting data access', async () => { const mockNonPersistingDataAccess = { ...mockDataAccess, isPersisting: jest.fn().mockReturnValue(false), }; const requestNetwork = new RequestNetwork({ dataAccess: mockNonPersistingDataAccess }); // Test request creation/updates with non-persisting data access }); it('should handle persistence state changes', async () => { const mockDynamicDataAccess = { ...mockDataAccess, isPersisting: jest.fn() .mockReturnValueOnce(true) .mockReturnValueOnce(false), }; const requestNetwork = new RequestNetwork({ dataAccess: mockDynamicDataAccess }); // Test behavior when persistence state changes }); });
Line range hint
11-21: Consider separating persistence-related tests.Based on the project's testing patterns, persistence-related tests are typically kept separate to avoid test interference. Consider moving the persistence behavior tests to a dedicated test file, such as
request-network-persistence.test.ts.This separation would:
- Maintain consistency with existing test organization
- Prevent potential interference with other test cases
- Provide a clear location for all persistence-related test scenarios
packages/request-client.js/src/http-data-access.ts (1)
Line range hint
15-23: Add JSDoc documentation for the persist parameterConsider adding documentation for the
persistparameter in the constructor's JSDoc comment to explain its purpose and impact:/** * Creates an instance of HttpDataAccess. * @param httpConfig @see ClientTypes.IHttpDataAccessConfig for available options. * @param nodeConnectionConfig Configuration options to connect to the node. + * @param persist Whether to persist transactions. When false, implements "Pay before Persist" functionality. */packages/request-client.js/src/http-data-access-config.ts (3)
22-46: Consider making the default baseURL configurable.The default baseURL is currently hardcoded to 'http://localhost:3000'. Consider making this configurable through environment variables or configuration files to support different environments.
this.nodeConnectionConfig = { - baseURL: 'http://localhost:3000', + baseURL: process.env.DEFAULT_BASE_URL || 'http://localhost:3000', headers: {
48-74: Enhance error handling in fetchAndRetry.Consider adding error handling to provide more context about the failure and potentially log retry attempts:
public async fetchAndRetry<T = unknown>( path: string, params: Record<string, unknown>, retryConfig: { maxRetries?: number; retryDelay?: number; exponentialBackoffDelay?: number; maxExponentialBackoffDelay?: number; } = {}, ): Promise<T> { retryConfig.maxRetries = retryConfig.maxRetries ?? this.httpConfig.httpRequestMaxRetry; retryConfig.retryDelay = retryConfig.retryDelay ?? this.httpConfig.httpRequestRetryDelay; retryConfig.exponentialBackoffDelay = retryConfig.exponentialBackoffDelay ?? this.httpConfig.httpRequestExponentialBackoffDelay; retryConfig.maxExponentialBackoffDelay = retryConfig.maxExponentialBackoffDelay ?? this.httpConfig.httpRequestMaxExponentialBackoffDelay; - return await retry(async () => await this.fetch<T>('GET', path, params), retryConfig)(); + try { + return await retry( + async (attempt) => { + console.debug(`Attempt ${attempt + 1}/${retryConfig.maxRetries} for ${path}`); + return await this.fetch<T>('GET', path, params); + }, + retryConfig + )(); + } catch (error) { + throw Object.assign(error, { + message: `Failed to fetch ${path} after ${retryConfig.maxRetries} attempts: ${error.message}`, + path, + params, + }); + }
106-109: Enhance error information.Consider including more context in the error object to aid in debugging:
- throw Object.assign(new Error(r.statusText), { + throw Object.assign(new Error(`${method} ${url} failed: ${r.statusText}`), { status: r.status, statusText: r.statusText, + url: url.toString(), + method, + params, + body, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/request-client.js/src/http-data-access-config.ts(1 hunks)packages/request-client.js/src/http-data-access.ts(8 hunks)packages/request-client.js/src/http-metamask-data-access.ts(3 hunks)packages/request-client.js/test/api/request-network.test.ts(1 hunks)packages/request-client.js/test/in-memory-request.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/request-client.js/test/in-memory-request.test.ts
- packages/request-client.js/src/http-metamask-data-access.ts
🧰 Additional context used
📓 Learnings (3)
packages/request-client.js/test/api/request-network.test.ts (1)
Learnt from: MantisClone
PR: RequestNetwork/requestNetwork#1386
File: packages/request-client.js/src/api/request-network.ts:25-25
Timestamp: 2024-11-12T14:52:33.204Z
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/http-data-access-config.ts (1)
Learnt from: kevindavee
PR: RequestNetwork/requestNetwork#1541
File: packages/request-client.js/src/http-data-read.ts:34-37
Timestamp: 2025-01-10T04:25:32.038Z
Learning: In PR #1541, the HttpDataRead class was created by extracting existing logic from HttpDataAccess without adding new validations or features, to maintain a focused refactoring scope.
packages/request-client.js/src/http-data-access.ts (1)
Learnt from: kevindavee
PR: RequestNetwork/requestNetwork#1541
File: packages/request-client.js/src/http-data-read.ts:34-37
Timestamp: 2025-01-10T04:25:32.038Z
Learning: In PR #1541, the HttpDataRead class was created by extracting existing logic from HttpDataAccess without adding new validations or features, to maintain a focused refactoring scope.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (6)
packages/request-client.js/src/http-data-access.ts (4)
3-14: Well-structured architectural changes!The refactoring improves code organization by:
- Extending
CombinedDataAccessfor better code reuse- Introducing clear separation between read and write operations
- Properly encapsulating configuration and transaction handling
24-28: Make thepersistparameter optional in type definitionThe type definition makes
persistrequired (persist: boolean), but the constructor provides a default value. This is contradictory and could cause TypeScript compilation errors.
35-46: Clean initialization with good separation of concerns!The constructor properly initializes components in a logical order:
- Configuration
- Transaction handler
- Reader
- Conditional writer based on persistence flag
78-78: Consistent implementation of delegation pattern!The methods properly delegate to their respective components:
- Transaction operations to writer/reader
- HTTP operations to dataAccessConfig
- Maintains existing validation logic
This approach improves maintainability and follows single responsibility principle.
Also applies to: 88-88, 101-101, 116-116, 131-131, 139-139, 156-158
packages/request-client.js/src/http-data-access-config.ts (2)
1-8: LGTM! Clean imports and type definitions.The imports are well-organized, and the
NodeConnectionConfigtype is properly exported with clear, typed properties.
10-20: LGTM! Well-documented class properties.The class properties are clearly defined with descriptive JSDoc comments explaining their purposes.
kevindavee
force-pushed
the
feat/allow-no-persist-for-custom-data-access
branch
from
977e88c to
f68fbff
Compare
|
@alexandre-abrioux @MantisClone , this PR is ready to review again |
There was a problem hiding this comment.
Very clean, well done 👌
I have one comment regarding naming of this new boolean variable. I think we can regroup the different names (isPersisting, persist, skipPersistence) into one. I would vote for skipPersistence: to me the wording is more explicit since the default is to persist, so opting-in to the feature of not persisting means setting skipPersistence to true.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/request-client.js/src/http-data-access-config.ts (1)
102-109: Enhance error handling with response body.The error handling could be improved by including the response body in the error object for better debugging.
if (r.ok) { return await r.json(); } + const errorBody = await r.text(); throw Object.assign(new Error(r.statusText), { status: r.status, statusText: r.statusText, + body: errorBody, });packages/request-client.js/test/in-memory-request.test.ts (2)
14-22: Add error handling test cases for MyCustomDataAccess.While the class implementation is good, consider adding test cases for error scenarios:
- Invalid pendingStore initialization
- Error during DataAccessRead operations
79-96: Add assertions for request state.The test verifies basic request creation, but consider adding assertions for:
- Request state after creation
- Persistence state of the custom data access
- Error cases when trying to modify the request
// Add these assertions expect(request.state).toBe('created'); expect(myCustomDataAccess.skipPersistence()).toBe(true); await expect(request.modify({})).rejects.toThrow();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/data-access/src/combined-data-access.ts(2 hunks)packages/request-client.js/src/api/request-network.ts(4 hunks)packages/request-client.js/src/http-data-access-config.ts(1 hunks)packages/request-client.js/src/http-data-access.ts(8 hunks)packages/request-client.js/src/http-request-network.ts(2 hunks)packages/request-client.js/test/api/request-network.test.ts(1 hunks)packages/request-client.js/test/in-memory-request.test.ts(2 hunks)packages/transaction-manager/test/index.test.ts(19 hunks)packages/types/src/data-access-types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/request-client.js/test/api/request-network.test.ts
- packages/types/src/data-access-types.ts
- packages/transaction-manager/test/index.test.ts
- packages/data-access/src/combined-data-access.ts
- packages/request-client.js/src/api/request-network.ts
🧰 Additional context used
📓 Learnings (4)
packages/request-client.js/src/http-request-network.ts (1)
Learnt from: kevindavee
PR: RequestNetwork/requestNetwork#1541
File: packages/request-client.js/src/http-data-access-config.ts:76-110
Timestamp: 2025-01-10T05:15:26.574Z
Learning: When refactoring code into new classes in the Request Network codebase, maintain the existing logic and behavior without modifications to minimize risk and keep changes focused on the extraction process.
packages/request-client.js/src/http-data-access.ts (2)
Learnt from: kevindavee
PR: RequestNetwork/requestNetwork#1541
File: packages/request-client.js/src/http-data-access-config.ts:76-110
Timestamp: 2025-01-10T05:15:26.574Z
Learning: When refactoring code into new classes in the Request Network codebase, maintain the existing logic and behavior without modifications to minimize risk and keep changes focused on the extraction process.
Learnt from: kevindavee
PR: RequestNetwork/requestNetwork#1541
File: packages/request-client.js/src/http-data-read.ts:34-37
Timestamp: 2025-01-10T04:25:32.038Z
Learning: In PR #1541, the HttpDataRead class was created by extracting existing logic from HttpDataAccess without adding new validations or features, to maintain a focused refactoring scope.
packages/request-client.js/src/http-data-access-config.ts (2)
Learnt from: kevindavee
PR: RequestNetwork/requestNetwork#1541
File: packages/request-client.js/src/http-data-access-config.ts:76-110
Timestamp: 2025-01-10T05:15:26.574Z
Learning: When refactoring code into new classes in the Request Network codebase, maintain the existing logic and behavior without modifications to minimize risk and keep changes focused on the extraction process.
Learnt from: kevindavee
PR: RequestNetwork/requestNetwork#1541
File: packages/request-client.js/src/http-data-read.ts:34-37
Timestamp: 2025-01-10T04:25:32.038Z
Learning: In PR #1541, the HttpDataRead class was created by extracting existing logic from HttpDataAccess without adding new validations or features, to maintain a focused refactoring scope.
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-11-12T14:52:33.204Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-and-test
🔇 Additional comments (5)
packages/request-client.js/src/http-request-network.ts (2)
12-12: LGTM! Clean import updates.The imports have been correctly updated to reflect the new structure, importing
NodeConnectionConfigfrom the new location.Also applies to: 15-15
60-60: LGTM! Clean constructor refactoring.The constructor has been simplified to use the new
HttpDataAccesswithskipPersistenceparameter, making the code more maintainable and easier to understand.packages/request-client.js/src/http-data-access.ts (3)
12-14: LGTM! Clean class inheritance update.The class now properly extends
CombinedDataAccessand declares necessary properties with appropriate access modifiers.
35-46: LGTM! Well-structured initialization.The constructor initialization is clean and follows a good pattern:
- Creates configuration first
- Initializes transaction handler
- Sets up reader and writer based on skipPersistence
- Calls super with dependencies
- Stores necessary references
78-78: LGTM! Consistent delegation pattern.The methods consistently delegate to the appropriate components (reader/writer/transaction), maintaining good separation of concerns.
Also applies to: 88-88, 101-101, 116-116, 131-131
kevindavee
force-pushed
the
feat/allow-no-persist-for-custom-data-access
branch
from
88fe992 to
7f36372
Compare
|
@MantisClone , shall I wait for your approval? This PR is supposed to |
|
No worries @MantisClone ! Thank you |
Description of the changes
To use Pay before Persist feature, it depends on
NoPersistHttpDataAccessand this class inherits fromHttpDataAccess. If we want to use our ownDataAccess, we can't use Pay Before Persist feature.This change allows builders to use their own custom
DataAccessand decide whether they want to persist it or not with thepersistparameter. We no longer needNoPersistHttpDataAccesssince the persistence logic will be handled in each of theDataAccessclass.If this change is accepted, we might not need #1540 .
Summary by CodeRabbit
Based on the comprehensive changes across multiple files, here are the updated release notes:
New Features
skipPersistence()method to data access interfaces to control transaction persistence behavior.NoPersistDataWriteclass for handling non-persistent transaction scenarios.HttpDataReadandHttpDataWriteclasses for enhanced HTTP data operations.HttpTransactionclass for managing transaction confirmations.Improvements
Refactoring
NoPersistHttpDataAccessclass to simplify the data access model.Breaking Changes