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.

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

Add --rune flag to resume command to enable resuming a specific etching #3679

Merged
merged 10 commits into from
May 17, 2024

Conversation

ldiego08
Copy link
Contributor

@ldiego08 ldiego08 commented Apr 24, 2024

Adds a new --rune <RUNE> flag to allow resuming a specific pending etching. This is useful in automated scenarios (if anyone else is into that kind of stuff) where you'd like to etch something, suspend (perhaps via --no-wait from #3526), do something else, and then resume after the 6 confirmations.

Copy link
Collaborator

@raphjaph raphjaph left a comment

Choose a reason for hiding this comment

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

This is good feature.

We should probably resume it by rune name. Another feature that would improve this would be a command to list all pending etchings, so it is easier to know what can be resumed. Probably best to do another PR for that.

src/subcommand/wallet/resume.rs Outdated Show resolved Hide resolved
src/subcommand/wallet/resume.rs Outdated Show resolved Hide resolved
src/subcommand/wallet/resume.rs Outdated Show resolved Hide resolved
@ldiego08
Copy link
Contributor Author

ldiego08 commented May 6, 2024

Thanks for the feedback, @raphjaph! Requested changes have been applied. I've also fixed linting issues. Will add the command to show pending etchings on a separate PR.

@ldiego08 ldiego08 changed the title Add --commitment flag to enable resuming a specific etching Add --rune flag to resume command to enable resuming a specific etching May 6, 2024
@ldiego08 ldiego08 requested a review from raphjaph May 6, 2024 22:35
src/subcommand/wallet/resume.rs Outdated Show resolved Hide resolved
src/subcommand/wallet/resume.rs Outdated Show resolved Hide resolved
src/subcommand/wallet/resume.rs Outdated Show resolved Hide resolved
src/subcommand/wallet/resume.rs Outdated Show resolved Hide resolved
@ldiego08 ldiego08 requested a review from raphjaph May 7, 2024 17:49
Copy link
Collaborator

@raphjaph raphjaph left a comment

Choose a reason for hiding this comment

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

Almost there! Still needs an integration test. Have a look at tests/wallet/resume.rs. For reasons this can only be tested on Mac and Linux. What system are you on?

@ldiego08
Copy link
Contributor Author

Almost there! Still needs an integration test. Have a look at tests/wallet/resume.rs. For reasons this can only be tested on Mac and Linux. What system are you on?

@raphjaph took me a while to get around to adding these tests, but here they are! Hope it's looking good. I'm on a Mac, btw.

@ldiego08 ldiego08 requested a review from raphjaph May 12, 2024 20:51
Copy link
Collaborator

@raphjaph raphjaph left a comment

Choose a reason for hiding this comment

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

Great, LGTM!

@raphjaph raphjaph enabled auto-merge (squash) May 17, 2024 15:32
@raphjaph raphjaph merged commit 401a01e into ordinals:master May 17, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants