Uh oh!
There was an error while loading. Please reload this page.
Follow SEP-41 argument names in token commands regardless of contract naming - #2708
Follow SEP-41 argument names in token commands regardless of contract naming#2708fnando wants to merge 1 commit into
Conversation
25faf84 to
ceefadaCompareThere was a problem hiding this comment.
Pull request overview
Updates token commands to map SEP-41 arguments by position through the generic contract invocation pipeline, supporting contracts with non-canonical parameter names.
Changes:
- Adds positional contract argument parsing with strict arity validation.
- Migrates token balance, decimals, and transfer calls to positional invocation.
- Adds unit and integration coverage using a renamed-parameter token fixture.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
cmd/soroban-cli/src/commands/token/transfer.rs | Uses positional transfer arguments. |
cmd/soroban-cli/src/commands/token/balance.rs | Uses positional balance and decimals arguments. |
cmd/soroban-cli/src/commands/token/args.rs | Adds shared positional token invocation helper. |
cmd/soroban-cli/src/commands/contract/invoke.rs | Supports internal positional invocations. |
cmd/soroban-cli/src/commands/contract/arg_parsing.rs | Implements positional parsing, arity checks, and unit tests. |
cmd/crates/soroban-test/tests/it/integration/token/renamed.rs | Tests renamed SEP-41 parameters end to end. |
cmd/crates/soroban-test/tests/it/integration/token/mod.rs | Registers the new integration tests. |
cmd/crates/soroban-test/tests/fixtures/test-wasms/token_renamed/src/lib.rs | Defines the renamed-parameter token fixture. |
cmd/crates/soroban-test/tests/fixtures/test-wasms/token_renamed/Cargo.toml | Configures the fixture crate. |
Cargo.lock | Records the new fixture package. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| vec![ | ||
| OsString::from("balance"), | ||
| OsString::from("--id"), | ||
| OsString::from(&account), | ||
| ], | ||
| "balance", | ||
| vec![account], |
There was a problem hiding this comment.
It's a bit unclear to me how this works, is the idea that the fns lose the named options like --id and are no longer in the form stellar token balance --id G... and become positional, for e.g. stellar token balance G...? That deviation from the way that stellar contract invoke works seems unnecessary, since the main problem being addressed is that a contract might use different names for fields other than those defined in SEP-41, and we can solve that problem with a change that diverges much less from the contract invoke subcommand by playing a game of aliasing.
By aliasing I mean taking the arguments as resolved, and adding an alias to the each field with their name as known by SEP-41 given their ordered position in the fn.
There was a problem hiding this comment.
The example I use above with 'id' being the account being queried is incorrect, instead using transfer as a better example:
Today I understand the command as:
stellar token transfer --id C... --from G... --to G... --amount 2
Can we introduce aliasing, so that a contract whose invoke command looks like this because of custom argument names:
stellar contract invoke --id C... -- transfer --src G... --dst G... --amount 2
Can be used as follows via aliasing:
stellar token transfer --id C... --src G... --dst G... --amount 2
or
stellar token transfer --id C... --from G... --to G... --amount 2
Or if supporting the custom names is undesirable, simply replacing the names but keeping the use of options/flags:
stellar token transfer --id C... --from G... --to G... --amount 2
There was a problem hiding this comment.
Because invoking a contract requires knowing the argument names as defined in the spec, we can't assume developers followed the naming as defined in SEP-41. E.g. fn allowance(env: Env, from: Address, spender: Address) -> i128; could be defined as fn allowance(env: Env, addr: Address, consumer: Address) -> i128;, which would need to be invoked as --addr <address> --consumer <address>.
So, instead, this allows us to call the function with positional arguments instead, which then is mapped to the actual contract's names.
The command's name must be stable and will match SEP-41's spec (e.g. fn allowance(env: Env, from: Address, spender: Address) -> i128; will directly translate to --from <from> --spender <spender>).
There was a problem hiding this comment.
One really big downside to positional cli's though, it's easy to mixup and misunderstand the parameters which for a token transfer can be detrimental.
For example, it's difficult to determine in the following positional cli use who is the sender and receiver:
stellar token transfer C... C... C... 2
Where-as with the use of aliasing, we retain that explicitness still fully addressing the issue of field names:
stellar token transfer --id C... --from G... --to G... --amount 2
If we want positional arguments, then I think we should do it on all invokes too rather than making this a bespoke behaviour for the token commands. The stellar contract invoke command could support both named and positional invocation. And then the token command would benefit from that and we'd have more consistency across the CLI.
Whichever way we go here, it can work for all the invoking commands, either through aliasing (which achieves the same thing) or through supporting positional on both. My preference is aliasing because I think the explicitness makes it safer.
What
stellar token balanceandstellar token transfernow map their arguments to the contract by position instead of by the contract's parameter names. Callers supply values in SEP-41's canonical order (transfer(from, to, amount),balance(id),decimals()) and the contract's actual parameter names no longer matter.Why
The commands used to build an argv like
transfer --from … --to … --amount …and let the generic invoke parser match those flags to the contract spec's parameter names. That only worked when the deployed contract literally named its parametersfrom/to/amount/id. A SEP-41-compliant contract can name them anything (sender,recipient,amt,addr, …), so the commands broke against those contracts. SEP-41 fixes the function names and argument order, not the parameter names — so mapping by position is the correct model. Doing this now keeps the two existing commands correct before more token subcommands (approve, allowance, mint, burn, …) are built on the same pattern.Known limitations
Positional mapping requires the contract function's arity to match the SEP-41 signature exactly; a deviating arity fails with a clear error rather than sending a malformed call. Attacker-influenceable spec parameter names are sanitized before appearing in any error message, consistent with the existing invoke path.