fix!: UnmarshalJSON is limited to the default Tree bug #277

sontrinh16 · 2023-12-13T16:36:10Z

Overview

Closed: #275

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

rootulp · 2023-12-13T16:40:12Z

@sontrinh16 test fails here

sontrinh16 · 2023-12-13T16:40:48Z

yeah i have to move to draft to fix this

sontrinh16 · 2023-12-13T17:21:23Z

alright seem all test pass now, sorry for the inconvenience guys

Wondertan · 2023-12-13T17:23:00Z

Thanks for fixing this! Committing to review this tmrw

rootulp

Thanks for the contribution! Excited to see this land

tree.go

extendeddatasquare.go

extendeddatacrossword_test.go

codecov · 2023-12-13T20:25:08Z

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (35d7798) 80.76% compared to head (c988b3d) 80.89%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
extendeddatasquare.go	58.33%	6 Missing and 4 partials ⚠️
default_tree.go	90.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #277      +/-   ##
==========================================
+ Coverage   80.76%   80.89%   +0.12%     
==========================================
  Files           7        8       +1     
  Lines         806      869      +63     
==========================================
+ Hits          651      703      +52     
- Misses        112      119       +7     
- Partials       43       47       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

extendeddatasquare.go

tree.go

staheri14

LGTM!

tree_test.go

tree.go

Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>

staheri14

Thanks for addressing the comments. LGTM!

extendeddatasquare.go

rootulp

I have a few comments about the tests that were added. Also one question about what happens if UnmarshalJSON is invoked on a tree that doesn't contain the Tree field. Do we want a test case for that scenario? I assume it should unmarshal with the default tree.

tree_test.go

sontrinh16 · 2024-01-09T19:44:56Z

I have a few comments about the tests that were added. Also one question about what happens if UnmarshalJSON is invoked on a tree that doesn't contain the Tree field. Do we want a test case for that scenario? I assume it should unmarshal with the default tree.

hmmmmm yeah i think we could add default tree as a fallback when Tree field is missing

rootulp

Overall LGTM. A few nits before we merge this. Thanks for all your hard work!

tree_test.go

extendeddatasquare_test.go

sontrinh16 · 2024-01-10T18:45:04Z

Overall LGTM. A few nits before we merge this. Thanks for all your hard work!

thanks for persevering through all the typo errors guys

rootulp

fantastic work

evan-forbes · 2024-01-11T14:00:53Z

approving as I don't want to block, but we should have @Wondertan review things again to ensure this is exactly what they wanted

meaning, we could merge and just not release until one last approval or wait for that approval

thanks a lot tho @sontrinh16 this took a lot

Wondertan

We wanted JSONUnmarshal to work with the correct tree builder, and we got it here without breaking anything, so approve ;)

…tiaorg#277)" This reverts commit bb5e119.

While attempting to bump celestia-app to the v0.12.0-rc2, I noticed that the `RegisterTree` design leaks an implementation detail to celestia-app: the registering and managing of `treeName`s. Celestia-app has two categories of of trees: 1. erasured namespaced merkle tree in [nmt_wrapper.go](https://github.com/celestiaorg/celestia-app/blob/main/pkg/wrapper/nmt_wrapper.go) 2. EDS subtree root cacher [nmt_caching.go](https://github.com/celestiaorg/celestia-app/blob/main/pkg/inclusion/nmt_caching.go) Each of those categories has trees based on square size and NMT options. Celestia-app needs to be careful to register all the appropriate trees once (and only once) before they are used (via `Compute` or `Import`). I'd like to explore a less breaking option to get celestia-node the original desired feature which was #275. In the meantime, I think we should revert the two big breaking changes so that main can remain release-able. Revert #277 Revert #287 Closes #295 because no longer relevant if we merge this.

sontrinh16 added 3 commits December 12, 2023 12:07

refator default tree to seperated file

80f81c4

add tree contructors global map

5ee186c

Merge branch 'main' into global_tree

936527b

sontrinh16 changed the title ~~[fix] UnmarshalJSON is limited to the default Tree Bug~~ [fix] UnmarshalJSON is limited to the default Tree bug Dec 13, 2023

rootulp changed the title ~~[fix] UnmarshalJSON is limited to the default Tree bug~~ fix: UnmarshalJSON is limited to the default Tree bug Dec 13, 2023

sontrinh16 marked this pull request as draft December 13, 2023 16:39

rootulp assigned sontrinh16 Dec 13, 2023

sontrinh16 added 3 commits December 14, 2023 00:09

accept tree name as arg instead

cde39fa

pull upstream

3502a53

register test tree constructor

c79bb47

sontrinh16 marked this pull request as ready for review December 13, 2023 17:20

rootulp reviewed Dec 13, 2023

View reviewed changes

rootulp requested a review from Wondertan December 13, 2023 19:34

sontrinh16 changed the title ~~fix: UnmarshalJSON is limited to the default Tree bug~~ fix!: UnmarshalJSON is limited to the default Tree bug Dec 13, 2023

sontrinh16 added 4 commits December 14, 2023 03:17

naming convention for tree name

857a175

fix test

503c757

fix lint

e52fbdf

Merge branch 'main' into global_tree

404dfa1

Wondertan requested changes Dec 14, 2023

View reviewed changes

extendeddatasquare.go Outdated Show resolved Hide resolved

extendeddatasquare.go Outdated Show resolved Hide resolved

sontrinh16 added 3 commits December 15, 2023 02:09

move constructor fetching to unmarshalJSON

15cfd82

pull upstream

051f085

minor

28aac5b

sontrinh16 requested a review from Wondertan December 14, 2023 19:24

Wondertan reviewed Dec 15, 2023

View reviewed changes

tree.go Show resolved Hide resolved

tree.go Outdated Show resolved Hide resolved

tree.go Outdated Show resolved Hide resolved

adding comment and privatise func

4e25dd3

staheri14 previously approved these changes Jan 5, 2024

View reviewed changes

tree_test.go Outdated Show resolved Hide resolved

tree_test.go Show resolved Hide resolved

tree.go Show resolved Hide resolved

minor update

b53cf8c

Co-authored-by: Sanaz Taheri <35961250+staheri14@users.noreply.github.com>

sontrinh16 dismissed stale reviews from staheri14 and rootulp via b53cf8c January 5, 2024 20:18

sontrinh16 added 3 commits January 6, 2024 03:18

Merge branch 'main' into global_tree

64f3e0a

add comment for test functions

2c07d00

minor

a7d1eb0

staheri14 previously approved these changes Jan 8, 2024

View reviewed changes

extendeddatasquare.go Show resolved Hide resolved

rootulp reviewed Jan 9, 2024

View reviewed changes

sontrinh16 added 3 commits January 10, 2024 13:11

add default tree as fallback

a2baef2

fix typo

0b16ad8

use assert for tree fn compare

3c31d8e

sontrinh16 dismissed staheri14’s stale review via 3c31d8e January 10, 2024 06:20

sontrinh16 added 2 commits January 10, 2024 13:22

lint fix

8067e13

add test for unmarshal json

6dde5d7

rootulp reviewed Jan 10, 2024

View reviewed changes

minor nit

002c3b0

rootulp approved these changes Jan 10, 2024

View reviewed changes

evan-forbes approved these changes Jan 11, 2024

View reviewed changes

rootulp requested a review from Wondertan January 11, 2024 16:48

Merge branch 'main' into global_tree

c988b3d

Wondertan approved these changes Jan 30, 2024

View reviewed changes

evan-forbes merged commit bb5e119 into celestiaorg:main Jan 30, 2024
5 checks passed

rootulp mentioned this pull request Jan 30, 2024

getTreeNameFromConstructorFn does not work for registered trees #286

Closed

rootulp added a commit to rootulp/rsmt2d that referenced this pull request Feb 3, 2024

Revert "fix!: UnmarshalJSON is limited to the default Tree bug (celes…

eaac726

…tiaorg#277)" This reverts commit bb5e119.

rootulp mentioned this pull request Feb 3, 2024

revert!: rsmt2d#277 and rsmt2d#287 #295

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix!: UnmarshalJSON is limited to the default Tree bug #277

fix!: UnmarshalJSON is limited to the default Tree bug #277

sontrinh16 commented Dec 13, 2023 •

edited

Loading

rootulp commented Dec 13, 2023

sontrinh16 commented Dec 13, 2023

sontrinh16 commented Dec 13, 2023

Wondertan commented Dec 13, 2023

rootulp left a comment

codecov bot commented Dec 13, 2023 •

edited

Loading

staheri14 left a comment

staheri14 left a comment

rootulp left a comment

sontrinh16 commented Jan 9, 2024

rootulp left a comment •

edited

Loading

sontrinh16 commented Jan 10, 2024

rootulp left a comment

evan-forbes commented Jan 11, 2024

Wondertan left a comment

fix!: UnmarshalJSON is limited to the default Tree bug #277

fix!: UnmarshalJSON is limited to the default Tree bug #277

Conversation

sontrinh16 commented Dec 13, 2023 • edited Loading

Overview

Checklist

rootulp commented Dec 13, 2023

sontrinh16 commented Dec 13, 2023

sontrinh16 commented Dec 13, 2023

Wondertan commented Dec 13, 2023

rootulp left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 13, 2023 • edited Loading

Codecov Report

staheri14 left a comment

Choose a reason for hiding this comment

staheri14 left a comment

Choose a reason for hiding this comment

rootulp left a comment

Choose a reason for hiding this comment

sontrinh16 commented Jan 9, 2024

rootulp left a comment • edited Loading

Choose a reason for hiding this comment

sontrinh16 commented Jan 10, 2024

rootulp left a comment

Choose a reason for hiding this comment

evan-forbes commented Jan 11, 2024

Wondertan left a comment

Choose a reason for hiding this comment

sontrinh16 commented Dec 13, 2023 •

edited

Loading

codecov bot commented Dec 13, 2023 •

edited

Loading

rootulp left a comment •

edited

Loading