Skip to content

wasm: runtime env flag to allow permissionless store/instantiate (THOR_WASM_PERMISSION_MODE)#12

Open
tiljrd wants to merge 8 commits intodevelopfrom
devin/1758652995-wasm-permission-flag
Open

wasm: runtime env flag to allow permissionless store/instantiate (THOR_WASM_PERMISSION_MODE)#12
tiljrd wants to merge 8 commits intodevelopfrom
devin/1758652995-wasm-permission-flag

Conversation

@tiljrd
Copy link
Collaborator

@tiljrd tiljrd commented Sep 23, 2025

Add bloctopus permissionless WASM manager with runtime env selection

Summary

Introduces a new permissionless WASM manager under x/bloctopus that bypasses all governance and permission checks for WASM operations (store, instantiate, execute, migrate, admin). The manager is selected at runtime via THOR_WASM_MANAGER=bloctopus environment variable, with the existing governance-controlled manager remaining as the default.

Key changes:

  • New x/bloctopus/wasm_manager.go: Complete WasmManager implementation using wasmkeeper.NewDefaultPermissionKeeper (no restrictions)
  • Modified x/thorchain/managers.go: Runtime env-based manager selection in GetWasmManager()
  • Default behavior unchanged: Existing governance controls remain active unless env var explicitly set

Review & Testing Checklist for Human

⚠️ HIGH RISK - Security-sensitive changes

  • Security review: Verify permissionless manager is only intended for development/testing environments, never production
  • Interface compliance: Confirm WasmMgrPermissionless correctly implements all WasmManager interface methods with expected signatures and error handling
  • Runtime switching: Test that THOR_WASM_MANAGER=bloctopus properly switches managers and that any other value (or unset) uses default governance-controlled manager
  • End-to-end testing: Deploy contracts using both managers to verify permissionless path works and default path still enforces governance
  • Production safeguards: Ensure deployment configurations/documentation clearly indicate when this env var should/shouldn't be used

Notes

  • This enables development workflows that don't require MIMIR governance transactions or hardcoded address allowlists
  • The permissionless manager uses cosmwasm's default permission keeper which allows all operations
  • Tested successfully with rujira-bow contract deployment, instantiation, funding, and execution
  • Session requested by @tiljrd - Devin run

@devin-ai-integration
Copy link

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

@qodo-code-review
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns

Unsafe feature toggle:
Introducing an env-based bypass for governance permissions can be risky if enabled unintentionally in production environments. Consider:

  • Restricting accepted values (e.g., explicit "permissionless") with normalization and rejecting unknown values.
  • Logging a clear warning when permissionless mode is active.
  • Optionally gating behind build tag or node config instead of process env, or requiring additional confirmation flags.
⚡ Recommended focus areas for review

Config Handling

Environment variable is read directly via os.Getenv at call sites; consider caching/normalizing mode once at init to avoid per-call overhead and accidental case/whitespace issues. Also ensure deterministic behavior in tests and long-running processes if env changes mid-run.

func (m WasmMgrVCUR) permissionedKeeper() *wasmkeeper.PermissionedKeeper {
	if mode := os.Getenv("THOR_WASM_PERMISSION_MODE"); mode == "permissionless" {
		return wasmkeeper.NewDefaultPermissionKeeper(m.wasmKeeper)
	}
	return wasmkeeper.NewGovPermissionKeeper(m.wasmKeeper)
}

func (m WasmMgrVCUR) checkCanStore(ctx cosmos.Context, actor cosmos.AccAddress) error {
	if mode := os.Getenv("THOR_WASM_PERMISSION_MODE"); mode == "permissionless" {
		return nil
	}
	err := m.checkActor(ctx, actor)
	if err != nil {
		return err
	}

	v, err := m.keeper.GetMimir(ctx, constants.MimirKeyWasmPermissionless)
	if err != nil {
		return err
	}
	if v > 0 && ctx.BlockHeight() > v {
		return nil
	}

	return m.permissions.CanStore(actor)
}

func (m WasmMgrVCUR) checkCanInstantiate(ctx cosmos.Context, actor cosmos.AccAddress) error {
	if mode := os.Getenv("THOR_WASM_PERMISSION_MODE"); mode == "permissionless" {
		return nil
	}
	err := m.checkActor(ctx, actor)
Bypass Scope

Skipping permission checks returns early before actor validation; verify this is intentional and safe (e.g., no additional invariants in checkActor). Confirm that other guardrails (halt, checksum, chain params) still apply when permissionless.

func (m WasmMgrVCUR) checkCanStore(ctx cosmos.Context, actor cosmos.AccAddress) error {
	if mode := os.Getenv("THOR_WASM_PERMISSION_MODE"); mode == "permissionless" {
		return nil
	}
	err := m.checkActor(ctx, actor)
	if err != nil {
		return err
	}

	v, err := m.keeper.GetMimir(ctx, constants.MimirKeyWasmPermissionless)
	if err != nil {
		return err
	}
	if v > 0 && ctx.BlockHeight() > v {
		return nil
	}

	return m.permissions.CanStore(actor)
}

func (m WasmMgrVCUR) checkCanInstantiate(ctx cosmos.Context, actor cosmos.AccAddress) error {
	if mode := os.Getenv("THOR_WASM_PERMISSION_MODE"); mode == "permissionless" {
		return nil
	}
	err := m.checkActor(ctx, actor)
	if err != nil {
		return err
	}
Observability

No logging when entering permissionless mode; consider emitting a warning/info at startup or when creating the permissioned keeper to make the mode explicit in node logs.

func (m WasmMgrVCUR) permissionedKeeper() *wasmkeeper.PermissionedKeeper {
	if mode := os.Getenv("THOR_WASM_PERMISSION_MODE"); mode == "permissionless" {
		return wasmkeeper.NewDefaultPermissionKeeper(m.wasmKeeper)
	}
	return wasmkeeper.NewGovPermissionKeeper(m.wasmKeeper)
}

@qodo-code-review
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Avoid repeated environment variable access

To improve maintainability and avoid repeated system calls, read the
THOR_WASM_PERMISSION_MODE environment variable once and reuse the value instead
of calling os.Getenv in multiple functions.

x/thorchain/manager_wasm_current.go [341-386]

 func (m WasmMgrVCUR) permissionedKeeper() *wasmkeeper.PermissionedKeeper {
-	if mode := os.Getenv("THOR_WASM_PERMISSION_MODE"); mode == "permissionless" {
+	// This function is not shown as it would require modifying the WasmMgrVCUR struct
+	// and its constructor, which are outside the changed lines in the diff.
+	// The improved logic would be:
+	// if m.permissionlessMode {
+	// 	 return wasmkeeper.NewDefaultPermissionKeeper(m.wasmKeeper)
+	// }
+	// return wasmkeeper.NewGovPermissionKeeper(m.wasmKeeper)
+
+	// The following is a minimal change to avoid modifying the struct:
+	if isPermissionlessWasmEnabled() {
 		return wasmkeeper.NewDefaultPermissionKeeper(m.wasmKeeper)
 	}
 	return wasmkeeper.NewGovPermissionKeeper(m.wasmKeeper)
 }
 
 func (m WasmMgrVCUR) checkCanStore(ctx cosmos.Context, actor cosmos.AccAddress) error {
-	if mode := os.Getenv("THOR_WASM_PERMISSION_MODE"); mode == "permissionless" {
+	if isPermissionlessWasmEnabled() {
 		return nil
 	}
 ...
 }
 
 func (m WasmMgrVCUR) checkCanInstantiate(ctx cosmos.Context, actor cosmos.AccAddress) error {
-	if mode := os.Getenv("THOR_WASM_PERMISSION_MODE"); mode == "permissionless" {
+	if isPermissionlessWasmEnabled() {
 		return nil
 	}
 ...
 }
 
+// Add this helper function at the package level
+func isPermissionlessWasmEnabled() bool {
+	return os.Getenv("THOR_WASM_PERMISSION_MODE") == "permissionless"
+}
+

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies repeated os.Getenv calls and proposes centralizing this logic, which improves code maintainability and readability, although the performance impact is negligible.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant