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

service/state: Provide simple scaffold for StateService and implement CoreAccess #420

Merged
merged 33 commits into from
Mar 30, 2022

Conversation

renaynay
Copy link
Member

@renaynay renaynay commented Feb 9, 2022

This PR is the first implementation of StateAccessor over core: CoreAccess. It will provide a light node with the ability to be initialised with a connection to a celestia-core endpoint over which it can query for account balances and submit transactions.

ADR for reference.

Two notes:

  1. Although it was documented in the ADR, I decided against using the lens dependency as it is still in development and introduced a bit more overhead / complexity than was necessary for two simple queries against the application.
  2. It is extremely difficult to write unit tests that test the full functionality of CoreAccess as we lack a good way to mock the celestia-app for testing purposes. This will be discussed in the following week and will hopefully provide more clarity in how we can increase test coverage for this feature.

TODO:

  • doc.go
  • swamp test
  • start/stop hooks w/ DI
  • construction
  • lint
  • update ADR

@renaynay renaynay marked this pull request as ready for review February 11, 2022 21:33
@renaynay renaynay changed the title [ DRAFT ] service/state: Provide simple scaffold for StateService and implement CoreAccess service/state: Provide simple scaffold for StateService and implement CoreAccess Feb 11, 2022
@renaynay renaynay added the area:state Related to fetching state and state execution label Feb 11, 2022
Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Lint error

node/services/config.go Outdated Show resolved Hide resolved
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

Although it was documented in the ADR, I decided against using the lens dependency as it is still in development and introduced a bit more overhead / complexity than was necessary for two simple queries against the application

Do you mind quickly updating the ADR accordingly in this PR? I've missed the updated ADR 🙈

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

👏🏼

node/state/core.go Outdated Show resolved Hide resolved
Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Merge conflicts

@renaynay
Copy link
Member Author

Rebased.

@renaynay
Copy link
Member Author

FYI the test is currently broken as mock celestia-app isn't working.

Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Lint errors!

service/state/core_access.go Outdated Show resolved Hide resolved
service/state/core_access.go Outdated Show resolved Hide resolved
service/state/core_access.go Show resolved Hide resolved
@renaynay
Copy link
Member Author

renaynay commented Mar 1, 2022

Test is failing due to failure in mock celestia-app

Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

Is CI expected to fail, or no?

@renaynay
Copy link
Member Author

renaynay commented Mar 3, 2022

Yes @adlerjohn

Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

image

Mostly looks good and straightforward. Good that the implementation turned out to be quite small even without lens.

Only a few things that require changes, other than that are mostly nits, questions, and discussions on how things should look further.

Also, part of the review process resulted into #415 (comment)

docs/adr/adr-004-state-interaction.md Outdated Show resolved Hide resolved
service/state/interface.go Outdated Show resolved Hide resolved
service/state/core_access.go Show resolved Hide resolved
service/state/core_access.go Show resolved Hide resolved
go.mod Show resolved Hide resolved
service/state/core_access.go Outdated Show resolved Hide resolved
node/components.go Outdated Show resolved Hide resolved
node/state/core.go Outdated Show resolved Hide resolved
service/state/core_access.go Outdated Show resolved Hide resolved
node/services/config.go Outdated Show resolved Hide resolved
renaynay and others added 23 commits March 30, 2022 13:14
Co-authored-by: Hlib Kanunnikov <hlibwondertan@gmail.com>
@renaynay renaynay force-pushed the state-scaffold-without-lens branch from 3fa7e5e to db90c98 Compare March 30, 2022 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:state Related to fetching state and state execution kind:feat Attached to feature PRs
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants