-
Notifications
You must be signed in to change notification settings - Fork 20
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: P-654 added solana signature check #2635
Conversation
6e8c895
to
e2bc891
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.
Do we have a place where we put shared/common code between workers? if not, what's the reason?
This is under discussion. Check issue P-600 and related ones. |
The changes looks good to me. Can you take a look at the CI test failure of one worker? |
e2bc891
to
25b2aa5
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.
Shall we have ts-test for it (at least happy path)?
25b2aa5
to
f3bb395
Compare
ed6caa8
to
b2a75c3
Compare
Yes integration tests have been added too. |
I'm not sure if the test is run in CI though |
ecea4b9
to
9721570
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.
Thanks!
Context
as titles
Labels
Please apply following PR-related labels when appropriate:
C0-breaking
: if your change could break the existing client, e.g. API change, critical logic changeC1-noteworthy
: if your change is non-breaking, but is still worth noticing for the client, e.g. reference code improvementHow (Optional)
Testing Evidences
Please attach any relevant evidences if applicable
Tested via unit test