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!(state): use node blob type instead of app type to fix inconsistent unmarshalling #2338

Merged
merged 5 commits into from
Jun 8, 2023

Conversation

distractedm1nd
Copy link
Collaborator

@distractedm1nd distractedm1nd commented Jun 7, 2023

Breaks API to v0.2.0

Because state.SubmitPayForBlob was using apptypes.Blob, which cannot be unmarshalled from a node blob.Blob JSON object, it was requiring two separate types downstream. Now both blob Service and state Service use blob.Blob type.

The caveat here is that I had to move the blob service implementation inside nodebuilder, as we do for header service as well - due to import cycles. This is not a long term solution, as eventually we will share the blob type with app

This means introducing an interface in blob.Service called blob.Submitter to avoid that state module import.

@distractedm1nd distractedm1nd added kind:break! Attached to breaking PRs kind:fix Attached to bug-fixing PRs labels Jun 7, 2023
@distractedm1nd distractedm1nd self-assigned this Jun 7, 2023
@distractedm1nd
Copy link
Collaborator Author

Going to rework this PR after a good suggestion from @vgonkivs to just have blob service take an interface for submitting data instead of moving blob/service to nodebuilder

@distractedm1nd distractedm1nd marked this pull request as draft June 7, 2023 13:11
@vgonkivs
Copy link
Member

vgonkivs commented Jun 7, 2023

It will help to break dependency blob -> state

@distractedm1nd distractedm1nd marked this pull request as ready for review June 8, 2023 07:53
Copy link
Member

@vgonkivs vgonkivs left a comment

Choose a reason for hiding this comment

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

One small comment. Other than that LGTM. Thank you.

api/gateway/state_test.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2023

Codecov Report

Merging #2338 (19eabc4) into main (f632908) will decrease coverage by 0.04%.
The diff coverage is 17.64%.

@@            Coverage Diff             @@
##             main    #2338      +/-   ##
==========================================
- Coverage   50.89%   50.86%   -0.04%     
==========================================
  Files         154      154              
  Lines        9746     9742       -4     
==========================================
- Hits         4960     4955       -5     
+ Misses       4352     4350       -2     
- Partials      434      437       +3     
Impacted Files Coverage Δ
cmd/celestia/rpc.go 9.88% <0.00%> (+0.05%) ⬆️
state/core_access.go 27.14% <0.00%> (-0.40%) ⬇️
api/gateway/state.go 7.85% <40.00%> (-3.03%) ⬇️
blob/service.go 72.61% <50.00%> (+1.68%) ⬆️

... and 4 files with indirect coverage changes

vgonkivs
vgonkivs previously approved these changes Jun 8, 2023
Wondertan
Wondertan previously approved these changes Jun 8, 2023
blob/service.go Show resolved Hide resolved
blob/service.go Show resolved Hide resolved
api/gateway/state.go Show resolved Hide resolved
api/gateway/state_test.go Show resolved Hide resolved
nodebuilder/node/admin.go Outdated Show resolved Hide resolved
walldiss
walldiss previously approved these changes Jun 8, 2023
@distractedm1nd distractedm1nd merged commit 0823f6b into main Jun 8, 2023
@distractedm1nd distractedm1nd deleted the pfb-submit-parity branch June 8, 2023 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:break! Attached to breaking PRs kind:fix Attached to bug-fixing PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants