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

Surface Bitcoin RPC used by fedimintd #5905

Merged
merged 4 commits into from
Sep 4, 2024
Merged

Surface Bitcoin RPC used by fedimintd #5905

merged 4 commits into from
Sep 4, 2024

Conversation

bradleystachurski
Copy link
Member

@bradleystachurski bradleystachurski commented Aug 22, 2024 •
edited
Loading

Followup addressing:

Can we assert in a test that the correct blockchain API is used to avoid regressions? Do you think that would be useful?

#5902 (review)

Adds two endpoints to wallet server:

  • authed endpoint that returns bitcoin rpc kind and url
    • auth required since bitcoin url should not be leaked to public
    • useful for surfacing to guardians in UI
    • url is stripped of user:pass prior to sending over the wire
  • no-auth endpoint that returns the bitcoin kind for a peer
    • useful for surfacing to clients (e.g. fedimint-observer)
bash-5.2$ fedimint-cli module wallet --help
Usage: wallet <COMMAND>

Commands:
  await-deposit           Await a deposit on a given deposit address
  get-bitcoin-rpc-kind    Returns the Bitcoin RPC kind
  get-bitcoin-rpc-config  Returns the Bitcoin RPC kind and URL, if authenticated
  help                    Print this message or the help of the given subcommand(s)

bash-5.2$ fedimint-cli module wallet get-bitcoin-rpc-kind 0
"bitcoind"

bash-5.2$ FM_OUR_ID=0 FM_PASSWORD=pass fedimint-cli module wallet get-bitcoin-rpc-config
{
  "kind": "bitcoind",
  "url": "http://127.0.0.1:11368/"
}

bradleystachurski
bradleystachurski commented Aug 22, 2024
View reviewed changes
modules/fedimint-wallet-tests/tests/tests.rs

// Verify the correct Bitcoin RPC is used

#[cfg(feature = "bitcoind")]
Copy link
Member Author

Choose a reason for hiding this comment

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

Another approach is using #[ignore], but that makes the calls to cargo nextest messier since it requires multiple flags and filters.

@bradleystachurski bradleystachurski marked this pull request as ready for review August 22, 2024 21:16
@bradleystachurski bradleystachurski requested review from a team as code owners August 22, 2024 21:16
dpc
dpc previously approved these changes Aug 22, 2024
View reviewed changes
@dpc
Copy link
Contributor

dpc commented Aug 22, 2024

@bradleystachurski

I thought that maybe we'll need fedimint-cli dev get-bitcoin-backend kind of thing.

@elsirion
Copy link
Contributor

I think @Kodylow could use the local config, especially the bitcoin backend in use, exposed as an admin API route anyway, maybe that way we could use an admin CLI tool like mentioned by @dpc to fetch and check it?

@bradleystachurski
Copy link
Member Author

Moving to draft until CI passes

@bradleystachurski bradleystachurski marked this pull request as draft September 4, 2024 15:38
@bradleystachurski bradleystachurski changed the title chore(tests): verify correct bitcoin rpc is used Surface Bitcoin RPC used by fedimintd Sep 4, 2024
@bradleystachurski bradleystachurski marked this pull request as ready for review September 4, 2024 17:41
@bradleystachurski bradleystachurski requested a review from a team as a code owner September 4, 2024 17:41
@bradleystachurski bradleystachurski added this pull request to the merge queue Sep 4, 2024
Merged via the queue into fedimint:master with commit c2a7865 Sep 4, 2024
23 checks passed
@bradleystachurski bradleystachurski deleted the assert-bitcoin-backend branch September 4, 2024 18:47
@bradleystachurski bradleystachurski mentioned this pull request Sep 4, 2024
Merged
@elsirion elsirion mentioned this pull request Sep 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpc dpc dpc approved these changes

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
@bradleystachurski @dpc @elsirion