Conversation
5d7e920 to
c018c90
Compare
b3c4095 to
616fd6f
Compare
616fd6f to
07fbe31
Compare
| mutable_props.extend(search_writeable_properties(self, 'self')) | ||
| mutable_props.extend(search_writeable_properties(ctx, 'ctx')) | ||
| custom_import = EXEC_BUILTINS['__import__'] | ||
| builtins = get_exec_builtins(runner=None) # TODO: This does not cover the lazy imports |
There was a problem hiding this comment.
TODO: Fix this test after the design is approved.
| from tests.nanocontracts.blueprints.unittest import BlueprintTestCase | ||
|
|
||
|
|
||
| @pytest.mark.skip # TODO: Fix this test |
There was a problem hiding this comment.
TODO: Fix this test after the design is approved.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/nano/contract-accessor #1384 +/- ##
===============================================================
+ Coverage 85.73% 85.81% +0.08%
===============================================================
Files 430 431 +1
Lines 32561 32594 +33
Branches 5081 5083 +2
===============================================================
+ Hits 27915 27970 +55
+ Misses 3621 3605 -16
+ Partials 1025 1019 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c018c90 to
c02679a
Compare
dd10f72 to
5bed5a6
Compare
# Conflicts: # tests/nanocontracts/blueprints/unittest.py
c02679a to
ba2ee51
Compare
5bed5a6 to
52a9956
Compare
jansegre
left a comment
There was a problem hiding this comment.
I like the interface and the implementation seems correct, even with important details like OnChainBlueprint.get_blueprint_class.
However, I think there are still a few problems:
get_contractwon't work for built-in Blueprints, it's not a deal-breaker IMO, but I think we should just drop support for built-in Blueprints before doing this, which would be a separate discussion- I believe there might still be problems stemming from the fact that
OnChainBlueprint.get_blueprint_classreturns different objects for different runners or forNone, even though it is the "correct" behavior, I think these problems would be solved, or rather, the structure to solve them would have to be in place first, when we have Blueprints' execution isolated in a separate process, so we might want to wait for that first. But I'll explain here.
Currently the deserializer classes of some types (most notably NamedTuple classes and any container combination with one, there might be other types, I'd have to check), have a reference to the class that is being deserialized, in order to be able to instantiate objects of that class. This means the deserializer depends on the "implementation class" that is defined in the Blueprint module, which means that different "loads" of that module will produce different "class objects", which means, that an object that was deserialized by some parsing OnChainBlueprint.get_blueprint_class(None).method will different than one deserialized by parsing OnChainBlueprint.get_blueprint_class(runner).method, this difference would have an effect on isinstance(some_tuple, MyNamedTuple) but also in other cases.
The Blueprint code that causes this situation might not be as niche as it sounds, we allow and support custom named-tuples classes, and they are being used, and we do support use of isinstance.
It's possible that the current code never mixes any objects from different types of "blueprint loads" (with different runners, or with runners and with None). But it's at least complex to verify that.
My point is that making the "loading of a blueprint's module" depend on a "runner object" is not good when we use the blueprint class (and this the objects created by the module), in the same runtime as the node's code. If we already had the execution isolated in a separate process, this would be a lot more trivial to trust. So, I'm currently leaning more towards that, even though I much prefer the DX of using get_contract instead of self.sysctl.get_contract.
2960f81 to
6166f29
Compare
Motivation
Implement support for lazy imports in blueprints, that is, imports that depend on a runtime Runner. This will enable us to remove syscalls and provide standalone functions instead, improving developer experience.
For now, the only the
get_contractsyscall is converted to the new API, but in other PRs we can also make other improvements such as changing thectxargument to aget_context()function, for example. All syscalls could be moved to this new system, allowing us to remove the unintuitivesyscallattribute from blueprints.Review Notes
I recommend reviewing files in the following order:
lazy_import.pyto understand how lazy imports work.custom_builtins.pyto see how the Runner is injected in lazy imports during contract execution. The diff is very large because theEXEC_BUILTINSconstant is moved to a function, use the "Hide whitespace" feature of GitHub to make it way smaller.contract_accessor.pyto see the first usage of a lazy import, theget_contractfunction.execstep during class load, not in thecallstep.Acceptance Criteria
__import__function to inject a Runner in lazy imports.EXEC_BUILTINSconstant to a function, because now it depends on a Runner.get_contractfunction to replace the syscall.test_types_across_contracts.pytest to use the new API, as a demo.Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets merged