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

add option to set inscription destination #1536

Merged
merged 12 commits into from
Feb 20, 2023
Merged

add option to set inscription destination #1536

merged 12 commits into from
Feb 20, 2023

Conversation

rot13maxi
Copy link
Contributor

Split just this feature out of #1512

@rot13maxi rot13maxi mentioned this pull request Feb 7, 2023
Closed
casey
casey previously requested changes Feb 7, 2023
View reviewed changes
Copy link
Collaborator

@casey casey left a comment

Choose a reason for hiding this comment

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

Nice, a few random comments. Love the test, but I think we need an integration test that does this e2e w/the flag.

src/subcommand/wallet/inscribe.rs Outdated Show resolved Hide resolved
src/subcommand/wallet/inscribe.rs Outdated Show resolved Hide resolved
src/subcommand/wallet/inscribe.rs Outdated Show resolved Hide resolved
@rot13maxi
address PR feedback
39c27c7
@rot13maxi
Merge remote-tracking branch 'upstream/master' into inscription-desti…
5cf8add 
…nation

# Conflicts:
#	src/subcommand/preview.rs
#	tests/wallet/inscribe.rs
@rot13maxi rot13maxi requested a review from casey February 10, 2023 02:24
@rot13maxi
Copy link
Contributor Author

rot13maxi commented Feb 10, 2023 •
edited
Loading

Moved the test to an integration test, and cleaned up the other bits. Left a comment in the thread about the closure around getting a ready-made destination address. happy to go either way.

@rot13maxi
Copy link
Contributor Author

@casey feedback addressed when you have time to take another look :)

@raphjaph raphjaph enabled auto-merge (squash) February 20, 2023 18:45
@raphjaph raphjaph merged commit cc112e2 into ordinals:master Feb 20, 2023
@veryordinally veryordinally mentioned this pull request Aug 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raphjaph raphjaph raphjaph approved these changes

@casey casey Awaiting requested review from casey

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rot13maxi @casey @raphjaph