Skip to content
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.

Sign up for GitHub

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

chore: clearer error message when function signature does not contain parentheses #9478

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

chore: clearer error message when function signature does not contain parentheses #9478

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1970dcc
chore: clearer error message when function signature does not contain…
yipu3 Dec 4, 2024
66273b6
Update crates/cli/src/utils/abi.rs
yipu3 Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/cli/src/utils/abi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,15 @@ pub async fn parse_function_args<T: Transport + Clone, P: Provider<T, AnyNetwork
let args = resolve_name_args(&args, provider).await;

if let Ok(data) = hex::decode(sig) {
return Ok((data, None))
return Ok((data, None));
}

let func = if sig.contains('(') {
// a regular function signature with parentheses
get_func(sig)?
} else {
let etherscan_api_key = etherscan_api_key.ok_or_eyre(
"If you wish to fetch function data from EtherScan, please provide an API key.",
"Function signature does not contain parentheses. If you wish to fetch function data from Etherscan, please provide an API key.",
Copy link
Member

@zerosnacks zerosnacks Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't make sense to include it in this error message to be honest

Should rather be different message or fall-through condition in this else block, not part of this error for handling missing Etherscan API key (which is valid on its own)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe when the signature does not contain parentheses the code try to treat the string as the function name and want to fetch the signature from Etherscan?

I think this message can help users that wrongly omit parentheses solve the problem faster without misleading info.

Copy link
Member

@zerosnacks zerosnacks Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is actually a problem with omitting parentheses here

You are having this issue because you need to pass an Etherscan API key and RPC URL.

The function deposit is also not a method on the Uniswap V2 Router but rather on WETH.

cast call 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2 'deposit' --etherscan-api-key <API_KEY> --rpc-url <RPC_URL>

cast call 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2 'deposit()' --etherscan-api-key <API_KEY> --rpc-url <RPC_URL>

yield the same result 0x, which is expected

cast call 0x7a250d5630B4cF539739dF2C5dAcb4c659F2488D 'factory' --etherscan-api-key <API_KEY> --rpc-url <RPC_URL>

yields: 0x5C69bEe701ef814a2B6a3EDD4B1652CB9cc5aA6f

whereas

cast call 0x7a250d5630B4cF539739dF2C5dAcb4c659F2488D 'factory()' --etherscan-api-key <API_KEY> --rpc-url <RPC_URL>

yields: 0x0000000000000000000000005c69bee701ef814a2b6a3edd4b1652cb9cc5aa6f

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it could be explained further that factory() is providing the full signature, while factory without parens is requesting a function called factory which can only be retrieved from an outside source

Copy link
Author

@yipu3 yipu3 Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My issue is when I wrongly omit the parentheses, the printed error message does not direct me to fix the problem.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yipu3 per comments above, we discussed a better UX would be to warn that the function will be retrieved from outside source, do you want to update the PR to reflect this? thanks

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thanks. I will update.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this:

Function is pure name. The signature will be retrieved from outside source, please provide an Etherscan API key.

Any suggestion?

)?;
let to = to.ok_or_eyre("A 'to' address must be provided to fetch function data.")?;
get_func_etherscan(sig, to, &args, chain, etherscan_api_key).await?
Expand Down
Loading