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 set-host command #14

Merged
merged 3 commits into from
May 11, 2023
Merged

Conversation

nicbus
Copy link
Member

@nicbus nicbus commented May 9, 2023

This PR adds the set-host command to set opret/tapret host, if needed.

This is intended to be called by a user that has prepared a PSBT with an output ready for the commitment but needing to set it as a host, via CLI.

This comes from (and depends on) rgb-wallet PR #46, which initially did the same in the pay command but now only makes opret and tapret modules public.

@nicbus nicbus marked this pull request as draft May 10, 2023 07:29
Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Some style improvements - otherwise LGTM. Why draft?

@dr-orlovsky dr-orlovsky added the enhancement New feature or request label May 10, 2023
@nicbus nicbus force-pushed the add_set-host_command branch from 5cbe900 to 733d074 Compare May 10, 2023 15:15
@nicbus
Copy link
Member Author

nicbus commented May 10, 2023

Some style improvements - otherwise LGTM

great, updated

Why draft?

This PR was set as draft since it could not work until rgb-wallet PR 46 was not merged.

note: I now introduced a commit that updates rgb-wallet and rgb-std to git/master so the set-host command can use the now-public opret/tapret modules.

@nicbus
Copy link
Member Author

nicbus commented May 10, 2023

added a commit to update an outdated github action (actually to re-trigger CI as it failed for an internal error on the previous run)

it still failed, though, with more internal errors

the PR IMO is ready, the only test that failed in the previous run (build/cli) succeeds locally (cargo build --no-default-features --features cli)

@nicbus nicbus marked this pull request as ready for review May 10, 2023 15:53
Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK with nits. If you want, I can merge it as is; but if you prefer to address nits, this is also an option

@nicbus nicbus force-pushed the add_set-host_command branch from 7aae7fe to 2237967 Compare May 10, 2023 21:04
@nicbus
Copy link
Member Author

nicbus commented May 10, 2023

ACK with nits. If you want, I can merge it as is; but if you prefer to address nits, this is also an option

liked the suggestions, updated

Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK 2237967

Very cool, thank you!

@dr-orlovsky dr-orlovsky merged commit 0fe17aa into RGB-WG:master May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants