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): JSON marshaling for sdk Address wrapper #2348

Merged
merged 4 commits into from
Jun 14, 2023

Conversation

distractedm1nd
Copy link
Collaborator

While debugging #2322 , I noticed that it is not only an RPC issue, but also a broken API one. All RPC methods that used the type state.Address were unusable, both via the client and via raw JSON calls. This is because the server was unable to marshal the address string value back into the interface type.

To circumvent this, I have embedded the sdk.Address type in the same way that we embed the fraud proof type, to allow us to unmarshal it back into a concrete type. I have also added unit tests to fix this.

In addition, it fixes the RPC parsing - so it closes #2322 .

@distractedm1nd distractedm1nd added kind:fix Attached to bug-fixing PRs area:api Related to celestia-node API labels Jun 8, 2023
@distractedm1nd distractedm1nd self-assigned this Jun 8, 2023
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.

So the core issue here is that cosmos sdk addresses does not implement json serialisation and reflection does not work automatically, right?

state/state.go Show resolved Hide resolved
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.

Thanks the tests!

state/state.go Show resolved Hide resolved
state/state.go Show resolved Hide resolved
state/address_test.go Show resolved Hide resolved
@codecov-commenter
Copy link

Codecov Report

Merging #2348 (41b1fe4) into main (c41738a) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2348      +/-   ##
==========================================
- Coverage   50.84%   50.82%   -0.02%     
==========================================
  Files         154      155       +1     
  Lines        9746     9760      +14     
==========================================
+ Hits         4955     4961       +6     
- Misses       4355     4365      +10     
+ Partials      436      434       -2     
Impacted Files Coverage Δ
api/gateway/state.go 7.85% <0.00%> (ø)
cmd/celestia/rpc.go 10.00% <0.00%> (+0.11%) ⬆️
state/core_access.go 27.14% <0.00%> (ø)
state/state.go 0.00% <0.00%> (ø)

... and 4 files with indirect coverage changes

cmd/celestia/rpc.go Show resolved Hide resolved
state/state.go Show resolved Hide resolved
@distractedm1nd distractedm1nd enabled auto-merge (squash) June 14, 2023 08:08
@distractedm1nd distractedm1nd merged commit 55db897 into main Jun 14, 2023
@distractedm1nd distractedm1nd deleted the rpc-parsing-address branch June 14, 2023 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Related to celestia-node API kind:fix Attached to bug-fixing PRs
Projects
None yet
5 participants