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!(types): Add Blob::index field introduced in celestia 0.13 #274

Merged
merged 4 commits into from
May 10, 2024

Conversation

S1nus
Copy link
Contributor

@S1nus S1nus commented May 6, 2024

Another attempt at adding this field. Now, trying to use an Option<u64>.

Copy link
Member

@zvolin zvolin left a comment

Choose a reason for hiding this comment

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

Looks good 👍 left some suggestions

@zvolin
Copy link
Member

zvolin commented May 6, 2024

could you look into the CI failures too?

@zvolin
Copy link
Member

zvolin commented May 6, 2024

some of the failures seems to be coming from toolchain updates, I'll look into that tomorrow

@zvolin
Copy link
Member

zvolin commented May 7, 2024

I have a fix for clippy and docs failures in #275. Unfortunately the rest of Lumina team is off-site, so if I won't find any reviewer, this will have to wait till Thursday.

I think those test failures are related to this change tho https://github.com/eigerco/lumina/actions/runs/8976014314/job/24653654722?pr=274

Copy link
Member

@fl0rek fl0rek left a comment

Choose a reason for hiding this comment

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

looks good, thanks for the contribution! 🎸

@fl0rek
Copy link
Member

fl0rek commented May 8, 2024

@S1nus I've just noticed that the commits aren't signed
Would you mind setting up commit signing and force-pushing? Checks won't allow merge unless all commits are signed.

@zvolin
Copy link
Member

zvolin commented May 8, 2024

Hey @S1nus, I took closer look on why do the tests fail. Hope you didn't mind my interference 🙏

So it turned out that the index field is always set when making requests using celestia blob submit ... cli. Example request caught using mitmproxy looks like:

{
    "id": 1,
    "jsonrpc": "2.0",
    "meta": {
        "SpanContext": "AABS1z6HOiQ+SS/vuHkNa2YrARBcp1onueEeAgA="
    },
    "method": "blob.Submit",
    "params": [
        [
            {
                "commitment": "27wECh7o43HkyXIRS0kcVMoJD9iWVNIEARmP4AlsjeM=",
                "data": "IkhlbGxvIHdvcmxkIg==",
                "index": -1,
                "namespace": "AAAAAAAAAAAAAAAAAAAAAAAAAAAADCBNOWAP3dM=",
                "share_version": 0
            }
        ],
        -1
    ]
}

@zvolin
Copy link
Member

zvolin commented May 8, 2024

this also makes sure that binary serializers will always serialize / deserialize the same type, this isn't issue in case of jsons, but could be for other like bincode or risc0

@zvolin zvolin requested a review from fl0rek May 8, 2024 11:23
@zvolin zvolin changed the title blob-index as option field feat!(types): Add Blob::index field introduced in celestia 0.13 May 8, 2024
@S1nus S1nus force-pushed the cnode/blob-index branch from 32891fc to 41ae86c Compare May 9, 2024 18:08
@S1nus
Copy link
Contributor Author

S1nus commented May 9, 2024

@fl0rek I set up signing, newer commits are signed. is it good? or need to do something else?

@zvolin zvolin force-pushed the cnode/blob-index branch from 42876ec to 42975c7 Compare May 10, 2024 09:25
@zvolin zvolin force-pushed the cnode/blob-index branch from 42975c7 to 5eb6554 Compare May 10, 2024 09:31
@zvolin
Copy link
Member

zvolin commented May 10, 2024

Previous commits needed to be signed too, but I just squashed them and it looks fine. I've added the fix for celestia-rpc tests, hopefully CI will be happy now

@fl0rek fl0rek merged commit 594e9fa into eigerco:main May 10, 2024
6 checks passed
@zvolin zvolin mentioned this pull request May 8, 2024
This was referenced May 20, 2024
@zvolin zvolin mentioned this pull request Jun 12, 2024
@zvolin zvolin mentioned this pull request Jul 5, 2024
@zvolin zvolin mentioned this pull request Jul 25, 2024
@zvolin zvolin mentioned this pull request Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants