-
Notifications
You must be signed in to change notification settings - Fork 81
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
lp-forwarder: Always use Centrifuge domain when wrapping outbound mes… #1980
base: main
Are you sure you want to change the base?
Conversation
63b41a0
to
365fcb3
Compare
365fcb3
to
173c44f
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.
Super simple solution, thanks @cdamian. Just one minor thing
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1980 +/- ##
==========================================
+ Coverage 48.98% 49.00% +0.01%
==========================================
Files 183 183
Lines 13202 13202
==========================================
+ Hits 6467 6469 +2
+ Misses 6735 6733 -2 ☔ View full report in Codecov by Sentry. |
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.
LGTM! Thanks @cdamian.
A question, the check of line: https://github.com/centrifuge/centrifuge-chain/pull/1980/files#diff-d763817d4b90117d6295fc32e8fd8a66d2ee4093bfb40d90941a3368e72aa099R244-R247 makes sense? I mean, should we treat the forward info domain as a whitelist for domains?
I would leave it like that for now and get back to it after we discuss the rest of the LP concerns. Furthermore, setting forwarding info can only be done by admins so we can trust it for the time being anyway IMO. TBD if we need extra/different validation. |
Description
The current implementation is using the stored domain when wrapping forward messages, however, this is incorrect since we should always use
Domain::Centrifuge
.Checklist