-
Notifications
You must be signed in to change notification settings - Fork 75
Add lang property to ManualNode and generalize input validation for each language; add BalanceNode #411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
chrisli30
commented
Oct 6, 2025
- Remove fallback values for public RPC endpoints and make program fail early when config is missing
- fix: modernize contractWrite processing and remove workflowContext backward compatibility (fix: modernize contractWrite processing and remove workflowContext backward compatibility #405)
- Migrate from workflowContext to settings for simulations (Migrate from workflowContext to settings for simulations #406)
- Remove unused method in core/taskengine/run_node_immediately.go
- Update core/testutil/utils.go
- Update core/taskengine/run_node_immediately.go
- fix: address Copilot PR fix: modernize contractWrite processing and remove workflowContext backward compatibility #407 review comments
- Added BALANCE_NODE_PRD.md
- feat: Add comprehensive input validation with size limits and JSON validation (feat: Add comprehensive input validation with size limits and JSON validation #408)
- feat: add BalanceNode (feat: add BalanceNode #410)
- fix: update the token balance response in vm_runner_balance.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a new BalanceNode for retrieving wallet token balances, introduces a new (and renumbered) Lang enum plus centralized input validation with size / format enforcement, and refactors contract write execution to require authenticated user/settings instead of legacy workflowContext. Also adds extensive tests and documentation for validation and new node behavior.
- Introduces BalanceNode protobuf messages, execution logic, and extraction support
- Renumbers / renames Lang enum values and enforces explicit language for manual trigger & code validation
- Centralizes input size & format validation; adds structured errors and many new test cases
Reviewed Changes
Copilot reviewed 47 out of 49 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| protobuf/avs.proto | Adds BalanceNode; rewrites Lang enum (breaking), adds balance outputs and configs |
| core/taskengine/vm_runner_balance.go | Implements BalanceNode execution (Moralis integration) |
| core/taskengine/validation_constants.go | Centralized size & language validation helpers |
| core/taskengine/run_node_immediately.go | Adds user auth & settings handling; structured manual trigger validation; size checks |
| core/taskengine/node_types.go | Registers BalanceNode and propagates manual trigger lang |
| core/taskengine/vm.go | Integrates BalanceNode in execution paths; removes chain name helper |
| core/taskengine/vm_runner_* (multiple) | Adds language validation calls for JS / GraphQL / Filter / Branch / CustomCode |
| core/testutil/utils.go | Changes test helpers to panic instead of fallback defaults |
| core/taskengine/vm_runner_contract_write.go | Tightens aa_sender / settings validation and error semantics |
| core/taskengine/*_test.go (many) | Updates tests for new lang enum, settings, size limits, structured errors |
| docs/VALIDATION_IMPLEMENTATION.md | New validation implementation guide |
| docs/INPUT_VALIDATION_AUDIT.md | New audit report documenting validation coverage |
| BALANCE_NODE_PRD.md | Product requirements for new BalanceNode |
| docs/Operator.md | Updates example RPC endpoints |
Comments suppressed due to low confidence (1)
core/testutil/utils.go:1
- [nitpick] Replacing the previous graceful fallback with panics makes tests brittle and harder to run in varied CI environments (e.g., when config is intentionally absent). Consider returning a default stub URL, skipping tests, or returning an error so callers can decide, instead of panicking inside utility getters.
package testutil
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…ch (#412) * fix: refactor the execution output logic using object-oriented approach * added helper functin for type conversion: assignOutputData * fix: metadata field not available after refactor * Remove the deprecated old code * fix: added two-phase balance retrival for BalanceNode * Added integration tests for the BalanceNode * fix: test of TestBalanceNode_MissingAPIKey --------- Co-authored-by: Chris Li <[email protected]>
… token addresses
* fix: add BalanceNode smart defaults and template variable support for token addresses
- Add intelligent address extraction from token objects (extracts id/address fields from {id, symbol} objects)
- Implement 3-phase token fetching strategy:
* Phase 1: Fetch all tokens to get native token (ETH, BNB, etc.)
* Phase 2: Fetch specific missing requested tokens
* Phase 3: Synthesize zero-balance entries for tokens not returned by Moralis
- Add smart default: auto-enable includeZeroBalances when tokenAddresses is provided
- Ensure explicitly requested tokens are always returned, even with zero balance
- Add comprehensive test coverage for template variables and token synthesis
- Handle Moralis API limitation where tokens wallet never held are not returned
This fixes the issue where users requesting specific token addresses would not receive
all requested tokens if the wallet had never interacted with some of them.
* Added .find() function test case to Branch
* Add core/taskengine/vm_runner_contract_write_uniswap_test.go
* Address copilot comments and add better documentation
---------
Co-authored-by: Chris Li <[email protected]>
…client-side sanitization
…em (#416) * fix: add is_simulated parameter to runNodeImmediately for real UserOp execution - Add is_simulated boolean field to RunNodeWithInputsReq protobuf message - Update RunNodeImmediatelyRPC to read is_simulated from request and pass to RunNodeImmediately - Modify RunNodeImmediately to accept variadic useSimulation parameter (defaults to true) - Enable real on-chain UserOp execution when is_simulated=false for testing contract interactions - Update smart wallet validation to allow addresses derivable from owner+salt (0-4) even if not in DB * fix: resolve bundler UserOp submission issues and refactor duplicate code Critical fixes for ERC-4337 UserOp handling: 1. Signature Format (EIP-191): - Ensure all UserOp signing uses signer.SignMessage for EIP-191 prefixed signatures - SimpleAccount contract expects EIP-191 format via toEthSignedMessageHash() - Consistent signature format in BuildUserOpWithPaymaster and sendUserOpCore 2. Nonce Management: - BuildUserOpWithPaymaster now explicitly fetches and sets nonce before signing - Add nonceOverride parameter to BuildUserOpWithPaymaster for sequential UserOps - Prevent nonce refresh in SendUserOp when already set to avoid invalidating signatures - Fix nonce collision issues for sequential transactions 3. Paymaster Signature Preservation: - Skip gas re-estimation when PaymasterAndData is present - Prevents invalidation of paymaster signatures after initial signing - Fixes paymaster signature validation failures 4. Gas Limits for Deployment: - Define gas limit constants (DEFAULT_*, DEPLOYMENT_VERIFICATION_GAS_LIMIT) - Increase verificationGasLimit to 1.6M for UUPS proxy deployment - Covers factory execution, proxy deployment, initialization, and validation 5. Code Refactoring: - Extract sendUserOpCore to eliminate ~350 lines of duplicated code - Reduce builder.go from ~900 to 718 lines (20% reduction) - Single source of truth for retry logic, nonce management, gas estimation - Both SendUserOp and SendUserOpWithWsClient now use shared core logic 6. Paymaster Integration: - Fix vm_runner_contract_write.go to use paymasterReq instead of nil - Enable paymaster sponsorship for contract writes when configured 7. Documentation: - Add bundler configuration guide for testing (--min_stake 0) - Document resolution for 'insufficient stake' errors with AAConfig - Explain ERC-4337 staking requirements and testing workarounds * Ignore the file/folder that is constantly added by Cursor * fix: correct PaymasterAndData uint48 timestamp encoding and GetHash parameter order - Fixed timestamp packing from 32-byte ABI encoding to proper 6-byte big-endian uint48 format - Fixed PaymasterAndData field order: validAfter (6 bytes) then validUntil (6 bytes) - Fixed GetHash call to use (userOp, validUntil, validAfter) matching contract ABI - Added extensive debug logging for paymaster signature validation - PaymasterAndData now correctly: address(20) + validAfter(6) + validUntil(6) + signature(65) = 97 bytes - Still debugging AA33 paymaster validation error * fix: use ABI-encoded timestamps in PaymasterAndData and add paymaster nonce tracking - Fixed PaymasterAndData encoding to use abi.encode(validUntil, validAfter) with 32-byte padding per Solidity contract - Changed order to validUntil FIRST, validAfter SECOND to match parsePaymasterAndData abi.decode - Added paymaster nonce fetching and logging (contract maintains per-sender nonce) - PaymasterAndData now correctly: address(20) + abi.encode(uint48, uint48)(64) + signature(65) = 149 bytes - First UserOp (approval) now successfully accepted by bundler! - Updated bundler URL to use SSH tunnel (localhost:4437) for testing - TODO: Fix paymaster nonce collision for sequential UserOps * fix: use EIP-55 checksummed EntryPoint address in bundler RPC calls - Fixed both sendUserOperationHTTP and sendUserOperationRPC to use checksummed EntryPoint address - Bundler was rejecting lowercase EntryPoint addresses with 'invalid UserOperation struct/fields' - Hardcoded the EIP-55 checksummed v0.6 EntryPoint: 0x5FF137D4b0FDCD49DcA30c7CF57E578a026d2789 - Both HTTP and RPC fallback now use the same checksummed format - Still debugging remaining 'invalid UserOperation struct/fields' error * Updated config yaml for aggregator and operator * Add make aggregator-sepolia command and remove the old make dev-agg * Minor updates to the config/aggregator.example.yaml * fix: implement gas estimation before paymaster signature and manual bundle trigger - Add gas estimation before generating paymaster signatures to prevent silent rejections - Implement manual bundle trigger (debug_bundler_sendBundleNow) for immediate bundling - Update shouldUsePaymaster() to check ETH balance with 20% buffer - Add SendBundleNow() method to BundlerClient - Add 5-second wait between sequential contract writes in Base test - Fix BuildUserOpWithPaymaster() test calls to include new gas override parameters - Create comprehensive documentation in docs/Account-Abstraction-Improvements.md These changes fix the root cause of UserOps being rejected by bundlers due to incorrect gas limits baked into paymaster signatures. Gas is now estimated accurately before signing, and manual bundling ensures immediate transaction confirmation (2-5 seconds instead of minutes). * Added waiting for contract write in simulateTask * fix: not treat nonce 0 of a smart wallet as invalid case * Set sepolia yaml config as the default for tests * fix: replacing the hardcoded EntryPointV06Address with the actual entrypoint parameter * fix: make sure the protobuf field means true by default while it is unset * Updated default yaml config of tests * fix: Debug bundler AA33 revert issue - add paymaster nonce tracking and improve logging * Minor fix of removing the unused GetTestPrivateKey from test utils * Added docs of AA33 reverted issue fix * minor fix of removing a deprecated lien in config/aggregator.example.yaml * fix: migrate all tests from TEST_PRIVATE_KEY to OWNER_EOA - Replace TEST_PRIVATE_KEY with OWNER_EOA in all test files - Update GitHub Actions to use OWNER_EOA variable instead of secret - Remove TEST_PRIVATE_KEY from testutil and CI scripts - Add new userops_withdraw_test.go with OWNER_EOA and RECIPIENT_ADDRESS support - Update RunNodeImmediately to accept shouldUsePaymasterOverride parameter - Add shouldUsePaymasterOverride field to VM struct - Remove deprecated withdrawal integration tests - Update documentation for controller-based signing pattern This improves security by using controller delegation pattern where: - OWNER_EOA: Used for smart wallet address derivation (salt:0) - Controller private key: Signs UserOps on behalf of the smart wallet - No need to expose owner's private key in tests or CI * fix: add shouldUsePaymasterOverride support to ETHTransfer and ContractWrite nodes - ETHTransfer: Add shouldUsePaymasterOverride check (priority 0) - ContractWrite: Add shouldUsePaymasterOverride check (priority 0) - Both nodes now respect explicit paymaster override from RunNodeImmediately - Improve gas cost estimation to use real-time gas prices (not hardcoded 20 gwei) - Add detailed logging for paymaster decision-making - Update ContractWrite to check EntryPoint deposit for self-funding eligibility This enables explicit control over paymaster usage in tests via: engine.RunNodeImmediately(nodeType, config, vars, user, false, &usePaymaster) Critical for testing paymaster sponsorship vs self-funded UserOps. * fix: correct paymaster data packing to use ABI encoding (AA33 solution) Critical AA33 reverted fix - paymaster timestamp packing: BEFORE (12-byte compact packing): validUntil + validAfter = 12 bytes compact Total paymasterAndData: 97 bytes Contract tried to decode at offset 84 → read into signature bytes → FAIL AFTER (64-byte ABI encoding): abi.Arguments.Pack(uint48, uint48) = 64 bytes (32+32 padded) Total paymasterAndData: 149 bytes Contract decodes cleanly at offset 20-84 → SUCCESS Additional fixes: - Refactor SendUserOp/SendUserOpWithWsClient into shared sendUserOpShared() - Fix gas estimation to use EXACT UserOp structure (including paymaster) - Increase DEFAULT_VERIFICATION_GAS_LIMIT from 150k to 1M (conservative) - Add UseLocalGasEstimation flag to skip bundler estimation if needed - Improve logging for gas estimation and paymaster signing This resolves the persistent AA33 errors by ensuring client-side packing matches the VerifyingPaymasterFixed.sol contract's expectations. * chore: minor cleanups and documentation updates - Remove legacy TEST_PRIVATE_KEY references in comments - Update gas estimation logging to remove emoji (per user preference) - Improve EIP-1559 gas price calculation logging - Clean up testutil comments for OWNER_EOA migration No functional changes - documentation and logging improvements only. * fix: implement robust nonce polling for AA25 error resolution - Replace fixed 1-second delay with adaptive polling mechanism - Poll for fresh nonce with 15-second timeout and 500ms intervals - Only waits as long as needed for RPC node synchronization - Matches existing polling patterns used throughout codebase - Prevents AA25 invalid account nonce errors in back-to-back test runs - Includes progress logging and timeout fallback for robustness * fix: add hardcoded gas fallback for self-funded UserOps and diagnostic checks - Add hardcoded gas fallback to self-funded UserOps when gas estimation fails with STF error - Self-funded UserOps now fall back to hardcoded gas limits (same as paymaster UserOps) - Add 4 diagnostic checks to verify allowance spender is correct before swap execution - Both RunNodeImmediately and deployed workflow execution now handle STF gas estimation failures - Resolves STF error in Base mainnet Uniswap swaps by using hardcoded gas limits as fallback * fix: consolidate Uniswap swap test files for multi-chain support - Consolidate run_node_uniswap_swap_test.go and run_node_uniswap_swap_base_test.go into single file - Add environment-based chain detection via TEST_CHAIN env var (sepolia/base) - Add Sepolia constants and configuration support - Create getChainConfig() function to select chain-specific addresses and settings - Update all test functions to use setup.chain.* instead of hardcoded BASE_* constants - Rename SEPOLIA_CHAIN_ID to TENDERLY_SEPOLIA_CHAIN_ID in tenderly_client_test.go to avoid conflicts - All 4 test variants (Base, SelfFunded, WithQuoter, CheckSpender) now work for both chains - Simplifies maintenance - single source of truth for Uniswap swap testing logic * fix: resolve failing unit tests after paymaster timestamp changes - Fix TestContractWrite_WithSettingsAndUserAuth nil pointer dereference by creating eth client before calling GetSenderAddress - Update TestGetHash expected hash to match new ABI-encoded timestamp format (64 bytes instead of compact 12 bytes) - Add missing ethclient import to run_node_immediately_settings_test.go * fix: change IsSimulated default to true for safety - Change GetIsSimulated() default return value from false to true when IsSimulated is nil - This prevents unintended real executions when simulation mode was intended - Aligns with documented safety default behavior where unset is_simulated should default to simulation mode - Critical safety fix to prevent accidental real transactions * Revert "fix: change IsSimulated default to true for safety" This reverts commit 0bc3016. * fix: update failing tests to use OWNER_EOA instead of hardcoded addresses - Fix TestUniswapApprovalAmountSimulation to use OWNER_EOA environment variable and derive smart wallet address - Fix TestEndToEndValuePropagation to use derived smart wallet address instead of hardcoded runner - Update TestUniswapApprovalAmountSimulation_PrintAddresses to use OWNER_EOA approach - Add proper wallet registration in database for both tests - Resolves CI test failures caused by address mismatches after OWNER_EOA migration * fix: rename integration tests for clarity and enable in CI - Rename TestUniswapApprovalAmountSimulation -> TestExecuteTask_UniswapApprovalAndSwap_DifferentApprovalAmounts_Sepolia - Rename TestEndToEndValuePropagation -> TestContractWrite_WETHDeposit_WithETHValue_Sepolia - Remove CI skip logic - GitHub Actions has Tenderly access and funded test wallets - Test names now clearly indicate: what they test, that they execute real tasks, and network (Sepolia) - Refactor WETH deposit test to use OWNER_EOA with salt:0 derivation - Fix aa.GetSenderAddress call signature (3 params instead of 5) Both tests now run in CI and local environments with same OWNER_EOA-derived smart wallet --------- Co-authored-by: Chris Li <[email protected]>