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.

Sign up for GitHub

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

Jump to bottom

feat!: drop legacy storage #1117

Merged
merged 48 commits into from
Oct 25, 2023
Merged

feat!: drop legacy storage #1117

merged 48 commits into from
Oct 25, 2023

Conversation

benjlevesque
Copy link
Contributor

@benjlevesque benjlevesque commented Jun 20, 2023 •
edited
Loading

BREAKING CHANGE: Dropping legacy indexing mechanism.

Changes:

  • enforce usage of TheGraph as a backend for request-node
  • drop all custom indexing code (mostly DataAccess)
  • refactor DataAccess to rely on an Indexer
  • refactor mock to use in-memory indexer
  • reduce dependency to TheGraph by abstracting most of the code transformation. TheGraph dependency is now down to only the SubgraphClient.

@benjlevesque benjlevesque changed the base branch from master to refactor/request-node/config June 20, 2023 22:17
@benjlevesque benjlevesque changed the title feat/drop legacy storage feat!: drop legacy storage Jun 20, 2023
Base automatically changed from refactor/request-node/config to master June 23, 2023 08:43
@MantisClone
Copy link
Member

I recognize that the legacy indexing mechanism doesn't work on many chains, is difficult to maintain, and that no one wants to spend the time to fix it. For those reasons, I agree that removing it now is the right move.

Nonetheless, would it be possible to leave injection points such that someone could plug in their own custom indexer into the protocol?

@benjlevesque
Copy link
Contributor Author

I recognize that the legacy indexing mechanism doesn't work on many chains, is difficult to maintain, and that no one wants to spend the time to fix it. For those reasons, I agree that removing it now is the right move.

Nonetheless, would it be possible to leave injection points such that someone could plug in their own custom indexer into the protocol?

Yes @MantisClone; that would be best indeed! These injection points are still available, but not as the Node level (same as today). I've introduced a new layer called "indexer" that represents better the way things are (it would actually be possible to keep the old code with this indexer layer, but it's not efficient so better remove it).

This PR is still draft because I'm trying to reduce the surface of TheGraph in the code; it should be down to only the Subgraph Client, graphql queries and types. The rest could be generalised to work with any type of indexer.

@benjlevesque benjlevesque force-pushed the feat/drop-legacy-storage branch 2 times, most recently from f449cb1 to 70119e9 Compare July 25, 2023 15:30
benjlevesque
benjlevesque commented Oct 6, 2023
View reviewed changes
packages/request-node/src/server.ts
@@ -1,42 +1,29 @@
#!/usr/bin/env node
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file now only creates the server object, but doesn't start it. startNode is called from bin.ts

benjlevesque
benjlevesque commented Oct 6, 2023
View reviewed changes
packages/request-node/src/requestNode.ts
@@ -1,36 +1,182 @@
import { DataAccess, TransactionIndex } from '@requestnetwork/data-access';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

requestNodeBase.ts moved to requestNode.ts

benjlevesque
benjlevesque commented Oct 6, 2023
View reviewed changes
packages/data-access/src/data-read.ts
@@ -0,0 +1,194 @@
import { DataAccessTypes, StorageTypes } from '@requestnetwork/types';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved from packages/thegraph-data-access/src/data-access.ts

benjlevesque
benjlevesque commented Oct 6, 2023
View reviewed changes
packages/data-access/src/pending-store.ts
@@ -0,0 +1,32 @@
import { DataAccessTypes } from '@requestnetwork/types';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved from packages/thegraph-data-access/src/data-access.ts

yomarion
yomarion approved these changes Oct 6, 2023
View reviewed changes
Copy link
Member

@yomarion yomarion left a comment

Choose a reason for hiding this comment

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

Massive!

packages/usage-examples/src/request-client-js-erc20-request.ts Outdated Show resolved Hide resolved
packages/request-client.js/src/http-metamask-data-access.ts Outdated Show resolved Hide resolved
packages/request-node/test/getConfirmedTransaction.test.ts Show resolved Hide resolved
packages/thegraph-data-access/src/subgraph-client.ts Show resolved Hide resolved
MantisClone
MantisClone reviewed Oct 9, 2023
View reviewed changes
Copy link
Member

@MantisClone MantisClone left a comment

Choose a reason for hiding this comment

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

I'm about 1/4th of the way through the review.

README.md Show resolved Hide resolved
MantisClone
MantisClone approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@MantisClone MantisClone left a comment

Choose a reason for hiding this comment

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

Impressive refactor! Only a few minor comments.

packages/request-client.js/src/http-request-network.ts Show resolved Hide resolved
packages/request-node/src/dataAccess.ts Show resolved Hide resolved
packages/request-node/src/dataAccess.ts Show resolved Hide resolved
packages/request-node/test/getChannelsByTopic.test.ts Show resolved Hide resolved
packages/request-node/test/getChannelsByTopic.test.ts Show resolved Hide resolved
packages/thegraph-data-access/src/types.ts Show resolved Hide resolved
alexandre-abrioux
alexandre-abrioux approved these changes Oct 12, 2023
View reviewed changes
Copy link
Member

@alexandre-abrioux alexandre-abrioux left a comment

Choose a reason for hiding this comment

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

👏

@benjlevesque benjlevesque mentioned this pull request Oct 12, 2023
Closed
@benjlevesque benjlevesque merged commit 9021cf2 into master Oct 25, 2023
2 of 4 checks passed
@benjlevesque benjlevesque deleted the feat/drop-legacy-storage branch October 25, 2023 16:52
This was referenced Oct 26, 2023
Open
Closed
@benjlevesque benjlevesque mentioned this pull request Nov 13, 2023
Merged
@benjlevesque
Copy link
Contributor Author

benjlevesque commented Nov 14, 2023 •
edited
Loading

NB: from now on, the IPFS_URL environment variable replaces IPFS_HOST, IPFS_PORT and IPFS_PROTOCOL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MantisClone MantisClone MantisClone approved these changes

@alexandre-abrioux alexandre-abrioux alexandre-abrioux approved these changes

@yomarion yomarion yomarion approved these changes

@kevindavee kevindavee Awaiting requested review from kevindavee

@leoslr leoslr Awaiting requested review from leoslr

@skiv71 skiv71 Awaiting requested review from skiv71

Assignees

@benjlevesque benjlevesque

Labels
None yet
Projects
Request Network Tech Backlog
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@benjlevesque @MantisClone @coveralls @alexandre-abrioux @yomarion