Conversation
* Problem: wrong cmd runs with chain_binary * Revert "Problem: wrong cmd runs with chain_binary" This reverts commit 8f26321. * Problem: cmd flag don't support multiple chains * changelog * Update pystarport/cluster.py Signed-off-by: yihuang <huang@crypto.com> --------- Signed-off-by: yihuang <huang@crypto.com> Co-authored-by: HuangYi <huang@crypto.com>
…m#143) * Problem: debug-addr rename to debug-listen-addr in relayer for more info, https://github.com/cosmos/relayer/pull/1504/files * keep previous
…m#145) * Problem: event-query-tx-for not compatible with chain-main * Update CHANGELOG.md Signed-off-by: yihuang <huang@crypto.com> * fix lint --------- Signed-off-by: yihuang <huang@crypto.com>
Bumps [setuptools](https://github.com/pypa/setuptools) from 70.0.0 to 78.1.1. - [Release notes](https://github.com/pypa/setuptools/releases) - [Changelog](https://github.com/pypa/setuptools/blob/main/NEWS.rst) - [Commits](pypa/setuptools@v70.0.0...v78.1.1) --- updated-dependencies: - dependency-name: setuptools dependency-version: 78.1.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
WalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClusterCLI
participant CosmosCLI
Note right of ClusterCLI #DDEBF7: node/account creation flow
User->>ClusterCLI: create_node(..., coin_type)
ClusterCLI->>ClusterCLI: determine data_dir per-chain
ClusterCLI->>ClusterCLI: create_account(..., coin_type / **kwargs)
ClusterCLI->>CosmosCLI: create_account(..., coin_type / **kwargs)
CosmosCLI-->>ClusterCLI: account info (address/pubkey)
sequenceDiagram
participant User
participant CosmosCLI
participant ChainCmd
Note right of CosmosCLI #F7F3DD: tx submit + optional event-query
User->>CosmosCLI: submit tx (transfer/delegate/...)
CosmosCLI->>ChainCmd: run tx command
ChainCmd-->>CosmosCLI: response {code, txhash}
alt has_event_query_tx_for && event_query_tx && response.code == 0
CosmosCLI->>ChainCmd: event_query_tx_for(response.txhash)
ChainCmd-->>CosmosCLI: event details
end
CosmosCLI-->>User: final response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
🔭 Outside diff range comments (1)
pystarport/cosmoscli.py (1)
159-173: 🛠️ Refactor suggestionThread-shared state is fragile
self.output/self.errorare mutable instance attributes; concurrent ledger calls will race. Use local variables or a small dataclass to carry the result back from the thread.
🧹 Nitpick comments (5)
pystarport/cosmoscli.py (2)
298-319: Error-string matching is brittleRelying on substring
"CommunityPool query exposed by the external community pool"ties the code to one exact SDK error. Prefer catching a specific exception type or probing module support via--helpfirst.
416-423: DRY: factor out duplicate success-handlerThe 5-line
if rsp["code"] == 0 …block is repeated many times. Extract to_post_tx(rsp)helper to cut maintenance overhead and avoid divergent future fixes.pystarport/cluster.py (2)
189-190: Document newcoin_typeparam
create_node()signature changed but docstring didn’t. Add a short note describing the flag and default (118).
1373-1388: Graceful skip when no app-configNice safeguard. Minor:
grpc_port = grpc_address.split(":")[1]can raise if address lacks colon. Usersplit(":",1)with default.CHANGELOG.md (1)
30-31: Grammar“binary that don't” → “binary that doesn't”
“suppport” → “support”.🧰 Tools
🪛 LanguageTool
[grammar] ~30-~30: There seems to be a noun/verb agreement error. Did you mean “doesn't” or “didn't”?
Context: ...5) Backward compatible with binary that don't have event-query-tx-for. - [crypto-com#147](https...(SINGULAR_NOUN_AGREEMENT_WHO_DONT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.flake8(1 hunks)CHANGELOG.md(1 hunks)pystarport/cluster.py(11 hunks)pystarport/convert.sh(0 hunks)pystarport/cosmoscli.py(31 hunks)pystarport/default.nix(0 hunks)
💤 Files with no reviewable changes (2)
- pystarport/default.nix
- pystarport/convert.sh
🧰 Additional context used
🧬 Code Graph Analysis (2)
pystarport/cluster.py (2)
pystarport/cosmoscli.py (6)
create_account(133-157)create_account_ledger(159-187)distribution_community(298-319)make_multisig(595-605)ibc_denom(1487-1498)account(346-351)pystarport/ports.py (1)
oracle_port(37-38)
pystarport/cosmoscli.py (2)
pystarport/cluster.py (7)
create_account(315-317)create_account(906-937)create_account_ledger(319-321)distribution_community(372-373)event_query_tx_for(778-779)make_multisig(514-515)ibc_denom(838-839)pystarport/utils.py (1)
parse_amount(71-80)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~30-~30: There seems to be a noun/verb agreement error. Did you mean “doesn't” or “didn't”?
Context: ...5) Backward compatible with binary that don't have event-query-tx-for. - [crypto-com#147](https...
(SINGULAR_NOUN_AGREEMENT_WHO_DONT)
🪛 Ruff (0.11.9)
pystarport/cluster.py
1256-1259: Use ternary operator cmds = cmd.split(",") if cmd is not None else [None] * len(chains) instead of if-else-block
Replace if-else-block with cmds = cmd.split(",") if cmd is not None else [None] * len(chains)
(SIM108)
🪛 Pylint (3.3.7)
pystarport/cluster.py
[refactor] 1261-1261: Redefining argument with the local name 'cmd'
(R1704)
🔇 Additional comments (10)
.flake8 (1)
2-4: Config looks sensibleBlack-friendly
E203ignore and 88-char line length are standard; exclude list is explicit. No issues.pystarport/cosmoscli.py (3)
133-156: Pass-through kwargs needs validationBlindly forwarding
**kwargstokeys addlets callers inject arbitrary CLI flags (e.g.--home,--output) and break invariants. Recommend whitelisting accepted options (currently onlycoin_type) or documenting that unrestricted flags are intentional.
294-297: Good compatibility tweakGracefully handles nested commission response – nice!
1487-1498: LGTM – neat helperHashing path once and re-using existing REST endpoint keeps things simple.
pystarport/cluster.py (6)
261-262: Propagates coin_type correctlyNice to see the new option wired through.
315-322: Same kwargs-validation concernFor consistency, consider the same whitelisting / validation here as in
CosmosCLI.create_account.
372-374: Expose extra query flags – good addition
514-516: LGTM
838-840: LGTM – thin delegate
1303-1305: Typo / duplication
api-listen-addrand newly addeddebug-listen-addrboth set to:5183; confirm rly accepts duplicates and this port isn’t already bound.
pystarport/cosmoscli.py
Outdated
| def prob_event_query_tx_for(self): | ||
| 'test if the command has "event-query-tx-for" subcommand' | ||
| try: | ||
| output = self("q", "event-query-tx-for") | ||
| except AssertionError: | ||
| # non-zero return code | ||
| return False | ||
|
|
||
| return "Available Commands" in output.decode() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Detection may give false negatives
chain-maind q event-query-tx-for usually needs extra args; running it without may return usage (exit-code 0) on some versions, yielding a misleading “sub-command supported” result. Consider adding --help or checking exit-code == 0 and help text to reduce false positives.
🤖 Prompt for AI Agents
In pystarport/cosmoscli.py around lines 71 to 79, the prob_event_query_tx_for
method runs the subcommand without arguments, which may return usage text with
exit code 0 and cause false positives. Modify the method to run the subcommand
with a --help argument or similar, then check that the exit code is 0 and the
output contains expected help text to confirm the subcommand is supported
reliably.
| # for multiple chains, there can be multiple cmds splited by `,` | ||
| if cmd is not None: | ||
| cmds = cmd.split(",") | ||
| else: | ||
| cmds = [None] * len(chains) | ||
|
|
There was a problem hiding this comment.
Edge-case: command count mismatch
zip(chains, cmds) silently drops extra chains if fewer cmds are supplied. Warn or pad cmds to len(chains) to avoid half-initialised clusters.
- if cmd is not None:
- cmds = cmd.split(",")
- else:
- cmds = [None] * len(chains)
+ cmds = (cmd.split(",") if cmd is not None else [])
+ if len(cmds) < len(chains):
+ cmds.extend([None] * (len(chains) - len(cmds)))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # for multiple chains, there can be multiple cmds splited by `,` | |
| if cmd is not None: | |
| cmds = cmd.split(",") | |
| else: | |
| cmds = [None] * len(chains) | |
| # for multiple chains, there can be multiple cmds splited by `,` | |
| cmds = cmd.split(",") if cmd is not None else [] | |
| if len(cmds) < len(chains): | |
| cmds.extend([None] * (len(chains) - len(cmds))) |
🧰 Tools
🪛 Ruff (0.11.9)
1256-1259: Use ternary operator cmds = cmd.split(",") if cmd is not None else [None] * len(chains) instead of if-else-block
Replace if-else-block with cmds = cmd.split(",") if cmd is not None else [None] * len(chains)
(SIM108)
🤖 Prompt for AI Agents
In pystarport/cluster.py around lines 1255 to 1260, the code splits cmds by
commas but does not handle cases where the number of cmds is less than the
number of chains, causing zip to drop extra chains silently. Modify the code to
check if cmds length is less than chains length and pad cmds with None or empty
strings to match the length of chains, ensuring all chains are initialized
properly without being dropped.
| def node_id(self): | ||
| "get tendermint node id" | ||
| output = self.raw("tendermint", "show-node-id", home=self.data_dir) | ||
| subcmd = "tendermint" if self.has_tendermint_subcommand else "comet" |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (4)
pystarport/cosmoscli.py (4)
145-158: Don’t pass coin_type through to CLI; map to key_type and drop it (handle str/int uniformly).Older CLIs may not recognize --coin-type; after mapping coin_type==60 to key_type, drop coin_type to avoid “unknown flag” errors. Also accept both int 60 and string "60".
- if kwargs.get("coin_type") == 60: - kwargs["key_type"] = "eth_secp256k1" + # Map coin_type to key_type (eth) and avoid passing coin_type to the CLI + ct = kwargs.pop("coin_type", None) + if ct is not None and str(ct) == "60": + kwargs.setdefault("key_type", "eth_secp256k1")Also applies to: 169-170
173-189: Mirror the coin_type-to-key_type mapping behaviour for ledger keys.Same rationale as create_account: remove coin_type after mapping to prevent unknown-flag issues and accept both 60 and "60".
- if kwargs.get("coin_type") == 60: - kwargs["key_type"] = "eth_secp256k1" + ct = kwargs.pop("coin_type", None) + if ct is not None and str(ct) == "60": + kwargs.setdefault("key_type", "eth_secp256k1")
1474-1490: Add event_query_tx toggle to pay_packet_fee for consistencyThe
pay_packet_feecall still always queries events whenhas_event_query_tx_foris true—even when consumers may want to disable it. Align its signature and gating with other tx methods.Location:
pystarport/cosmoscli.py, around lines 1474–1490Suggested diff:
- def pay_packet_fee(self, port_id, channel_id, packet_seq, **kwargs): + def pay_packet_fee(self, port_id, channel_id, packet_seq, event_query_tx=True, **kwargs): @@ - if rsp["code"] == 0 and self.has_event_query_tx_for: + if rsp["code"] == 0 and event_query_tx and self.has_event_query_tx_for: rsp = self.event_query_tx_for(rsp["txhash"])
1453-1472: Ensure event_query_tx toggle is added to both fee‐related tx methodsBoth
register_counterparty_payeeandpay_packet_feecurrently always post‐query whenhas_event_query_tx_foris true, but lack theevent_query_txparameter found on other tx methods. Add the toggle for caller control:Affected locations:
- pystarport/cosmoscli.py:
register_counterparty_payee(around line 1453)- pystarport/cosmoscli.py:
pay_packet_fee(around line 1474)Suggested diff:
--- a/pystarport/cosmoscli.py +++ b/pystarport/cosmoscli.py @@ def register_counterparty_payee( - def register_counterparty_payee( - self, port_id, channel_id, relayer, counterparty_payee, **kwargs + def register_counterparty_payee( + self, port_id, channel_id, relayer, counterparty_payee, + event_query_tx=True, **kwargs ): @@ - if rsp["code"] == 0 and self.has_event_query_tx_for: + if rsp["code"] == 0 and event_query_tx and self.has_event_query_tx_for: rsp = self.event_query_tx_for(rsp["txhash"]) return rsp @@ def pay_packet_fee(self, port_id, channel_id, packet_seq, **kwargs): - def pay_packet_fee(self, port_id, channel_id, packet_seq, **kwargs): + def pay_packet_fee( + self, port_id, channel_id, packet_seq, + event_query_tx=True, **kwargs + ): @@ - if rsp["code"] == 0 and self.has_event_query_tx_for: + if rsp["code"] == 0 and event_query_tx and self.has_event_query_tx_for: rsp = self.event_query_tx_for(rsp["txhash"]) return rsp
♻️ Duplicate comments (1)
pystarport/cosmoscli.py (1)
81-89: Make event-query-tx-for capability probe reliable (use --help; avoid “Available Commands” check).Running the subcommand without args can return “usage” with exit code 0 on some versions, causing false positives/negatives. Also, leaf commands won’t print “Available Commands”. Probe with --help and look for expected help text.
Apply this diff:
def prob_event_query_tx_for(self): 'test if the command has "event-query-tx-for" subcommand' try: - output = self("q", "event-query-tx-for") + # Use --help to reduce false positives from bare invocation + output = self("q", "event-query-tx-for", "--help") except AssertionError: # non-zero return code return False - return "Available Commands" in output.decode() + txt = output.decode() + # Leaf command: expect its name and standard help headers + return "event-query-tx-for" in txt and ("Usage" in txt or "Flags" in txt)
🧹 Nitpick comments (1)
pystarport/cosmoscli.py (1)
126-130: Nit: Update the docstring to reflect comet fallback.You now select tendermint or comet dynamically; make the docstring generic.
- def node_id(self): - "get tendermint node id" + def node_id(self): + "get node id (tendermint/comet)"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
pystarport/cosmoscli.py(33 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pystarport/cosmoscli.py (2)
pystarport/cluster.py (10)
node_id(307-309)home(286-288)create_account(315-317)create_account(906-937)create_account_ledger(319-321)distribution_community(372-373)node_rpc(293-295)event_query_tx_for(778-779)make_multisig(514-515)ibc_denom(838-839)pystarport/utils.py (1)
parse_amount(71-80)
🔇 Additional comments (2)
pystarport/cosmoscli.py (2)
432-438: Event query gating looks good.Conditional invocation of event-query-tx-for based on rsp["code"] == 0, generate_only, and capability probe avoids unnecessary calls and errors on unsupported CLIs.
1505-1516: LGTM: IBC denom query helper is correct and useful.Hashing the path and querying ibc-transfer denom aligns with ICS20; returning the denom string is the expected output.
| res = res["commission"] | ||
| return parse_amount(res[0]) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against empty commission arrays.
When no commission has accrued, res can be an empty list, leading to IndexError on res[0].
if isinstance(res, dict):
res = res["commission"]
- return parse_amount(res[0])
+ if not res:
+ return 0
+ return parse_amount(res[0])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| res = res["commission"] | |
| return parse_amount(res[0]) | |
| if isinstance(res, dict): | |
| res = res["commission"] | |
| if not res: | |
| return 0 | |
| return parse_amount(res[0]) |
🤖 Prompt for AI Agents
In pystarport/cosmoscli.py around lines 311 to 313, the code assumes
res["commission"] is a non-empty list and does return parse_amount(res[0]),
which raises IndexError when commission is empty; guard by checking if res is a
list and empty (len(res) == 0) and in that case return a zero amount (e.g.,
return parse_amount("0") or the function's canonical zero value); otherwise
proceed to parse res[0] as before.
| def distribution_community(self, **kwargs): | ||
| for module in ["distribution", "protocolpool"]: | ||
| try: | ||
| res = json.loads( | ||
| self.raw( | ||
| "query", | ||
| module, | ||
| "community-pool", | ||
| output="json", | ||
| node=self.node_rpc, | ||
| **kwargs, | ||
| ) | ||
| ) | ||
| return parse_amount(res["pool"][0]) | ||
| except Exception as e: | ||
| if ( | ||
| module == "distribution" | ||
| and "CommunityPool query exposed by the external community pool" | ||
| in str(e) | ||
| ): | ||
| continue | ||
| raise | ||
|
|
There was a problem hiding this comment.
Handle empty community pool safely; avoid index errors and continue fallback cleanly.
If pool is empty, res["pool"][0] raises IndexError. Return 0 instead. Keep the existing “distribution” -> “protocolpool” fallback.
- return parse_amount(res["pool"][0])
+ pool = res.get("pool") or []
+ if not pool:
+ return 0
+ return parse_amount(pool[0])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def distribution_community(self, **kwargs): | |
| for module in ["distribution", "protocolpool"]: | |
| try: | |
| res = json.loads( | |
| self.raw( | |
| "query", | |
| module, | |
| "community-pool", | |
| output="json", | |
| node=self.node_rpc, | |
| **kwargs, | |
| ) | |
| ) | |
| return parse_amount(res["pool"][0]) | |
| except Exception as e: | |
| if ( | |
| module == "distribution" | |
| and "CommunityPool query exposed by the external community pool" | |
| in str(e) | |
| ): | |
| continue | |
| raise | |
| def distribution_community(self, **kwargs): | |
| for module in ["distribution", "protocolpool"]: | |
| try: | |
| res = json.loads( | |
| self.raw( | |
| "query", | |
| module, | |
| "community-pool", | |
| output="json", | |
| node=self.node_rpc, | |
| **kwargs, | |
| ) | |
| ) | |
| pool = res.get("pool") or [] | |
| if not pool: | |
| return 0 | |
| return parse_amount(pool[0]) | |
| except Exception as e: | |
| if ( | |
| module == "distribution" | |
| and "CommunityPool query exposed by the external community pool" | |
| in str(e) | |
| ): | |
| continue | |
| raise |
🤖 Prompt for AI Agents
pystarport/cosmoscli.py around lines 314-336: the code assumes res["pool"][0]
exists and will raise IndexError when the community pool is empty; after parsing
the JSON, check res.get("pool", []) and if it's empty return 0, otherwise call
parse_amount on the first element; keep the existing try/except and the
distribution→protocolpool fallback behavior unchanged so only empty-pool cases
return 0 instead of raising.
…alidation for gentx (#5)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pystarport/cluster.py (1)
1267-1274: Edge-case: command count mismatch silently drops chains (zip truncation)When fewer cmds than chains are supplied, zip() truncates, leaving some chains uninitialized. Pad cmds to match chains length. Also satisfies Ruff’s SIM108 (ternary).
Apply this diff:
- # for multiple chains, there can be multiple cmds splited by `,` - if cmd is not None: - cmds = cmd.split(",") - else: - cmds = [None] * len(chains) + # for multiple chains, there can be multiple cmds split by `,` + cmds = cmd.split(",") if cmd is not None else [] + if len(cmds) < len(chains): + cmds.extend([None] * (len(chains) - len(cmds))) for chain, cmd in zip(chains, cmds):
🧹 Nitpick comments (2)
pystarport/cluster.py (2)
189-190: Add coin_type to create_node: good propagation hookAdding coin_type here is appropriate and aligns with downstream handling in cosmoscli. Consider also persisting this value into the validators entry you append (only when provided) so it’s discoverable later (parity with config-driven init).
Example (outside this hunk):
# inside the validators.append(...) block: validator_cfg = { "base_port": base_port, "hostname": hostname, "moniker": moniker, } if coin_type is not None: validator_cfg["coin-type"] = coin_type self.config["validators"].append(validator_cfg)
907-918: Normalize coin_type to int to ensure key_type switch triggersIf coin_type is provided as a string from YAML (e.g., "60"), CosmosCLI’s comparison (== 60) won’t match. Normalize early to avoid surprises.
Apply this diff:
- coin_type = account.get("coin-type") + coin_type = account.get("coin-type") + if coin_type is not None: + try: + coin_type = int(coin_type) + except (TypeError, ValueError): + raise ValueError(f"invalid coin-type: {coin_type!r}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
pystarport/cluster.py(11 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pystarport/cluster.py (3)
pystarport/cosmoscli.py (7)
create_account(145-171)create_account_ledger(173-203)distribution_community(314-335)make_multisig(611-621)ibc_denom(1505-1516)account(362-367)gentx(233-245)pystarport/cli.py (1)
cli(174-181)pystarport/ports.py (1)
oracle_port(37-38)
🪛 Ruff (0.12.2)
pystarport/cluster.py
1268-1271: Use ternary operator cmds = cmd.split(",") if cmd is not None else [None] * len(chains) instead of if-else-block
Replace if-else-block with cmds = cmd.split(",") if cmd is not None else [None] * len(chains)
(SIM108)
🔇 Additional comments (8)
pystarport/cluster.py (8)
261-261: LGTM: pass coin_type during validator account creationThe new coin_type is correctly forwarded into account creation.
315-318: LGTM: create_account forwards kwargsForwarding kwargs keeps ClusterCLI in sync with CosmosCLI (e.g., coin_type). Signature and call are consistent.
319-322: LGTM: create_account_ledger forwards kwargsLedger variant mirrors the standard account creation path and supports coin_type. Good.
372-374: LGTM: distribution_community passthroughPropagating kwargs allows selective module behaviors/upstream flags. Matches CosmosCLI changes.
514-516: LGTM: make_multisig passthroughExtending to kwargs is consistent with CosmosCLI.
838-840: LGTM: ibc_denom forwarding methodThis cleanly exposes the CosmosCLI ibc_denom helper through ClusterCLI.
1023-1026: LGTM: propagate per-node coin-type into validator account creationThis aligns validator keys with the chain’s configured coin-type.
1315-1317: LGTM: relayer debug knobs (rly)Adding debug-listen-addr and enable-debug-server improves operability. Defaults are reasonable.
| gentx_extra_args = [config.get("cmd-flags")] | ||
| # build extra positional args; inject --offline if sequence or account_number set | ||
| has_acct = "account_number" in extra_kwargs | ||
| has_seq = "sequence" in extra_kwargs | ||
| if has_acct ^ has_seq: # xor: only one provided | ||
| raise ValueError("Both 'account_number' and 'sequence' must be provided together for offline gentx") | ||
| if has_acct and has_seq: | ||
| gentx_extra_args.append("--offline") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid passing None or opaque “cmd-flags” into gentx args
gentx_extra_args currently includes config.get("cmd-flags") even when None, which risks propagating None into the underlying CLI builder. Also, if cmd-flags is a list/tuple, flattening is safer.
Apply this diff:
- gentx_extra_args = [config.get("cmd-flags")]
+ gentx_extra_args = []
+ flags = config.get("cmd-flags")
+ if flags:
+ if isinstance(flags, (list, tuple)):
+ gentx_extra_args.extend(map(str, flags))
+ else:
+ gentx_extra_args.append(str(flags))
# build extra positional args; inject --offline if sequence or account_number set
has_acct = "account_number" in extra_kwargs
has_seq = "sequence" in extra_kwargs
if has_acct ^ has_seq: # xor: only one provided
raise ValueError("Both 'account_number' and 'sequence' must be provided together for offline gentx")
if has_acct and has_seq:
gentx_extra_args.append("--offline")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gentx_extra_args = [config.get("cmd-flags")] | |
| # build extra positional args; inject --offline if sequence or account_number set | |
| has_acct = "account_number" in extra_kwargs | |
| has_seq = "sequence" in extra_kwargs | |
| if has_acct ^ has_seq: # xor: only one provided | |
| raise ValueError("Both 'account_number' and 'sequence' must be provided together for offline gentx") | |
| if has_acct and has_seq: | |
| gentx_extra_args.append("--offline") | |
| gentx_extra_args = [] | |
| flags = config.get("cmd-flags") | |
| if flags: | |
| if isinstance(flags, (list, tuple)): | |
| gentx_extra_args.extend(map(str, flags)) | |
| else: | |
| gentx_extra_args.append(str(flags)) | |
| # build extra positional args; inject --offline if sequence or account_number set | |
| has_acct = "account_number" in extra_kwargs | |
| has_seq = "sequence" in extra_kwargs | |
| if has_acct ^ has_seq: # xor: only one provided | |
| raise ValueError("Both 'account_number' and 'sequence' must be provided together for offline gentx") | |
| if has_acct and has_seq: | |
| gentx_extra_args.append("--offline") |
🤖 Prompt for AI Agents
In pystarport/cluster.py around lines 1050 to 1057, gentx_extra_args currently
unconditionally includes config.get("cmd-flags") which may be None or a non-flat
sequence; change the logic to skip None/empty values and to normalize/flatten
cmd-flags: if cmd-flags is a string, split it into tokens; if it's a list/tuple,
extend gentx_extra_args with its items; if it's a single other value, append it;
ensure you only modify gentx_extra_args when the normalized flags are non-empty
so no None or nested sequences are passed to the CLI builder.
| app_cfg = node.get("app-config", {}) | ||
| if not app_cfg: | ||
| continue | ||
| oracle_config = app_cfg.get("oracle", {}) | ||
| if oracle_config.get("enabled"): | ||
| oracle_port = ports.oracle_port(node["base_port"]) | ||
| grpc_address = node["app-config"].get("grpc", {}).get("address", "") | ||
| grpc_address = app_cfg.get("grpc", {}).get("address", "") | ||
| grpc_port = grpc_address.split(":")[1] if ":" in grpc_address else ports.grpc_port(node["base_port"]) | ||
| oracle_section = f"program:{chain_id}-node{i}-oracle" |
There was a problem hiding this comment.
Bug: grpc_port extraction breaks for addresses with scheme (e.g., tcp://127.0.0.1:9090)
Using split(":")[1] returns the second segment, which is incorrect when a scheme is present. Use rsplit(":", 1) to reliably capture the port; also preserve the existing fallback.
Apply this diff:
- grpc_address = app_cfg.get("grpc", {}).get("address", "")
- grpc_port = grpc_address.split(":")[1] if ":" in grpc_address else ports.grpc_port(node["base_port"])
+ grpc_address = app_cfg.get("grpc", {}).get("address", "")
+ grpc_port = ports.grpc_port(node["base_port"])
+ if ":" in grpc_address:
+ grpc_port = grpc_address.rsplit(":", 1)[-1]Optionally, guard for IPv6 literals with brackets in the future.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app_cfg = node.get("app-config", {}) | |
| if not app_cfg: | |
| continue | |
| oracle_config = app_cfg.get("oracle", {}) | |
| if oracle_config.get("enabled"): | |
| oracle_port = ports.oracle_port(node["base_port"]) | |
| grpc_address = node["app-config"].get("grpc", {}).get("address", "") | |
| grpc_address = app_cfg.get("grpc", {}).get("address", "") | |
| grpc_port = grpc_address.split(":")[1] if ":" in grpc_address else ports.grpc_port(node["base_port"]) | |
| oracle_section = f"program:{chain_id}-node{i}-oracle" | |
| app_cfg = node.get("app-config", {}) | |
| if not app_cfg: | |
| continue | |
| oracle_config = app_cfg.get("oracle", {}) | |
| if oracle_config.get("enabled"): | |
| oracle_port = ports.oracle_port(node["base_port"]) | |
| grpc_address = app_cfg.get("grpc", {}).get("address", "") | |
| grpc_port = ports.grpc_port(node["base_port"]) | |
| if ":" in grpc_address: | |
| grpc_port = grpc_address.rsplit(":", 1)[-1] | |
| oracle_section = f"program:{chain_id}-node{i}-oracle" |
Solution: - raw json config can be used if user want to expand jsonnet by itself
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pystarport/cluster.py (3)
1271-1278: Handle cmd count vs. chain count; avoid silent truncation by zipzip drops extra chains if fewer cmds are provided. Pad cmds to match chains and keep behavior explicit. Also satisfies Ruff SIM108 if you keep the ternary.
Apply this diff:
- # for multiple chains, there can be multiple cmds splited by `,` - if cmd is not None: - cmds = cmd.split(",") - else: - cmds = [None] * len(chains) - - for chain, cmd in zip(chains, cmds): + # for multiple chains, there can be multiple cmds split by `,` + cmds = cmd.split(",") if cmd is not None else [] + if len(cmds) < len(chains): + cmds.extend([None] * (len(chains) - len(cmds))) + # optionally warn if extra cmds > chains (ignored) + for chain, cmd in zip(chains, cmds):
1050-1057: Avoid passing None and normalize cmd-flags; keep offline gatingCurrently gentx_extra_args = [config.get("cmd-flags")] may inject None and/or an unparsed string. Normalize and skip empties.
Apply this diff:
- gentx_extra_args = [config.get("cmd-flags")] - # build extra positional args; inject --offline if sequence or account_number set + gentx_extra_args = [] + # normalize optional CLI flags (string/list/tuple); skip None/empty + flags = config.get("cmd-flags") + if flags: + if isinstance(flags, str): + gentx_extra_args.extend(shlex.split(flags)) + elif isinstance(flags, (list, tuple)): + gentx_extra_args.extend(map(str, flags)) + else: + gentx_extra_args.append(str(flags)) + # build extra positional args; inject --offline if sequence or account_number set has_acct = "account_number" in extra_kwargs has_seq = "sequence" in extra_kwargs if has_acct ^ has_seq: # xor: only one provided raise ValueError("Both 'account_number' and 'sequence' must be provided together for offline gentx") if has_acct and has_seq: gentx_extra_args.append("--offline")Add this import at the top of the file:
import shlex
1395-1396: grpc_port parsing breaks with scheme; use rsplit with safe fallbacksplit(":")[1] is wrong for values like tcp://127.0.0.1:9090. Use rsplit(":", 1).
Apply this diff:
- grpc_address = app_cfg.get("grpc", {}).get("address", "") - grpc_port = grpc_address.split(":")[1] if ":" in grpc_address else ports.grpc_port(node["base_port"]) + grpc_address = app_cfg.get("grpc", {}).get("address", "") + grpc_port = ports.grpc_port(node["base_port"]) + if ":" in grpc_address: + grpc_port = grpc_address.rsplit(":", 1)[-1]
🧹 Nitpick comments (1)
pystarport/cluster.py (1)
1319-1321: Potential port conflict: api-listen-addr and debug-listen-addr both :5183RLY may try to bind two servers to the same port. Consider a separate default (e.g., :5184) or make it configurable.
Apply this diff:
- "api-listen-addr": ":5183", - "debug-listen-addr": ":5183", + "api-listen-addr": ":5183", + "debug-listen-addr": ":5184",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
pystarport/cluster.py(11 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
pystarport/cluster.py
1272-1275: Use ternary operator cmds = cmd.split(",") if cmd is not None else [None] * len(chains) instead of if-else-block
Replace if-else-block with cmds = cmd.split(",") if cmd is not None else [None] * len(chains)
(SIM108)
🔇 Additional comments (12)
pystarport/cluster.py (12)
189-190: Good addition: coin_type surfaced on node creationPassing coin_type at node creation is useful for downstream key derivation.
261-262: Coin type forwarded to validator accountCorrectly propagates coin_type into account creation.
315-318: create_account now forwards kwargsAllows future extensibility (e.g., coin_type). Looks good.
319-322: create_account_ledger forwards kwargsLedger flow can now accept coin_type. All good.
372-374: distribution_community forwards kwargsKeeps CLI surface consistent. OK.
514-516: make_multisig forwards kwargsConsistent forwarding; fine.
838-840: New ibc_denom passthroughUseful convenience method; matches ibc_denom_trace counterpart.
907-918: coin_type respected during devnet account creationBoth ledger and mnemonic flows handle coin_type correctly.
1023-1026: Validator account coin_type honored from node configCorrect propagation during init_devnet.
1034-1046: Expanded gentx optional fieldsAdditional gentx kwargs (account_number, sequence, gas, fees) are reasonable and align with CLI expectations.
1256-1263: JSON config support and explicit error on unsupported formatsNice enhancement; error message is clear.
1389-1392: Fix aligns with PR goal: guard when no app-config providedSkipping oracle process when app-config is absent prevents KeyError. Good.
Problem: chain id is not refreshed based on config
event-query-tx-for rename to wait-tx as well
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
pystarport/cosmoscli.py (2)
303-324: Handle empty community pool safely.The past review comment about empty pool arrays remains unaddressed. If the pool is empty,
res["pool"][0]raisesIndexError. The module fallback logic is good, but empty pool handling is still needed.Apply this diff to address the issue:
**kwargs, ) ) - return parse_amount(res["pool"][0]) + pool = res.get("pool") or [] + if not pool: + return 0 + return parse_amount(pool[0]) except Exception as e:
299-301: Guard against empty commission arrays.The past review comment about empty arrays remains unaddressed. When no commission has accrued,
rescan be an empty list after the dict extraction, causingIndexErroronres[0].Apply this diff to address the issue:
if isinstance(res, dict): res = res["commission"] +if not res: + return 0 return parse_amount(res[0])
🧹 Nitpick comments (1)
pystarport/cosmoscli.py (1)
71-79: Potential detection reliability issue.Similar to the existing concern with
prob_event_query_tx_for, runningtendermintwithout arguments may return usage text with exit code 0, potentially causing false positives. Consider adding--helpor verifying both exit code and help text content.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
pystarport/cosmoscli.py(14 hunks)
🔇 Additional comments (10)
pystarport/cosmoscli.py (10)
113-113: LGTM!The
has_tendermint_subcommandattribute is properly initialized and follows the established pattern for capability detection.
117-118: LGTM!The conditional subcommand selection properly handles compatibility between versions that use "tendermint" versus "comet" subcommands.
134-160: LGTM!The coin_type to key_type mapping for Ethereum (coin_type==60) is correct, and kwargs forwarding enhances extensibility.
162-192: LGTM!The changes are consistent with
create_accountand properly handle Ethereum key types for Ledger devices.
421-426: LGTM!The event_query_tx gating correctly preserves generate-only behavior and allows callers to conditionally disable event querying, improving flexibility.
461-466: LGTM!The event_query_tx gating is consistent with the
transfermethod and correctly handles the conditional behavior.
598-607: LGTM!The kwargs forwarding enhances extensibility and is consistent with other method updates in this PR.
758-761: LGTM!The conditional subcommand selection maintains compatibility across different chain versions and is consistent with the
node_idmethod.
805-808: LGTM!The conditional subcommand selection is consistent with
create_validatorand properly handles version compatibility.
1491-1503: LGTM!The new
ibc_denommethod is correctly implemented, follows the same pattern asibc_denom_trace, and uses standard SHA256 hashing for the denom calculation.
3 participants
and sync upstream
Summary by CodeRabbit
New Features
Chores
Bug Fixes