GeezerVison

examples/pvm_runtime_chain_demo/README.md (3)

1-14: Consider adding a Prerequisites section.

The README jumps directly into file descriptions without mentioning what needs to be installed or configured. Adding a brief Prerequisites section before "Files" would help users prepare their environment.

For example:

• Rust toolchain version
• Python version requirements
• libffi (especially for macOS users)
• Any other dependencies

---

23-28: Consider consolidating macOS libffi instructions.

The DYLD_LIBRARY_PATH setup for macOS appears three times (lines 23-28, 39-40, 102-106). While this repetition ensures users don't miss it, you could improve organization by:

1. Creating a "Troubleshooting" or "Platform-Specific Notes" section, or
2. Moving this to a Prerequisites section with a note like "macOS users: Export this before running any commands"

This would reduce duplication while maintaining clarity.

Also applies to: 39-40, 102-106

---

109-113: Clarify scope of "Contract Behavior" section.

This section describes only the basic contract.py behavior, not the other demo contracts (escrow_marketplace_demo.py, batch_payroll_demo.py, staking_rewards_demo.py) listed in the Files section. Consider either:

1. Renaming to "Basic Contract Behavior" to clarify the scope, or
2. Expanding to briefly describe what each demo contract demonstrates

This would help users understand what to expect when running the different demos.

examples/ast_visualize/README.md (1)

10-12: Consider adding language hints to fenced code blocks.

Multiple code blocks lack language specification, which affects syntax highlighting and tooling. For shell commands, use bash or sh; for the command examples, use bash.

Example fix for a few blocks

 Tree view (default):

-```
+```bash
 ./target/release/rustpython examples/ast_visualize/ast_view.py --file examples/ast_visualize/sample.py

...

macOS (Homebrew):

- +bash
brew install graphviz

Also applies to: 16-18, 22-24, 30-32, 36-38, 42-45, 49-51, 55-57, 61-63, 67-69, 73-75, 81-83, 87-89, 93-95

examples/ast_visualize/ast_view.py (1)

150-151: Consider escaping additional DOT special characters.

The current escaping handles backslashes and double quotes, but DOT labels may also need escaping for newlines and potentially angle brackets if they appear in node summaries (e.g., from string literals containing < or >).

More comprehensive escaping

 def escape_dot_label(text: str) -> str:
-    return text.replace("\\", "\\\\").replace('"', "\\\"")
+    return (
+        text.replace("\\", "\\\\")
+        .replace('"', '\\"')
+        .replace("\n", "\\n")
+        .replace("<", "\\<")
+        .replace(">", "\\>")
+    )

examples/pvm_runtime_chain_demo/run_checkpoint_demo.sh (1)

24-30: Backup filenames can collide within the same second.

If the script runs twice quickly, date +%s can produce the same suffix and mv will fail. Consider adding PID or using mktemp for uniqueness.

💡 Example tweak

-ts=$(date +%s)
+ts="$(date +%s)-$$"

examples/breakpoint_resume_demo/state_store.py (1)

19-22: Consider atomic writes for state persistence.

Direct writes can leave partial/corrupt JSON if interrupted.

🛡️ Suggested refactor (atomic write)

-import json
+import json
+import os
+import tempfile
 from pathlib import Path
 from typing import Any
@@
 def save_state(state: dict[str, Any]) -> None:
     # Write checkpoint state, ensuring the directory exists.
     DATA_DIR.mkdir(parents=True, exist_ok=True)
-    STATE_FILE.write_text(json.dumps(state, sort_keys=True, indent=2))
+    payload = json.dumps(state, sort_keys=True, indent=2)
+    with tempfile.NamedTemporaryFile(
+        "w", delete=False, dir=DATA_DIR, encoding="utf-8"
+    ) as tmp:
+        tmp.write(payload)
+        tmp_path = tmp.name
+    os.replace(tmp_path, STATE_FILE)

examples/pvm_actor_transfer_demo/main.rs (1)

129-137: Optional: de-duplicate encode_hex.

This helper matches the one in examples/pvm_runtime_chain_demo/main.rs (lines 149-157); consider a shared utility to reduce drift.

examples/pvm_actor_transfer_demo/contract.py (2)

41-44: bool values pass the int check due to Python's type hierarchy.

In Python, bool is a subclass of int, so isinstance(True, int) returns True. If boolean values should be rejected, add an explicit check.

♻️ Proposed fix to exclude booleans

 def _require_int(value, name):
-    if not isinstance(value, int):
+    if not isinstance(value, int) or isinstance(value, bool):
         raise ValueError(f"{name} must be int")
     return value

---

140-151: Consider adding authorization for mint operations.

The _handle_mint function allows anyone to mint tokens without authorization checks. While this may be intentional for a demo, in production smart contracts, minting should typically be restricted to authorized accounts.

examples/breakpoint_resume_demo/comprehensive_demo.py (1)

77-77: Remove unnecessary f-string prefix.

Multiple print statements use f-strings without placeholders. Per PEP 8 and linting guidelines, use regular strings when there are no interpolated values.

♻️ Proposed fix for f-strings without placeholders

-print(f"\n[Test 2] For Loop with List Iteration")
+print("\n[Test 2] For Loop with List Iteration")

Similar changes apply to lines: 88, 96, 107, 115, 127, 135, 150, 161, 168, 171, 181, 191, 203, 211, 224, 235, 241, 244, 252, 267, 270, 278, 289, 292, 300, 313, 316, 324, 337, 340, 357, 363, 365, 370.

examples/breakpoint_resume_demo/test_checkpoint.py (2)

6-6: Remove unused sys import.

The sys module is imported but never used. The script uses raise SystemExit(main()) which doesn't require sys.

♻️ Proposed fix

-import sys

---

10-17: Binary name mismatch in help text.

The resolve_bin_path function looks for pvm binary, but the --bin argument help text says "rustpython executable". Consider aligning the documentation with the actual binary name.

♻️ Proposed fix

     parser.add_argument(
         "--bin",
-        help="Path to the rustpython executable, defaults to target/release/rustpython",
+        help="Path to the pvm executable, defaults to target/release/pvm",
     )

Also applies to: 29-33

examples/pvm_runtime_chain_demo/determinism_check.py (2)

55-58: Consider using a more explicit deduplication approach.

Using dict.fromkeys(paths) for deduplication works but is somewhat indirect. A more readable approach would use a set or explicit loop for clarity.

♻️ Suggested refactor

     if paths:
-        env["DYLD_FALLBACK_LIBRARY_PATH"] = os.pathsep.join(
-            dict.fromkeys(paths)
-        )
+        # Deduplicate while preserving order
+        seen = set()
+        unique_paths = [p for p in paths if not (p in seen or seen.add(p))]
+        env["DYLD_FALLBACK_LIBRARY_PATH"] = os.pathsep.join(unique_paths)

---

166-171: Narrow the exception catch for better error diagnostics.

Catching bare Exception masks the specific failure mode. Consider catching the expected exceptions (ValueError from fromhex, UnicodeDecodeError from decode, json.JSONDecodeError from loads).

♻️ Suggested refactor

         if args.decode:
             try:
                 decoded = bytes.fromhex(out_hex).decode("utf-8")
                 parsed = json.loads(decoded)
                 print(json.dumps(parsed, indent=2, sort_keys=True))
-            except Exception as exc:
+            except (ValueError, UnicodeDecodeError, json.JSONDecodeError) as exc:
                 print(f"decode failed: {exc}")

examples/pvm_runtime_chain_demo/determinism_demo.py (1)

39-43: Use a context manager for the file write.
This avoids leaving the file handle open when the write succeeds.

♻️ Suggested tweak

-    open("forbidden.txt", "wb").write(b"x")
+    with open("forbidden.txt", "wb") as f:
+        f.write(b"x")

examples/pvm_alto_call_demo.rs (1)

41-44: Consider adding explicit path validation and configurability for better error messages.

The contract file at examples/pvm_actor_transfer_demo/contract.py currently exists, and the ? operator provides appropriate error handling. However, an explicit existence check would provide a clearer error message if the file is ever missing or renamed:

Optional improvement for fail-fast with clearer error

-    let script_path = "examples/pvm_actor_transfer_demo/contract.py";
-    let code = fs::read(script_path)?;
+    let script_path = PathBuf::from("examples/pvm_actor_transfer_demo/contract.py");
+    if !script_path.exists() {
+        return Err(format!("missing contract script: {}", script_path.display()).into());
+    }
+    let code = fs::read(&script_path)?;

-    let options = default_options().with_source_path(script_path);
+    let options = default_options().with_source_path(script_path.to_string_lossy());

crates/vm/src/stdlib/rustpython_checkpoint.rs (1)

22-34: Consider factoring shared checkpoint enqueue logic.

checkpoint and checkpoint_bytes duplicate the same frame/lasti/request logic; a small helper would keep the error path and locking behavior consistent.

crates/vm/src/vm/compile.rs (1)

54-70: Avoid drift by sharing the safe-path/sys.path logic.

run_script and run_script_resume now duplicate the same directory insertion; consider extracting a small helper to keep behavior aligned.

examples/pvm_runtime_chain_demo/main.rs (1)

22-82: Treat unknown flags as errors to avoid mis-parsing script path.

Right now any unrecognized --flag is accepted as a script path, which can produce confusing errors. Consider rejecting unknown options that start with -.

♻️ Suggested tweak

             _ => {
+                if arg.starts_with('-') {
+                    return Err(usage().into());
+                }
                 if script_path.is_none() {
                     script_path = Some(arg);
                 } else if input.is_none() {
                     input = Some(arg.into_bytes());
                 } else {

examples/pvm_runtime_chain_demo/checkpoint_demo.py (1)

5-6: Handle immediate coroutine completion in run.

If the coroutine returns without awaiting, send(None) raises StopIteration; catching it makes the helper more robust.

♻️ Suggested tweak

 def run(coro):
-    return coro.send(None)
+    try:
+        return coro.send(None)
+    except StopIteration as exc:
+        return exc.value

src/settings.rs (1)

157-163: Consider validating --resume/--continuation pairing.
If one flag is supplied without the other, consider defaulting or emitting a clear error to avoid ambiguous behavior.

examples/pvm_dex_demo/main.rs (2)

19-80: Consider using clap for argument parsing.

The manual argument parsing is functional but verbose. For an example binary, this is acceptable to minimize dependencies. However, if this example grows, consider using the clap crate for more robust and maintainable CLI handling.

---

120-128: Duplicate encode_hex implementation.

This function is identical to the one in crates/pvm-alto/src/lib.rs. Consider either:

1. Exporting encode_hex from pvm_alto and using it here, or
2. Adding a shared utility crate for common helpers.

For an example binary, some duplication is tolerable, but consolidating would improve maintainability.

crates/codegen/src/pvm_fsm.rs (3)

228-263: Consider handling multiple return statements.

The parse_body function (lines 228-263) processes the body sequentially and captures the last return statement's expression. If multiple return statements exist, only the last one is used for the generated FSM, which could lead to unexpected behavior.

Consider either:

1. Validating that only one return statement exists at the end, or
2. Documenting this limitation clearly.

---

396-476: Generated code uses inline imports.

The generated FSM functions include import pvm_sdk inside each function body (lines 409, 430). While this ensures the import is available regardless of module-level imports, it incurs repeated import overhead.

This is acceptable for the code generation approach, but consider documenting that generated functions have this characteristic or exploring module-level import tracking if performance becomes a concern.

---

478-503: Similar contains_await pattern exists in compile.rs.

This contains_await implementation is similar to the one in crates/codegen/src/compile.rs (lines 5764-5780), but operates on Stmt instead of Expr. Both are needed for their respective contexts, but consider extracting a shared visitor utility if more await-detection logic is needed elsewhere.

crates/pvm-runtime/src/module.rs (1)

118-152: Consider using specialized exception types in host_error.

DeterministicValidationError, NonDeterministicError, and OutOfGasError are defined but the host_error function always returns the base HostError type. Consider mapping specific HostError variants to these specialized types for better Python-side exception handling.

♻️ Suggested enhancement

 fn host_error(vm: &VirtualMachine, err: HostError) -> PyBaseExceptionRef {
-    let exc = vm.new_exception(
-        host_error_type(vm),
-        vec![vm.ctx.new_str(err.to_string()).into()],
-    );
+    let exc_type = match &err {
+        HostError::OutOfGas => out_of_gas_error_type(vm),
+        HostError::Forbidden => nondeterministic_error_type(vm),
+        HostError::InvalidInput => deterministic_validation_error_type(vm),
+        _ => host_error_type(vm),
+    };
+    let exc = vm.new_exception(
+        exc_type,
+        vec![vm.ctx.new_str(err.to_string()).into()],
+    );
     let _ = exc
         .as_object()
         .set_attr("code", vm.new_pyobj(err.code()), vm);

crates/vm/src/frame.rs (1)

568-571: SystemExit after checkpoint may confuse error handling.

After successfully saving a checkpoint, raising SystemExit with message "checkpoint exit" could be confusing since it's not an error. The calling code in crates/pvm-runtime/src/lib.rs handles this by checking for checkpoint bytes, but a more explicit signal (like a custom exception type) would be clearer.

crates/pvm-runtime/src/lib.rs (3)

164-190: Modifying determinism options during resume changes behavior mid-execution.

The code modifies the whitelist/blacklist of the determinism options when resume_bytes is present to allow OS-related imports. This mutation of the options struct could lead to unexpected behavior if the options are reused. Consider cloning or documenting this side effect.

---

319-327: Using expect for dict set operations could panic.

While these operations are unlikely to fail, using expect in library code can cause unexpected panics. Consider propagating errors or logging warnings instead.

♻️ Suggested change

     main_module
         .dict()
         .set_item("__annotations__", vm.ctx.new_dict().into(), vm)
-        .expect("Failed to initialize __main__.__annotations__");
+        .ok(); // Ignore errors for optional metadata
     main_module
         .dict()
         .set_item("__file__", vm.ctx.new_str(options.source_path.clone()).into(), vm)
-        .expect("Failed to initialize __main__.__file__");
+        .ok();
     main_module
         .dict()
         .set_item("__cached__", vm.ctx.none(), vm)
-        .expect("Failed to initialize __main__.__cached__");
+        .ok();

---

562-589: Consider using serde_json for JSON serialization if comprehensive JSON compliance is needed.

The manual JSON escaping implementation currently handles basic cases (\, ", \n, \r, \t) but misses escaping control characters that JSON specification requires (e.g., backspace \b, form feed \f, and other ASCII control characters below 0x20). If serde_json is acceptable as a dependency, it would provide more robust and spec-compliant serialization.

crates/vm/src/vm/checkpoint.rs (2)

232-239: Empty else branch in fastlocals restoration loop.

The else branch is empty and serves no purpose. Consider removing it for clarity.

♻️ Suggested fix

         for (idx, varname) in varnames.iter().enumerate() {
             if let Some(value) = locals_dict.get_item_opt(*varname, vm)? {
                 fastlocals[idx] = Some(value);
-            } else {
             }
         }

---

433-436: Remove redundant emptiness check.

The check if !frames.is_empty() on line 434 is redundant since line 423-425 already returns an error if frames is empty.

♻️ Suggested fix

     // Build blocks vec: only innermost frame gets blocks, others get empty vec.
     // Outer frames are waiting for inner frames to return and their block state
     // can be safely reconstructed as empty since they're not in active control flow.
     let mut all_blocks = vec![Vec::new(); frames.len()];
-    if !frames.is_empty() {
-        all_blocks[frames.len() - 1] = innermost_blocks;
-    }
+    all_blocks[frames.len() - 1] = innermost_blocks;