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

Jump to bottom

🐛 Nonce Defaulting to 0 in --interactive Mode #22

Closed
adamdossa opened this issue Mar 13, 2025 · 9 comments · Fixed by #24
Closed

🐛 Nonce Defaulting to 0 in --interactive Mode #22

adamdossa opened this issue Mar 13, 2025 · 9 comments · Fixed by #24
Assignees
@pcaversaccio
Labels
bug 🐛 Something isn't working

Comments

@adamdossa
Copy link

In order to verify hashes before submitting an initial signature, I am using:
./safe_hashes.sh --network ethereum --address XXX --nonce YYY --interactive
then entering the transaction data.

Despite specifying the nonce, the script seems to use a nonce of 0 - it is worth noting that at the point I'm running the script, the transaction has not yet been added to the SAFE API.

If I don't specify a nonce on the CLI, then it gives an error message:

Enter the Safe multisig version (default: 1.3.0):
Invalid `nonce` value: "". Must be a non-negative integer!
@pcaversaccio pcaversaccio self-assigned this Mar 13, 2025
@adamdossa
Copy link
Author

I proposed a fix in PR:
#23

@pcaversaccio pcaversaccio added the bug 🐛 Something isn't working label Mar 13, 2025
@pcaversaccio pcaversaccio changed the title Nonce defaulting to 0 in --interactive mode 🐛 Nonce Defaulting to 0 in --interactive Mode Mar 13, 2025
@pcaversaccio
Copy link
Owner

Oh I also made a PR: #24. Can you test that one please?

@adamdossa
Copy link
Author

Yep - that worked fine for me. In theory you can also default the to / address field from the --address passed in at the command line, but there is already an option to enter it, so it's not such a big issue.

@adamdossa
Copy link
Author

I guess it would be nice to make all parameters optional if you're using --interactive and just add them manually (i.e. no need to specify --address or --nonce at the command line if you don't want to use the SAFE API and just add them manually) but this may be a bigger change.

@pcaversaccio
Copy link
Owner

pcaversaccio commented Mar 13, 2025 •
edited
Loading

I don't want to break user space again here tbh for --interactive mode. Let's force people to always use --network, --address, and --nonce (except for off-chain signatures) on the CLI.

@pcaversaccio
Copy link
Owner

If you agree I will merge my PR and close yours to keep everything KISS.

@pcaversaccio
Copy link
Owner

Thx for raising it!

@adamdossa
Copy link
Author

Sounds good - thanks for the prompt fix (and the utility overall!).

@pcaversaccio
Copy link
Owner

Give me another hour or so before I merge it - I wanna run some sanity tests locally.

This was referenced Mar 13, 2025
Merged
Closed
pcaversaccio added a commit that referenced this issue Mar 13, 2025
@pcaversaccio
🐛 Fix Nonce Default in --interactive Mode (#24)
66643a9 
### 🕓 Changelog

Fixes #22. This PR removes the unnecessary `nonce` override since it is
already provided via the CLI. Additionally, fallback values are added
for the `version` and `response` parameters in case `curl` fails
silently. These changes ensure the script runs smoothly under all
conditions.

---------

Signed-off-by: Pascal Marco Caversaccio <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pcaversaccio pcaversaccio

Labels
bug 🐛 Something isn't working
Projects
None yet
Milestone
No milestone
2 participants
@adamdossa @pcaversaccio