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

feat: index genesis transactions #34

Merged
merged 40 commits into from
Sep 13, 2024
Merged

Conversation

n0izn0iz
Copy link
Contributor

@n0izn0iz n0izn0iz commented Apr 14, 2024

Depends on gnolang/gno#1941

Screenshot 2024-06-11 at 15 11 01

@n0izn0iz n0izn0iz force-pushed the handle-genesis branch 4 times, most recently from cbfd80e to 19c991f Compare April 15, 2024 01:00
Signed-off-by: Norman Meier <[email protected]>
@jefft0
Copy link

jefft0 commented Jun 3, 2024

Related to gnolang/gno#1941

@n0izn0iz n0izn0iz changed the title feat: handle genesis feat: index genesis transactions Jun 11, 2024
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
Signed-off-by: Norman Meier <[email protected]>
@zivkovicmilos
Copy link
Member

@n0izn0iz
I think you can just add a special "genesis" fetch before we start the main run routines for fetching blocks / block result chunks, since for the genesis block we only need the block results (block 0 is unfetchable) as part of gnolang/gno#1941

@n0izn0iz
Copy link
Contributor Author

agree, would simplify, was planning on doing that

Signed-off-by: Norman Meier <[email protected]>
@n0izn0iz
Copy link
Contributor Author

added tests in 763c68a

Copy link
Collaborator

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

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

Please, check the tests. Mocks are used to simulate dependencies our system interacts with. This helps us focus on testing the real functionality without checking the mock itself.

To make sure our tests are testing what is needed, we need to test not just the happy path but also different possible errors. Here are some scenarios we should include:

  • Remote Server Failure: What happens if the remote server fails? We need to make sure our system handles server issues properly.
  • Genesis Retrieval Issues: What happens if the genesis data can’t be retrieved? This could be due to network problems, data corruption, or other issues.
  • Unexpected Genesis Object Type: What happens if the genesis object is not the expected type? Type mismatches can cause unexpected behavior, so our tests should check how the system deals with this.

These scenarios are important, but we should also think of other possible cases to ensure our system is robust and reliable.

fetch/fetch_test.go Outdated Show resolved Hide resolved
@n0izn0iz
Copy link
Contributor Author

improved tests in 0a31d61

it helped me catch nil deref corner cases, thanks

@n0izn0iz n0izn0iz requested a review from ajnavarro July 31, 2024 15:34
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Looking at the implementation now, I think we made a right choice in the relevant gno genesis PR 👍

I've left a few comments that should be addressed before we merge, but nothing groundbreaking 🙏

cmd/start.go Outdated Show resolved Hide resolved
fetch/mocks_test.go Outdated Show resolved Hide resolved
fetch/fetch.go Show resolved Hide resolved
fetch/fetch.go Show resolved Hide resolved
fetch/fetch.go Outdated Show resolved Hide resolved
fetch/fetch.go Show resolved Hide resolved
fetch/fetch.go Outdated Show resolved Hide resolved
fetch/fetch.go Outdated Show resolved Hide resolved
@n0izn0iz
Copy link
Contributor Author

n0izn0iz commented Aug 15, 2024

there is one remaining problem, maybe the indexer should ignore genesis result if the node returns the well known "height must be greater than 0" otherwise we can't index test4 anymore

btw just saw that test3 returns "Height must be greater than 0" (yes the error message lost capitalization on "height" in test4)

@zivkovicmilos
Copy link
Member

@n0izn0iz I'll fix up the tests on this branch 🙏

@zivkovicmilos
Copy link
Member

@n0izn0iz

I think we got the PR to a working state -- but there is a problem:

The indexer will break for networks that don't have the genesis results in their storage.

I've circumvented this problem by providing the genesis state fetch error as a soft one, which doesn't derail the indexer:
ca459d3

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

Thank you for the fixup 🙏

Copy link
Collaborator

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

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

Tested locally, it won't break current indexes.

@ajnavarro ajnavarro merged commit 3e524a7 into gnolang:main Sep 13, 2024
4 checks passed
@n0izn0iz n0izn0iz deleted the handle-genesis branch September 13, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants