Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove node-localstorage, add StorageProvider interface #1508

Merged
merged 4 commits into from
Dec 5, 2024

Conversation

rodrigopavezi
Copy link
Member

@rodrigopavezi rodrigopavezi commented Dec 4, 2024

Description of the changes

Fixes #1501

refactor: remove node-localstorage dependency and update storage handling in LitProvider

  • Removed node-localstorage from package.json and yarn.lock.
  • Updated LitProvider to use a new StorageProvider interface.
  • Refactored storage handling logic to improve compatibility with Node.js.
  • Enhanced the RequestNetwork class to include getter and setter for cipherProvider.
  • Modified GraphQL queries to improve transaction retrieval logic and pagination settings.

This change streamlines storage management and enhances the overall architecture of the project.

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced LitProvider class with improved type safety for storage providers.
    • Introduced optional cipherProvider functionality in the RequestNetwork class, allowing better management of encryption features.
  • Improvements

    • Updated GraphQL queries to focus on transactions directly, improving data retrieval efficiency.
    • Increased pagination limits for transaction retrieval, allowing for larger data sets to be handled in a single request.
  • Bug Fixes

    • Adjusted Jest test timeouts to ensure tests complete successfully without premature termination.

…ling in LitProvider

- Removed node-localstorage from package.json and yarn.lock.
- Updated LitProvider to use a new StorageProvider interface.
- Refactored storage handling logic to improve compatibility with Node.js.
- Enhanced the RequestNetwork class to include getter and setter for cipherProvider.
- Modified GraphQL queries to improve transaction retrieval logic and pagination settings.

This change streamlines storage management and enhances the overall architecture of the project.
Copy link
Contributor

coderabbitai bot commented Dec 4, 2024

Walkthrough

This pull request includes modifications to several files across different packages. Key changes involve the removal of the node-localstorage dependency from the @requestnetwork/lit-protocol-cipher package, updates to the LitProvider class to enhance type safety, and the introduction of a cipherProvider property in the RequestNetwork class. Additionally, the GraphQL query in queries.ts has been adjusted to focus on transactions, and pagination settings have been updated in subgraph-client.ts. Finally, the docker-compose.yml file has been modified to change the build context and Dockerfile path for the graph-deploy service.

Changes

File Change Summary
packages/lit-protocol-cipher/package.json Removed node-localstorage from dependencies and @types/node-localstorage from devDependencies.
packages/lit-protocol-cipher/src/lit-protocol-cipher-provider.ts Added nodeJsStorageProvider property and updated constructor and methods to use the new storage provider.
packages/request-client.js/src/api/request-network.ts Added cipherProvider property and methods getCipherProvider and setCipherProvider to RequestNetwork.
packages/thegraph-data-access/src/queries.ts Modified GetTransactionsByTopics query to focus on transactions instead of channels, adjusting pagination parameters.
packages/thegraph-data-access/src/subgraph-client.ts Increased DEFAULT_PAGE_SIZE and MAX_PAGE_SIZE, simplified transaction retrieval logic in getTransactionsByTopics.
packages/request-node/test/getTransactionsByChannelId.test.ts Increased Jest test timeout from 20,000 ms to 30,000 ms.
packages/request-node/test/persistTransaction.test.ts Added Jest test timeout setting to 30,000 ms.
docker-compose.yml Updated graph-deploy service's build context and Dockerfile path, modified IPFS_HOST environment variable.

Possibly related PRs

Suggested reviewers

  • alexandre-abrioux
  • sstefdev
  • aimensahnoun
  • MantisClone
  • kevindavee

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

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

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

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

Other keywords and placeholders

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

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)
packages/thegraph-data-access/src/queries.ts (1)

82-89: Consider performance implications of case-insensitive search

The switch to topics_contains_nocase for topic matching provides better usability but might impact query performance on large datasets. Consider:

  1. Adding an index hint if available in your GraphQL implementation
  2. Documenting the performance characteristics for consumers
packages/thegraph-data-access/src/subgraph-client.ts (1)

83-92: Consider adding error handling for large result sets

The pagination logic has been simplified, but there's no explicit handling for cases where the response might be truncated by the server or when dealing with rate limits.

Consider adding:

 const indexedTransactions = response.transactions.map(this.toIndexedTransaction);
+
+// Add warning if response size equals page size
+if (indexedTransactions.length === effectivePageSize) {
+  console.warn(`Retrieved maximum number of results (${effectivePageSize}). Some data might be truncated.`);
+}

 return {
   transactions: indexedTransactions,
packages/lit-protocol-cipher/src/lit-protocol-cipher-provider.ts (2)

80-86: Add validation for nodeJsStorageProvider in Node.js environment

While the storage provider integration is correct, consider adding validation to ensure nodeJsStorageProvider is provided when running in Node.js environment to prevent potential runtime errors.

 else {
+  if (!this.nodeJsStorageProvider) {
+    throw new Error('nodeJsStorageProvider is required in Node.js environment');
+  }
   const { LitNodeClientNodeJs } = await import('@lit-protocol/lit-node-client');
   this.client = new LitNodeClientNodeJs({
     litNetwork: this.network,
     storageProvider: this.nodeJsStorageProvider,

Also applies to: 105-108


129-131: Enhance type safety for storage provider clear operation

Consider adding type checking for the clear method to ensure it exists on the provider interface.

 if (this.nodeJsStorageProvider) {
+  if (typeof this.nodeJsStorageProvider.provider.clear !== 'function') {
+    throw new Error('Storage provider must implement clear method');
+  }
   this.nodeJsStorageProvider.provider.clear();
 }
packages/request-client.js/src/api/request-network.ts (1)

80-93: Add validation in setCipherProvider method

Consider adding validation to ensure the provided cipherProvider implements the required interface methods.

 public setCipherProvider(cipherProvider: CipherProviderTypes.ICipherProvider): void {
+  // Validate required methods
+  const requiredMethods = ['encrypt', 'decrypt', 'isEncryptionAvailable', 'isDecryptionAvailable'];
+  for (const method of requiredMethods) {
+    if (typeof cipherProvider[method] !== 'function') {
+      throw new Error(`CipherProvider must implement ${method} method`);
+    }
+  }
   this.cipherProvider = cipherProvider;
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between cf4c125 and 8abdd8a.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (5)
  • packages/lit-protocol-cipher/package.json (1 hunks)
  • packages/lit-protocol-cipher/src/lit-protocol-cipher-provider.ts (6 hunks)
  • packages/request-client.js/src/api/request-network.ts (2 hunks)
  • packages/thegraph-data-access/src/queries.ts (1 hunks)
  • packages/thegraph-data-access/src/subgraph-client.ts (2 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/lit-protocol-cipher/src/lit-protocol-cipher-provider.ts (1)
Learnt from: rodrigopavezi
PR: RequestNetwork/requestNetwork#1475
File: packages/lit-protocol-cipher/src/lit-protocol-cipher-provider.ts:124-153
Timestamp: 2024-11-21T08:59:19.774Z
Learning: The code in `packages/lit-protocol-cipher/src/lit-protocol-cipher-provider.ts` that follows the Lit Protocol Documentation should not be modified.
🔇 Additional comments (5)
packages/lit-protocol-cipher/package.json (1)

53-53: Verify ethers version compatibility

The package uses ethers v5.7.2, which is not the latest major version. Consider evaluating the upgrade to ethers v6.x for potential improvements and security updates, while being mindful of breaking changes.

packages/thegraph-data-access/src/subgraph-client.ts (2)

74-81: LGTM: Improved query parameter handling

The query parameters are now more clearly structured and the GraphQL request is more concise.


22-23: Verify pagination limits with GraphQL server

The significant increase in page sizes (DEFAULT_PAGE_SIZE to 100 and MAX_PAGE_SIZE to 1000) could impact:

  1. Memory usage on both client and server
  2. Response times for large result sets
  3. Server-side query timeout limits

Please ensure these values align with the GraphQL server's capabilities.

✅ Verification successful

Based on the GraphQL implementation findings, I can now provide a final response:

The increased page sizes are safe to use with The Graph protocol

The implementation uses The Graph's pagination parameters (first and skip) which are well-suited for these increased limits:

  • The DEFAULT_PAGE_SIZE of 100 is a common default for GraphQL APIs
  • The MAX_PAGE_SIZE of 1000 is within The Graph's capabilities
  • The query implementation properly handles pagination through first and skip parameters

These values are reasonable and align with common GraphQL server implementations, including The Graph protocol which is being used here.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any server-side pagination limits in the codebase
echo "Checking for GraphQL server configuration..."
rg --type ts "pageSize|limit" -g "**/server/**"

Length of output: 297


Script:

#!/bin/bash
# Let's search for pagination-related configurations in the entire codebase
echo "Searching for pagination configurations..."
rg --type ts "pageSize|limit" -A 2 -B 2

# Also check if we're using any GraphQL client configurations
echo -e "\nChecking GraphQL client configurations..."
fd -e ts -e js -e json "graphql" --exec grep -l "limit\|page"

Length of output: 40172


Script:

#!/bin/bash
# Let's check the GraphQL query implementation to understand the server-side limits
echo "Checking GraphQL query implementation..."
rg -A 10 "query GetTransactions" --type ts

# Also check if there are any GraphQL schema definitions
echo -e "\nChecking GraphQL schema definitions..."
fd -e graphql . --exec cat {}

Length of output: 8420

packages/lit-protocol-cipher/src/lit-protocol-cipher-provider.ts (1)

11-11: LGTM! Type safety improvements

The addition of the StorageProvider type import and the properly typed nodeJsStorageProvider property enhances type safety and code maintainability.

Also applies to: 58-58

packages/request-client.js/src/api/request-network.ts (1)

43-44: LGTM! Well-structured property addition

The cipherProvider property is properly encapsulated and initialized, following TypeScript best practices.

Also applies to: 77-78

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
packages/data-access/src/data-read.ts (2)

Line range hint 137-159: Code Quality Improvement: Simplify Data Structures in Reducers

The reducers used for constructing storageMeta, transactionsStorageLocation, and transactions (lines 137-159) are currently initializing empty objects with type assertions. Consider simplifying the initialization by specifying the type directly, which improves readability and maintains type safety.

Apply this diff to refactor the reducers:

- storageMeta: paginatedTxs.reduce((acc, tx) => {
-   acc[tx.channelId] = [this.toStorageMeta(tx, result.blockNumber, this.network)];
-   return acc;
- }, {} as Record<string, StorageTypes.IEntryMetadata[]>),
+ storageMeta: paginatedTxs.reduce<Record<string, StorageTypes.IEntryMetadata[]>>((acc, tx) => {
+   acc[tx.channelId] = [this.toStorageMeta(tx, result.blockNumber, this.network)];
+   return acc;
+ }, {}),

- transactionsStorageLocation: paginatedTxs.reduce((prev, curr) => {
-   if (!prev[curr.channelId]) {
-     prev[curr.channelId] = [];
-   }
-   prev[curr.channelId].push(curr.hash);
-   return prev;
- }, {} as Record<string, string[]>),
+ transactionsStorageLocation: paginatedTxs.reduce<Record<string, string[]>>((prev, curr) => {
+   if (!prev[curr.channelId]) {
+     prev[curr.channelId] = [];
+   }
+   prev[curr.channelId].push(curr.hash);
+   return prev;
+ }, {}),

- transactions: paginatedTxs.reduce((prev, curr) => {
-   if (!prev[curr.channelId]) {
-     prev[curr.channelId] = [];
-   }
-   prev[curr.channelId].push(this.toTimestampedTransaction(curr));
-   return prev;
- }, {} as DataAccessTypes.ITransactionsByChannelIds),
+ transactions: paginatedTxs.reduce<DataAccessTypes.ITransactionsByChannelIds>((prev, curr) => {
+   if (!prev[curr.channelId]) {
+     prev[curr.channelId] = [];
+   }
+   prev[curr.channelId].push(this.toTimestampedTransaction(curr));
+   return prev;
+ }, {}),

This refactor enhances code readability and leverages TypeScript's type inference for more maintainable code.


131-134: Edge Case Handling: Ensure Pagination Indices Are Within Bounds

When calculating the start and end indices for pagination (lines 131-134), there is a possibility that start could exceed the number of available transactions, resulting in an empty paginatedTxs array. This could occur if the page parameter is too large.

Consider adding a check to ensure that start is within the bounds of filteredTxs.length and adjust accordingly to prevent returning an empty result when transactions are available.

Example adjustment:

const totalTxs = filteredTxs.length;
const start = Math.min(((page || 1) - 1) * (pageSize || totalTxs), totalTxs);

This change ensures that pagination indices are correctly calculated, enhancing user experience when navigating through pages.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 455b28c and cfd2861.

📒 Files selected for processing (1)
  • packages/data-access/src/data-read.ts (2 hunks)

packages/data-access/src/data-read.ts Outdated Show resolved Hide resolved
@MantisClone MantisClone changed the title fix: lit protocol changes fix: remove node-localstorage, add StorageProvider interface Dec 4, 2024
@MantisClone
Copy link
Member

The ethereum-storage tests flaked so I'm re-running them.
The integration-tests failed due to a change in the pagination logic - @rodrigopavezi please take a look.

- Modified docker-compose.yml to adjust the build context and dockerfile path for the graph-deploy service.
- Enhanced data access logic in data-read.ts to streamline transaction filtering and pagination.
- Updated GraphQL queries in queries.ts to correctly retrieve transactions by topics and ensure proper ordering.
- Refactored subgraph-client.ts to handle channel-based transaction retrieval, improving data organization and pagination handling.

These changes improve the overall structure and functionality of the data access layer.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
docker-compose.yml (1)

55-56: LGTM: Build configuration changes improve precision

The build context is now more precisely specified, which helps reduce the build context size and improves build performance. The simplified Dockerfile path correctly aligns with the new context.

Consider documenting these Docker-related changes in the repository's documentation to help other developers understand the build structure.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between cfd2861 and 18103d7.

📒 Files selected for processing (3)
  • docker-compose.yml (1 hunks)
  • packages/thegraph-data-access/src/queries.ts (1 hunks)
  • packages/thegraph-data-access/src/subgraph-client.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/thegraph-data-access/src/queries.ts
  • packages/thegraph-data-access/src/subgraph-client.ts
🔇 Additional comments (1)
docker-compose.yml (1)

64-64: Verify IPFS protocol change impact

The addition of the http:// prefix makes the protocol explicit, which is good practice. However, we should verify this change is compatible with all dependent services.

✅ Verification successful

Let me analyze the code that interacts with IPFS to verify the protocol compatibility.


Let me check the actual IPFS client implementation and configuration usage to understand how the URL is handled.


Let me check the actual configuration file to see how the IPFS URL is defined and handled.


Based on the analysis of the codebase, I can now provide a final response about the IPFS protocol change impact.

IPFS URL format change is compatible with all dependencies

The codebase consistently handles IPFS URLs with both formats (with and without protocol prefix):

  • The default configuration in packages/ethereum-storage/src/config.ts already uses http://localhost:5001
  • The IPFS client implementation properly handles URLs with explicit protocol
  • The change in docker-compose.yml aligns with the existing configuration patterns
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if other services or configurations might be affected by the IPFS URL format change

# Search for IPFS URL patterns in the codebase
echo "Searching for IPFS URL patterns:"
rg -g '!{*.lock,yarn.lock,package-lock.json}' 'ipfs:5001|localhost:5001'

# Search for IPFS configuration patterns
echo "Searching for IPFS configuration patterns:"
rg -g '!{*.lock,yarn.lock,package-lock.json}' 'IPFS_HOST|ipfs.*host'

Length of output: 689


Script:

#!/bin/bash
# Search for IPFS client initialization or URL usage patterns
ast-grep --pattern 'new IPFS($$$)' 
ast-grep --pattern 'create($$$IPFS$$$)'

# Look for IPFS related imports and configurations
rg -A 3 'import.*ipfs|require.*ipfs' 

# Check for environment variable usage
rg 'process\.env\.IPFS_HOST'

Length of output: 1816


Script:

#!/bin/bash
# Check IPFS manager and configuration implementations
rg -A 10 'class IpfsManager|interface IpfsOptions' packages/ethereum-storage/src/ipfs-manager.ts

# Look for URL/host configuration usage
rg -A 5 'ipfs.*url|ipfs.*host' packages/ethereum-storage/src/

# Check for environment configuration handling
rg -A 5 'config.*ipfs|ipfs.*config' packages/ethereum-storage/src/

Length of output: 3647


Script:

#!/bin/bash
# Look for the configuration file content
rg -A 10 'ipfs.*url.*=|ipfs:.*{' packages/ethereum-storage/src/config.ts

# Check for any default configuration files
fd -g "*config*.{js,ts,json}" -x cat {} 2>/dev/null

Length of output: 50813

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove node-localstorage
3 participants