fix: validate config keys against known set in config set#12
Open
P-r-e-m-i-u-m wants to merge 4 commits intochainstacklabs:mainfrom
Open
fix: validate config keys against known set in config set#12P-r-e-m-i-u-m wants to merge 4 commits intochainstacklabs:mainfrom
P-r-e-m-i-u-m wants to merge 4 commits intochainstacklabs:mainfrom
Conversation
Signed-off-by: 🄂ʏᴇᴅ 🄰ʙᴅᴜʟ 🄰ᴍᴀ🄝 ✧ <amanbaba9404522@gmail.com>
📝 WalkthroughWalkthroughValidate keys in the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use your project's `ruff` configuration to improve the quality of Python code reviews.Add a Ruff configuration file to your project to customize how CodeRabbit runs |
smypmsa
previously approved these changes
Mar 19, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pumpfun_cli/commands/config.py`:
- Around line 21-31: The config key list is duplicated here (KNOWN_KEYS) causing
drift; remove KNOWN_KEYS from config_set and delegate validation to the core
config module (pumpfun_cli.core.config) by importing its authoritative map or
validator (e.g., ENV_MAP or a provided is_valid_key/validate_config_key
function) and call that from config_set, then call error(...) only when the core
validation fails; keep config_set as Typer wiring (parse args, call core
validator, then error()) and do not reimplement the key list in this file.
In `@tests/test_commands/test_config_cmd.py`:
- Around line 85-91: The test test_config_set_unknown_key should also assert the
failed key wasn't persisted: after invoking config set with the invalid key
(rpcc), call the CLI to list or dump config (e.g., runner.invoke(app, ["config",
"list", "--json"]) or another existing list/get command) and assert the output
does not contain "rpcc" (or that a JSON payload lacks that key); update
test_config_set_unknown_key to run that follow-up runner.invoke and include an
assertion that the invalid key is absent from the configuration output to catch
regressions that write invalid keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6cabd160-4a73-4e49-acbc-272c70ea4a54
📒 Files selected for processing (2)
src/pumpfun_cli/commands/config.pytests/test_commands/test_config_cmd.py
…hainstacklabs#17) - 11 core-layer unit tests verifying graduated→pumpswap routing sequence (mocked RPC) - 10 command-layer unit tests verifying parameter forwarding (slippage, priority-fee, dry-run, confirm) through the fallback path and edge cases (not_found does not trigger fallback, pool-not-found after graduation surfaces correctly) - 3 surfpool integration tests verifying full auto-routing end-to-end against a forked mainnet node No production code changed. Test delta: +21 unit tests (339→360), +3 surfpool tests. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ted in test Signed-off-by: 🄂ʏᴇᴅ 🄰ʙᴅᴜʟ 🄰ᴍᴀ🄝 ✧ <amanbaba9404522@gmail.com>
Author
|
"Moved KNOWN_KEYS to core/config.py derived from ENV_MAP to avoid drift, updated import in commands/config.py, and added assertion that invalid key is not persisted in the test. Ready for re-review @smypmsa 👍" |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_core/test_auto_routing.py`:
- Around line 176-230: Update the two tests
(test_buy_auto_route_forwards_slippage and
test_sell_auto_route_forwards_slippage) so they actually verify the passed
slippage is used: call buy_pumpswap/sell_pumpswap in a mode or with arguments
that return the dry_run details (or otherwise expose the computed minimum-out),
then assert that the dry_run/minimum-out field reflects slippage=5 (e.g.
min_base_amount_out equals ~95% of the estimated amount). Concretely, modify the
test to capture the returned dry_run object or computed min value from
buy_pumpswap/sell_pumpswap and add an assertion comparing it to the expected 95%
value rather than only asserting action/venue.
In `@tests/test_surfpool/test_auto_routing.py`:
- Around line 108-154: The test test_buy_auto_route_full_command_layer mutates
process-wide os.environ and the shared keystore; change it to avoid global state
by creating a private config dir under tmp_path (e.g., tmp_path /
"pumpfun-cli"), copy test_keystore to that dir as wallet.enc, construct an env
dict with PUMPFUN_RPC, PUMPFUN_PASSWORD and XDG_CONFIG_HOME=str(tmp_path) (do
not write to os.environ), and pass that env into runner.invoke(..., env=env) so
the CLI sees the isolated config; remove the os.environ writes and the
finally-pop pops so no global env is modified. Ensure you reference the same
symbols used in the test (test_keystore, tmp_path, runner.invoke, app,
graduated_mint) when applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1e0f19a1-a4fd-4bfb-9440-eb85d1e3bfec
📒 Files selected for processing (6)
src/pumpfun_cli/commands/config.pysrc/pumpfun_cli/core/config.pytests/test_commands/test_config_cmd.pytests/test_commands/test_trade_cmd.pytests/test_core/test_auto_routing.pytests/test_surfpool/test_auto_routing.py
Comment on lines
+176
to
+230
| @pytest.mark.asyncio | ||
| async def test_buy_auto_route_forwards_slippage(tmp_keystore): | ||
| """slippage=5 is forwarded to buy_pumpswap.""" | ||
| pool_data = build_pool_data() | ||
| with patch("pumpfun_cli.core.pumpswap.RpcClient") as MockClient: | ||
| client = AsyncMock() | ||
| MockClient.return_value = client | ||
| client.get_account_info.side_effect = [ | ||
| _mock_pool_resp(pool_data), | ||
| _mock_token_program_resp(), | ||
| _mock_global_config_resp(), | ||
| _mock_vol_accumulator_resp(), | ||
| ] | ||
| client.get_token_account_balance.side_effect = [1_000_000_000, 30_000_000_000] | ||
| client.get_balance.return_value = 10_000_000_000 | ||
| client.send_tx.return_value = "buysig" | ||
| client.close = AsyncMock() | ||
|
|
||
| result = await buy_pumpswap( | ||
| "http://rpc", tmp_keystore, "testpass", _VALID_MINT, 0.01, slippage=5 | ||
| ) | ||
|
|
||
| assert result["action"] == "buy" | ||
| assert result["venue"] == "pumpswap" | ||
| # With slippage=5, min_base_amount_out should be 95% of estimated | ||
| # (verified by the fact that the trade succeeded with slippage=5) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_sell_auto_route_forwards_slippage(tmp_keystore): | ||
| """slippage=5 is forwarded to sell_pumpswap.""" | ||
| pool_data = build_pool_data() | ||
| with patch("pumpfun_cli.core.pumpswap.RpcClient") as MockClient: | ||
| client = AsyncMock() | ||
| MockClient.return_value = client | ||
| client.get_account_info.side_effect = [ | ||
| _mock_pool_resp(pool_data), | ||
| _mock_token_program_resp(), | ||
| _mock_global_config_resp(), | ||
| ] | ||
| client.get_token_account_balance.side_effect = [ | ||
| 1_000_000, | ||
| 1_000_000_000, | ||
| 30_000_000_000, | ||
| ] | ||
| client.send_tx.return_value = "sellsig" | ||
| client.close = AsyncMock() | ||
|
|
||
| result = await sell_pumpswap( | ||
| "http://rpc", tmp_keystore, "testpass", _VALID_MINT, "all", slippage=5 | ||
| ) | ||
|
|
||
| assert result["action"] == "sell" | ||
| assert result["venue"] == "pumpswap" | ||
|
|
There was a problem hiding this comment.
These slippage tests don't currently prove slippage handling.
Both tests call buy_pumpswap / sell_pumpswap directly and only assert success, so they still pass if the implementation ignores the passed slippage and uses the default. Assert on a slippage-derived field in dry_run output, or inspect the computed minimum-out value instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_core/test_auto_routing.py` around lines 176 - 230, Update the two
tests (test_buy_auto_route_forwards_slippage and
test_sell_auto_route_forwards_slippage) so they actually verify the passed
slippage is used: call buy_pumpswap/sell_pumpswap in a mode or with arguments
that return the dry_run details (or otherwise expose the computed minimum-out),
then assert that the dry_run/minimum-out field reflects slippage=5 (e.g.
min_base_amount_out equals ~95% of the estimated amount). Concretely, modify the
test to capture the returned dry_run object or computed min value from
buy_pumpswap/sell_pumpswap and add an assertion comparing it to the expected 95%
value rather than only asserting action/venue.
Comment on lines
+108
to
+154
| def test_buy_auto_route_full_command_layer( | ||
| surfpool_rpc, funded_keypair, test_keystore, test_password, graduated_mint, tmp_path | ||
| ): | ||
| """CliRunner buy without --force-amm shows venue=pumpswap.""" | ||
| import json | ||
| import os | ||
|
|
||
| # Set up env for CLI | ||
| env = { | ||
| "PUMPFUN_RPC": surfpool_rpc, | ||
| "PUMPFUN_PASSWORD": test_password, | ||
| "XDG_CONFIG_HOME": str(test_keystore.parent.parent), | ||
| } | ||
| for key, val in env.items(): | ||
| os.environ[key] = val | ||
|
|
||
| try: | ||
| # Rename the keystore dir to match what the CLI expects | ||
| import shutil | ||
|
|
||
| cli_config_dir = test_keystore.parent.parent / "pumpfun-cli" | ||
| if not cli_config_dir.exists(): | ||
| cli_config_dir.mkdir() | ||
| cli_wallet = cli_config_dir / "wallet.enc" | ||
| if not cli_wallet.exists(): | ||
| shutil.copy2(str(test_keystore), str(cli_wallet)) | ||
|
|
||
| result = runner.invoke( | ||
| app, | ||
| [ | ||
| "--json", | ||
| "--rpc", | ||
| surfpool_rpc, | ||
| "buy", | ||
| graduated_mint, | ||
| "0.001", | ||
| "--slippage", | ||
| "50", | ||
| ], | ||
| ) | ||
|
|
||
| assert result.exit_code == 0, f"CLI failed: {result.output}" | ||
| data = json.loads(result.output) | ||
| assert data["venue"] == "pumpswap" | ||
| finally: | ||
| for key in env: | ||
| os.environ.pop(key, None) |
There was a problem hiding this comment.
Isolate this CLI integration test from process-wide environment state.
This block writes directly to os.environ, copies the wallet into a shared session directory, and then blindly deletes the keys in finally. That can clobber pre-existing values and make later surfpool tests order-dependent. Use runner.invoke(..., env=env) and point XDG_CONFIG_HOME at tmp_path so the copied wallet is private to this test.
♻️ Suggested isolation pattern
def test_buy_auto_route_full_command_layer(
surfpool_rpc, funded_keypair, test_keystore, test_password, graduated_mint, tmp_path
):
"""CliRunner buy without --force-amm shows venue=pumpswap."""
import json
- import os
+ import shutil
- # Set up env for CLI
+ cli_config_dir = tmp_path / "pumpfun-cli"
+ cli_config_dir.mkdir()
+ shutil.copy2(test_keystore, cli_config_dir / "wallet.enc")
+
env = {
"PUMPFUN_RPC": surfpool_rpc,
"PUMPFUN_PASSWORD": test_password,
- "XDG_CONFIG_HOME": str(test_keystore.parent.parent),
+ "XDG_CONFIG_HOME": str(tmp_path),
}
- for key, val in env.items():
- os.environ[key] = val
-
- try:
- # Rename the keystore dir to match what the CLI expects
- import shutil
-
- cli_config_dir = test_keystore.parent.parent / "pumpfun-cli"
- if not cli_config_dir.exists():
- cli_config_dir.mkdir()
- cli_wallet = cli_config_dir / "wallet.enc"
- if not cli_wallet.exists():
- shutil.copy2(str(test_keystore), str(cli_wallet))
-
- result = runner.invoke(
- app,
- [
- "--json",
- "--rpc",
- surfpool_rpc,
- "buy",
- graduated_mint,
- "0.001",
- "--slippage",
- "50",
- ],
- )
-
- assert result.exit_code == 0, f"CLI failed: {result.output}"
- data = json.loads(result.output)
- assert data["venue"] == "pumpswap"
- finally:
- for key in env:
- os.environ.pop(key, None)
+ result = runner.invoke(
+ app,
+ [
+ "--json",
+ "--rpc",
+ surfpool_rpc,
+ "buy",
+ graduated_mint,
+ "0.001",
+ "--slippage",
+ "50",
+ ],
+ env=env,
+ )
+
+ assert result.exit_code == 0, f"CLI failed: {result.output}"
+ data = json.loads(result.output)
+ assert data["venue"] == "pumpswap"🧰 Tools
🪛 Ruff (0.15.6)
[warning] 109-109: Unused function argument: funded_keypair
(ARG001)
[warning] 109-109: Unused function argument: tmp_path
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_surfpool/test_auto_routing.py` around lines 108 - 154, The test
test_buy_auto_route_full_command_layer mutates process-wide os.environ and the
shared keystore; change it to avoid global state by creating a private config
dir under tmp_path (e.g., tmp_path / "pumpfun-cli"), copy test_keystore to that
dir as wallet.enc, construct an env dict with PUMPFUN_RPC, PUMPFUN_PASSWORD and
XDG_CONFIG_HOME=str(tmp_path) (do not write to os.environ), and pass that env
into runner.invoke(..., env=env) so the CLI sees the isolated config; remove the
os.environ writes and the finally-pop pops so no global env is modified. Ensure
you reference the same symbols used in the test (test_keystore, tmp_path,
runner.invoke, app, graduated_mint) when applying the change.
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #6
Added validation in
config_set()to reject unknown config keys before saving.Changes
src/pumpfun_cli/commands/config.pyKNOWN_KEYS = {"rpc", "keyfile", "priority_fee", "compute_units"}config_set()— rejects unknown keys with a clear error and hint showing valid keystests/test_commands/test_config_cmd.pytest_config_set_unknown_key— asserts that typos likerpccare rejected with exit code != 0 and error message containing "unknown config key" and "valid keys"Risk Assessment: Low Risk — Configuration Validation Only
This PR adds strict key validation to the CLI config setter and a corresponding test. It does not modify any code that constructs or sends Solana transactions.
What changed
Security / funds impact
Architectural considerations
Tests
Overall: defensive input validation that safely reduces misconfiguration risk without touching transaction-sending or wallet-handling implementation.