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

refactor: "feat: amending computeDigest func. + related test cases (#2132)" #2180

Merged
merged 3 commits into from
Nov 8, 2023

Conversation

ABresting
Copy link
Contributor

@ABresting ABresting commented Nov 2, 2023

This reverts commit 1669f71.

Description

As discussed with Hanno earlier, this will revert the computeDigest PR introduced in Waku archive and associated test cases changes as well.
We are doing this to make sure that we keep original behavior intact and use waku_core https://github.com/waku-org/nwaku/blob/master/waku/waku_core/message/digest.nim#L29 for computing messageHash attribute of database.

Changes

  • revert the commit

Issue

#2112

Copy link

github-actions bot commented Nov 2, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2180

Built from aad9645

@ABresting ABresting changed the title Revert "feat: amending computeDigest func. + related test cases (#2132)" Revert feat: amending computeDigest func. + related test cases (#2132) Nov 2, 2023
@ABresting ABresting changed the title Revert feat: amending computeDigest func. + related test cases (#2132) refactor "feat: amending computeDigest func. + related test cases (#2132)" Nov 2, 2023
@ABresting ABresting changed the title refactor "feat: amending computeDigest func. + related test cases (#2132)" refactor: "feat: amending computeDigest func. + related test cases (#2132)" Nov 2, 2023
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

I'm a bit confused, as I didn't realise that this was already merged into master. These changes seemed to still be part of the recently opened PR that changed the DB schema, but perhaps it was simply a polluted diff?
It would be good to understand what the actual impact is in terms of client implementation backwards compatibility if the underlying cursor is changed.

@ABresting
Copy link
Contributor Author

ABresting commented Nov 2, 2023

I'm a bit confused, as I didn't realise that this was already merged into master. These changes seemed to still be part of the recently opened PR that changed the DB schema, but perhaps it was simply a polluted diff? It would be good to understand what the actual impact is in terms of client implementation backwards compatibility if the underlying cursor is changed.

This was a separate PR since on for issue #2112 tasks are done serially to have a visible separation component-wise merging in, that PR modified the computeDigest functionality.

AFAIK, this change only improved the cursor functionality since with new digest/id computed as per the RFC value, in js-Waku it failed a test case value and it was due to the existing digest functionality not updated in the jsWaku client.

in jsWaku and goWaku client it passed all test cases. About backwards compatibility idk tbh. @jm-clius WDYT?

@ABresting ABresting merged commit d7ef3ca into master Nov 8, 2023
3 checks passed
@ABresting ABresting deleted the revert-1669f710 branch November 8, 2023 00:41
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.

None yet

3 participants