refactor: adopt single-key signing and on-behalf account context#37
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors hlgo’s signing/config model to use a single configured private key, adds an explicit “on behalf of” account context for supported L1 (phantom-agent) flows, and introduces fail-loud validation where on-behalf usage is unsupported. It updates CLI flags, docs, and tests to match the new model.
Changes:
- Replace dual-key config (
agent_key_env/master_key_env) with a singleprivate_key_envand route executor construction through it. - Add
--on-behalf-ofto supported L1 command flows (order/position/agent bracket) and align open-order lookups with the explicit address when provided. - Add explicit validation errors for unsupported
--on-behalf-ofusage (user-signed account actions andorder schedule-cancel), and update docs/tests accordingly.
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| skill/hlgo/references/contracts-and-safety.md | Update signing-path matrix to single-key terminology. |
| skill/hlgo/references/command-reference.md | Update CLI reference for new key env + --on-behalf-of. |
| skill/hlgo/SKILL.md | Update skill guidance to single-key + new failure modes. |
| plans/issues/github-issues-seed.md | Align plan docs with single-key model terminology. |
| plans/hlgo-technical-spec-v0.1.0.md | Update technical spec for user-signed chain ID and on-behalf support notes. |
| pkg/info/address_test.go | Update address resolution tests to use PrivateKeyEnv. |
| pkg/info/address.go | Resolve default address from private_key_env. |
| pkg/exchange/types.go | Update comment language around “account” vs “master wallet”. |
| pkg/exchange/executor.go | Thread OnBehalfOf through L1 flows; reject it for unsupported flows. |
| pkg/config/context_test.go | Update context round-trip test for PrivateKeyEnv. |
| pkg/config/config_test.go | Update config defaults/load tests for private_key_env. |
| pkg/config/config.go | Remove dual-key fields; default to HL_PRIVATE_KEY. |
| e2e/integration_cli_test.go | Update integration tests/config fixtures to single-key env var. |
| e2e/e2e_test.go | Update E2E env setup to HL_PRIVATE_KEY. |
| e2e/agent_simulation_test.go | Update agent simulation to single-key env var. |
| cmd/root_test.go | Update config injection test for private_key_env. |
| cmd/position_test.go | Update tests to validate --on-behalf-of instead of --vault. |
| cmd/position_margin.go | Replace --vault with --on-behalf-of and plumb through executor call. |
| cmd/position_leverage.go | Replace --vault with --on-behalf-of and plumb through executor call. |
| cmd/position.go | Update help text to reflect configured-key signing. |
| cmd/order_test.go | Update invalid address tests and add schedule-cancel on-behalf unsupported test. |
| cmd/order_schedule_cancel.go | Add --on-behalf-of parsing/validation and pass through to executor (which rejects). |
| cmd/order_place.go | Replace --vault with --on-behalf-of; build executor from PrivateKeyEnv. |
| cmd/order_modify.go | Replace --vault with --on-behalf-of; update lookup to use explicit address when provided. |
| cmd/order_market.go | Replace --vault with --on-behalf-of and plumb through. |
| cmd/order_cancel.go | Replace --vault with --on-behalf-of; align cancel-all lookup address behavior. |
| cmd/order_batch.go | Replace --vault with --on-behalf-of and plumb through. |
| cmd/order.go | Update help text to reflect configured-key signing. |
| cmd/info_user.go | Update default address help text to configured private key. |
| cmd/info_test.go | Update test harness config fixture to private_key_env. |
| cmd/config_test.go | Update config init/show/test tests for single-key config schema. |
| cmd/config.go | Update config init/show/test implementation for private_key_env only. |
| cmd/agent_snapshot.go | Update default address help text to configured private key. |
| cmd/agent_pnl.go | Update default address help text to configured private key. |
| cmd/agent_bracket.go | Replace --vault with --on-behalf-of and plumb through. |
| cmd/account_withdraw.go | Route through single executor; accept --on-behalf-of (executor rejects for user-signed). |
| cmd/account_transfer.go | Route through single executor; accept --on-behalf-of (executor rejects for user-signed). |
| cmd/account_test.go | Add coverage for on-behalf unsupported on account actions; update missing-key tests. |
| cmd/account_set_abstraction.go | Route through single executor; accept --on-behalf-of (executor rejects for user-signed). |
| cmd/account_send_asset.go | Route through single executor; accept --on-behalf-of (executor rejects for user-signed). |
| cmd/account_helpers.go | Remove buildMasterExecutor helper in favor of unified executor creation. |
| cmd/account_class_transfer.go | Route through single executor; accept --on-behalf-of (executor rejects for user-signed). |
| cmd/account_approve_agent.go | Route through single executor; accept --on-behalf-of (executor rejects for user-signed). |
| cmd/account.go | Update help text to reflect configured-key user-signed flow. |
| SOUL.md | Update principles to single-key + explicit account context model. |
| README.md | Update quick start env var to HL_PRIVATE_KEY. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| cmd.Flags().String("destination", "", "destination EVM address") | ||
| cmd.Flags().String("amount", "", "USDC amount") | ||
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") |
There was a problem hiding this comment.
This command registers --on-behalf-of with help text implying it’s supported, but user-signed actions are rejected when onBehalfOf is set (see executor validation). Consider updating the flag description to explicitly note it’s unsupported for account commands (or remove the flag to avoid advertising a non-functional option).
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") | |
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of (currently unsupported for account commands; setting this may cause the request to be rejected)") |
| cmd.Flags().String("destination", "", "destination EVM address") | ||
| cmd.Flags().String("token", "", "spot token identifier (e.g. PURR:0x1)") | ||
| cmd.Flags().String("amount", "", "token amount") | ||
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") |
There was a problem hiding this comment.
This command registers --on-behalf-of with help text implying it’s supported, but user-signed actions are rejected when onBehalfOf is set (see executor validation). Consider updating the flag description to explicitly note it’s unsupported for account commands (or remove the flag to avoid advertising a non-functional option).
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") | |
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of (currently unsupported for account commands; reserved for future use)") |
|
|
||
| cmd.Flags().String("user", "", "user address") | ||
| cmd.Flags().String("abstraction", "", "abstraction string") | ||
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") |
There was a problem hiding this comment.
This command registers --on-behalf-of with help text implying it’s supported, but user-signed actions are rejected when onBehalfOf is set (see executor validation). Consider updating the flag description to explicitly note it’s unsupported for account commands (or remove the flag to avoid advertising a non-functional option).
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") | |
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of (unsupported for account commands; must be empty for user-signed actions)") |
| | Command | Purpose | Key Flags | Example | | ||
| |---|---|---|---| | ||
| | `hlgo config init` | Create config file | `--agent-key-env`, `--master-key-env`, `--default-dex`, `--metadata-ttl`, `--force` | `hlgo config init --agent-key-env HL_AGENT_KEY --master-key-env HL_MASTER_KEY` | | ||
| | `hlgo config init` | Create config file | `--private-key-env`, `--default-dex`, `--metadata-ttl`, `--force` | `hlgo config init --private-key-env HL_PRIVATE_KEY` | |
There was a problem hiding this comment.
The hlgo config init row lists --private-key-env twice (both in the key-flags column and in the example). This reads like a copy/paste error and could mislead users; it should list the flag once and the example should pass it once.
| | `order`, `position`, `agent bracket` | Configured key (`private_key_env`) | L1 phantom-agent path | | ||
| | `account` commands | Configured key (`private_key_env`) | User-signed path | |
There was a problem hiding this comment.
This matrix still labels the signer as “Agent wallet” vs “Master wallet”, but both rows now point at the same private_key_env. To avoid implying two separate wallets still exist, consider renaming the Wallet column to something like “Configured key” (and describing the difference as L1 path vs user-signed path).
| // setDefaults registers default values for all config keys. | ||
| func setDefaults(v *viper.Viper) { | ||
| v.SetDefault("agent_key_env", "HL_AGENT_KEY") | ||
| v.SetDefault("master_key_env", "HL_MASTER_KEY") | ||
| v.SetDefault("private_key_env", "HL_PRIVATE_KEY") | ||
| v.SetDefault("default_dex", "") | ||
| v.SetDefault("metadata_ttl", 300) |
There was a problem hiding this comment.
Config loading no longer recognizes agent_key_env / master_key_env. Because private_key_env has a default, an existing config file containing only the old keys will be silently ignored and HL_PRIVATE_KEY will be used instead, which is a breaking change with a confusing failure mode. Consider adding a migration/fallback in Load() (e.g., if private_key_env isn’t set, map agent_key_env/master_key_env to it, and if both old keys are set but differ, return a clear CONFIG_ERROR explaining how to update).
|
|
||
| cmd.Flags().String("timeout", "", "cancellation timeout (Go duration, e.g. 5m, 1h)") | ||
| cmd.Flags().Bool("clear", false, "clear existing schedule") | ||
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") |
There was a problem hiding this comment.
schedule-cancel exposes --on-behalf-of in --help, but the executor always rejects any non-empty value as unsupported. The flag help text currently implies it works; consider updating the description to explicitly say it’s not supported (and why), or omit the flag entirely and surface the structured validation error another way.
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") | |
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of (currently not supported for schedule-cancel; non-empty values will be rejected by the server)") |
| cmd.Flags().String("amount", "", "USDC amount") | ||
| cmd.Flags().Bool("to-perp", false, "transfer from spot to perp") | ||
| cmd.Flags().Bool("to-spot", false, "transfer from perp to spot") | ||
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") |
There was a problem hiding this comment.
This command registers --on-behalf-of with help text implying it’s supported, but user-signed actions are rejected when onBehalfOf is set (see executor validation). Consider updating the flag description to explicitly note it’s unsupported for account commands (or remove the flag to avoid advertising a non-functional option).
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") | |
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of (unsupported for account transfer commands; may be rejected)") |
|
|
||
| cmd.Flags().String("agent", "", "agent wallet address") | ||
| cmd.Flags().String("name", "", "optional agent label") | ||
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") |
There was a problem hiding this comment.
This command registers --on-behalf-of with help text implying it’s supported, but user-signed actions are rejected when onBehalfOf is set (see executor validation). Consider updating the flag description to explicitly note it’s unsupported for account commands (or remove the flag to avoid advertising a non-functional option).
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") | |
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of (unsupported for account commands; reserved for future use)") |
| cmd.Flags().String("amount", "", "transfer amount") | ||
| cmd.Flags().Bool("to-perp", false, "transfer toward perp class") | ||
| cmd.Flags().Bool("to-spot", false, "transfer toward spot class") | ||
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") |
There was a problem hiding this comment.
This command registers --on-behalf-of with help text implying it’s supported, but user-signed actions are rejected when onBehalfOf is set (see executor validation). Consider updating the flag description to explicitly note it’s unsupported for account commands (or remove the flag to avoid advertising a non-functional option).
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of") | |
| cmd.Flags().String("on-behalf-of", "", "account address to act on behalf of (unsupported for account commands; setting this will cause the request to be rejected)") |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| return e.client.PostExchange(ctx, action, nonce, sigToWire(sig), vaultAddr, expiresAfter) | ||
| return e.client.PostExchange(ctx, action, nonce, sigToWire(sig), "", expiresAfter) |
There was a problem hiding this comment.
PlaceBatchOrders ignores its onBehalfOf argument and calls PostExchange with an empty vaultAddress, so order batch --on-behalf-of ... won’t act on the delegated account. Pass onBehalfOf through to PostExchange if delegation is carried via vaultAddress.
| // 8. Send to exchange. | ||
| resp, err := e.client.PostExchange(ctx, action, nonce, sigToWire(sig), input.VaultAddr, input.ExpiresAfter) | ||
| resp, err := e.client.PostExchange(ctx, action, nonce, sigToWire(sig), "", input.ExpiresAfter) |
There was a problem hiding this comment.
PlaceOrderInput.OnBehalfOf is not applied when sending the request: PostExchange is called with an empty vaultAddress. This makes --on-behalf-of ineffective for order placement. If delegated context is supposed to be conveyed via the /exchange vaultAddress field, pass input.OnBehalfOf to PostExchange (while keeping SignL1Action(..., nil, ...) if it must not affect the hash).
|
|
||
| return e.client.PostExchange(ctx, action, nonce, sigToWire(sig), vaultAddr, expiresAfter) | ||
| return e.client.PostExchange(ctx, action, nonce, sigToWire(sig), "", expiresAfter) |
There was a problem hiding this comment.
CancelOrders posts to /exchange with an empty vaultAddress, ignoring the onBehalfOf argument. That means order cancel / cancel-all with --on-behalf-of will target the signing account rather than the delegated account. Pass onBehalfOf through to PostExchange if that’s the intended mechanism.
|
|
||
| return e.client.PostExchange(ctx, action, nonce, sigToWire(sig), vaultAddr, expiresAfter) | ||
| return e.client.PostExchange(ctx, action, nonce, sigToWire(sig), "", expiresAfter) |
There was a problem hiding this comment.
CancelByCloid ignores onBehalfOf and always calls PostExchange with an empty vaultAddress, so --on-behalf-of won’t actually cancel orders on the delegated account. Pass onBehalfOf into PostExchange if delegation is meant to be carried via vaultAddress.
|
|
||
| return e.client.PostExchange(ctx, action, nonce, sigToWire(sig), input.VaultAddr, nil) | ||
| return e.client.PostExchange(ctx, action, nonce, sigToWire(sig), "", nil) |
There was a problem hiding this comment.
UpdateLeverageInput.OnBehalfOf isn’t applied: PostExchange is called with an empty vaultAddress. This makes position leverage --on-behalf-of ... ineffective. Pass input.OnBehalfOf to PostExchange if /exchange vaultAddress is the delegated-account mechanism.
|
|
||
| return e.client.PostExchange(ctx, action, nonce, sigToWire(sig), input.VaultAddr, nil) | ||
| return e.client.PostExchange(ctx, action, nonce, sigToWire(sig), "", nil) |
There was a problem hiding this comment.
UpdateIsolatedMarginInput.OnBehalfOf is ignored when posting to /exchange (empty vaultAddress). As a result, position margin --on-behalf-of ... will apply to the signing account. Pass input.OnBehalfOf through to PostExchange if that’s how account context is specified.
|
|
||
| resp, err := e.client.PostExchange(ctx, action, nonce, sigToWire(sig), input.VaultAddr, input.ExpiresAfter) | ||
| resp, err := e.client.PostExchange(ctx, action, nonce, sigToWire(sig), "", input.ExpiresAfter) | ||
| if err != nil { |
There was a problem hiding this comment.
ModifyOrderInput.OnBehalfOf isn’t used when sending the modify to /exchange (empty vaultAddress), even though open-order lookup can run against the delegated address. This mismatch can cause modify to be applied to the wrong account. Pass input.OnBehalfOf into PostExchange so the write context matches the read context.
|
Addressed the review feedback about inconsistent account address validation. Changes in commit c076c82:
Validation, lint, and full test suite pass via gofmt -s -w . ────────────────────────────────────────── |
|
Follow-up update with correct formatting: Addressed the review feedback about inconsistent account-address validation. Included in commit c076c82:
Pre-push checks passed via make check (fmt, tidy, verify, vet, lint, full tests). |
Summary
Validation
go mod tidy
go mod verify
all modules verified
go vet ./...
golangci-lint run
0 issues.
? github.com/timbrinded/hlgo [no test files]
ok github.com/timbrinded/hlgo/cmd 4.958s
ok github.com/timbrinded/hlgo/pkg/client 3.682s
ok github.com/timbrinded/hlgo/pkg/config 1.015s
ok github.com/timbrinded/hlgo/pkg/exchange 1.083s
ok github.com/timbrinded/hlgo/pkg/info 1.025s
ok github.com/timbrinded/hlgo/pkg/output 1.035s
ok github.com/timbrinded/hlgo/pkg/resolver 1.616s
ok github.com/timbrinded/hlgo/pkg/signer 1.023s
ok github.com/timbrinded/hlgo/pkg/wire 1.070s
──────────────────────────────────────────
Tests: 9 passed, 0 failed
──────────────────────────────────────────
integration_cli_test.go:1154: live account integration requires env vars: HL_TEST_WITHDRAW_DESTINATION, HL_TEST_WITHDRAW_AMOUNT, HL_TEST_SEND_ASSET_DESTINATION, HL_TEST_SEND_ASSET_TOKEN, HL_TEST_SEND_ASSET_AMOUNT, HL_TEST_SET_ABSTRACTION_USER, HL_TEST_SET_ABSTRACTION_VALUE
FAIL
FAIL github.com/timbrinded/hlgo/e2e 27.270s
FAIL