[CHIA-2041] Simplify action scopes and add support for nesting #19013
Conversation
Quexington
added
the
Added
Quexington
changed the title
Quexington
changed the title
| @@ -99,10 +30,9 @@ class ActionScope(Generic[_T_SideEffects, _T_Config]): | |||
| from interfering with each other. | |||
| """ | |||
|
|
|||
| _resource_manager: ResourceManager | |||
| _side_effects_format: type[_T_SideEffects] | |||
| _lock: Callable[..., contextlib.AbstractAsyncContextManager[Any]] | |||
There was a problem hiding this comment.
This seems unused.
Depending on the intended usage this comment may not apply but generally when you take a callable it's not useful to have ... for parameters since you can't fill that and rather it needs to satisfy a specific parameter list like [].
Assuming it is supposed to be used, and noting that the tests are not failing, I wonder if there's a missing test that should be written around the exclusivity.
|
|
||
| await self._resource_manager.save_resource(interface.side_effects) | ||
| self._callback = interface.callback | ||
| new_interface = copy.deepcopy(self._active_interface) |
There was a problem hiding this comment.
deepcopy() is pretty implicit so let's chat about this some.
| yield self._active_interface | ||
| self._active_interface._callbacks_allowed = previous_interface._callbacks_allowed | ||
| for field, value in self._active_interface.__dict__.items(): | ||
| previous_interface.__dict__[field] = value |
There was a problem hiding this comment.
Is this trying to avoid a setattr()? Not all attributes are in .__dict__ necessarily. At least https://docs.python.org/3.12/reference/datamodel.html#slots.
| for field, value in self._active_interface.__dict__.items(): | ||
| previous_interface.__dict__[field] = value | ||
| finally: | ||
| self._active_interface = previous_interface |
There was a problem hiding this comment.
Do we need three copies? Previous, new, being-updated-from-previous? Or, a maybe a we-are-damaged marker that blocks further usage in case the local actions here fail and the caller consumes the failue and keeps on going.
(hide whitespace to review)
This PR tries to drastically simplify the action scope primitive and remove the sqlite dependency. The motivation behind this was to support nested scopes, but that's not really why this had to happen, it was moreso discovered along the way.
Reviewing the test changes as a diff makes sense, but it might make sense to just review the new
action_scope.pyfile manually since the diff is really a conceptual one rather than a line-by-line one.The idea now is, instead of using serialization into a sqlite DB as a protection against unexpected edits, we just pay more attention to the references we're handing out. The process basically goes as follows when you want to "check out" the mutable object that is held at the top level:
There's also an additional change that was made along the way which is somewhat unrelated but is actually a feature improvement on action scopes. They now require a lock acquisition function (like
DBWrapper.writer) when being constructed and will acquire that lock when opening the scope and release it when closing the scope. This should enforce consistent usage of the idea behindWalletStateManager.lock(to prevent concurrent actions) and eliminate any need for it.