-
Notifications
You must be signed in to change notification settings - Fork 43
fix(store): fix validation to allow store query by hash #2330
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
base: master
Are you sure you want to change the base?
Conversation
bb686dd
to
453f4ef
Compare
size-limit report 📦
|
453f4ef
to
645b543
Compare
645b543
to
beb60c6
Compare
beb60c6
to
fb54552
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the validation logic in the Store RPC to allow queries by message hash and prevents conflicts with content filter criteria. Key changes include:
- Adjusting the validation in the core and SDK store modules to handle message hash queries.
- Updating tests to cover message hash query scenarios.
- Adding a dependency for message hash functionality in the tests package.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
packages/tests/tests/store/utils.ts | Updated sendMessages to support optional timestamps. |
packages/tests/tests/store/message_hash.spec.ts | Added tests to verify message hash queries. |
packages/tests/package.json | Added dependency for "@waku/message-hash". |
packages/sdk/src/store/store.ts | Introduced queryByMessageHashes and adjusted regular query flow. |
packages/core/src/lib/store/store.ts | Modified content topic validation for message hash queries. |
packages/core/src/lib/store/rpc.ts | Adjusted validation to allow message hash queries without content filters. |
Comments suppressed due to low confidence (1)
packages/core/src/lib/store/store.ts:49
- Using toString() for comparing arrays may be brittle and order sensitive; consider using a more robust array equality check such as comparing sorted arrays.
queryOpts.contentTopics.toString() !== Array.from(decoders.keys()).toString()
}); | ||
|
||
it("can query messages by message hash", async function () { | ||
// Send messages first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's not have not needed comments :)
here and in other places/PRs too
AI sometimes adds it, but I think it doesn't bring value as we are literally getting:
// send messages
await sendMessages();
cc @waku-org/js-waku
} | ||
} | ||
|
||
// Note: The real issue might be that message hash lookup is not properly supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am missing the value of the test here. It would succeed even if no messages are found, right?
pubsubTopic: string | ||
): Promise<void> { | ||
pubsubTopic: string, | ||
timestamp: boolean = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timestamp: boolean = false | |
includeTimestamp: boolean = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments, overall looks good!
did you validate is in SDS context?
Problem / Description
Validation logic in the our implementation of the Store RPC requests prevented queries based on message hashes from going through
Solution
Notes
Checklist