fix(offchain): Enforce entities list on offchain data sources #4147
Conversation
leoyvens
force-pushed
the
leo/enforce-enitites-on-offchain
branch
from
417fe9f to
c3e1634
Compare
| @@ -42,6 +42,33 @@ pub enum DataSourceCreationError { | |||
| Unknown(#[from] Error), | |||
| } | |||
|
|
|||
| /// Which entity types a data source can read and write to. | |||
| pub enum EntityTypeAccess { | |||
There was a problem hiding this comment.
Might be worth adding a bit more here about how this interacts with causality region based isolation
There was a problem hiding this comment.
I've written a more detailed comment.
| fn check_entity_type_access(&self, entity_type: &EntityType) -> Result<(), HostExportError> { | ||
| match self.entity_type_access.allows(entity_type) { | ||
| true => Ok(()), | ||
| false => Err(HostExportError::Deterministic(anyhow!( |
There was a problem hiding this comment.
Could this error be added as a typed error? Seems quite specific to be matched on string so I think it could be useful to have an IsolationError subtype
There was a problem hiding this comment.
It could, but given that for now we don't need to match on it I think it's fine to just use the Deterministic variant.
| @@ -97,6 +99,20 @@ impl<C: Blockchain> HostExports<C> { | |||
| } | |||
| } | |||
|
|
|||
| fn check_entity_type_access(&self, entity_type: &EntityType) -> Result<(), HostExportError> { | |||
There was a problem hiding this comment.
even though this is not exported I think we should a bit of context here as a comment
There was a problem hiding this comment.
I've written a short comment and linked it to the one on enum EntityTypeAccess.
|
|
||
| let stop_block = test_ptr(5); | ||
| let err = ctx.start_and_sync_to_error(stop_block).await; | ||
| let message = "entity type `IpfsFile1` is not on the 'entities' list for data source `File2`. \ |
There was a problem hiding this comment.
would it be enough to check if this is an isolation error somehow (left a suggestion about subtype but that might be lost). Maybe we could match an important part of the match like IpfsFile1 or somnething rather than the full message which also includes a backtrace. As it is it will be potentially broken if this function changes.
There was a problem hiding this comment.
If the test breaks when functionality changes, that's good! Gives us a chance to check that the change was intended. Even if its for the backtrace which is not directly related to what's being tested.
|
@mangas thanks for the review! I've addressed the comments. |
First part of #3770.
This enforces that file data sources can only call
entity.get,entity.setandentity.removeon entity types they've declared in theentities:array in the manifest.