-
Notifications
You must be signed in to change notification settings - Fork 104
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
Lock down token transfer to AssetHub only #1047
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1047 +/- ##
==========================================
+ Coverage 81.31% 81.34% +0.02%
==========================================
Files 54 54
Lines 2243 2246 +3
Branches 71 71
==========================================
+ Hits 1824 1827 +3
Misses 402 402
Partials 17 17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -111,6 +130,12 @@ where | |||
command: Command::AgentExecute { agent_id, command: agent_execute_command }, | |||
}; | |||
|
|||
// filter out message | |||
if !ExportFilter::contains(&local_sub, &outbound_message) { |
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.
Notice that ExportFilter
is declared as EverythingBut<NonAssetHubTokenTransfers>
in runtime, so could it be simplified as NonAssetHubTokenTransfers
and remove the not operation here?
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.
Yeah, I was also a bit unsure on which way to go with this, so I looked at other "Filter" constraints in the Polkadot codebase to see what is the standard way, and it seems that if the filter returns true, the call should be allowed. This allows you to use the traits Everything
to allow all, and Nothing
to block all.
Examples:
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.
IMHO the filters above are more of a whitelist whereas we're blocking a blacklist item here? So will it make more sense to rename it as ExportInterceptor
and make the simplification?
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.
What does local_sub
mean? Local subscription?
@@ -111,6 +130,12 @@ where | |||
command: Command::AgentExecute { agent_id, command: agent_execute_command }, | |||
}; | |||
|
|||
// filter out message | |||
if !ExportFilter::contains(&local_sub, &outbound_message) { |
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.
What does local_sub
mean? Local subscription?
fa311d4
to
42499fe
Compare
0ab70eb
to
b6fafe4
Compare
Resolves: SNO-771
Polkadot-Sdk: Snowfork/polkadot-sdk#62