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

[ADP-3305] Add send faucet end-point to local cluster #4566

Merged
merged 7 commits into from
May 9, 2024
Merged

[ADP-3305] Add send faucet end-point to local cluster #4566

merged 7 commits into from
May 9, 2024

Conversation

paolino
Copy link
Collaborator

@paolino paolino commented Apr 30, 2024 •
edited
Loading

  • Add a send faucet assets end-point to the local cluster http service

ADP-3305

@paolino paolino self-assigned this Apr 30, 2024
@paolino paolino changed the base branch from master to paolino/ADP-3305/add-monitoring-and-control-to-local-cluster April 30, 2024 14:21
@paolino paolino changed the title Paolino/adp 3305/add send faucet end point to local cluster [ADP-3305] add send faucet end point to local cluster Apr 30, 2024
@paolino paolino changed the title [ADP-3305] add send faucet end point to local cluster [ADP-3305] Add send faucet end-point to local cluster Apr 30, 2024
@paolino paolino added CI CI related Conway PRs to prepare to the Conway bump Deposit and removed Conway PRs to prepare to the Conway bump labels Apr 30, 2024
@paolino paolino force-pushed the paolino/ADP-3305/add-monitoring-and-control-to-local-cluster branch 7 times, most recently from 658ccbf to 70ee798 Compare May 2, 2024 10:53
Base automatically changed from paolino/ADP-3305/add-monitoring-and-control-to-local-cluster to master May 2, 2024 13:05
@paolino paolino force-pushed the paolino/ADP-3305/add-send-faucet-end-point-to-local-cluster branch from dd8c4d2 to 5738324 Compare May 3, 2024 09:55
@paolino paolino changed the base branch from master to paolino/ADP-3305/add-node-socket-option May 3, 2024 16:17
@paolino paolino force-pushed the paolino/ADP-3305/add-send-faucet-end-point-to-local-cluster branch from c6044e3 to 6b163b4 Compare May 3, 2024 16:21
@paolino paolino marked this pull request as ready for review May 3, 2024 17:59
@paolino paolino marked this pull request as draft May 6, 2024 13:32
@paolino paolino force-pushed the paolino/ADP-3305/add-send-faucet-end-point-to-local-cluster branch 4 times, most recently from 31c3d71 to ed57c88 Compare May 6, 2024 14:38
@paolino paolino marked this pull request as ready for review May 6, 2024 14:49
@paolino paolino force-pushed the paolino/ADP-3305/add-node-socket-option branch 2 times, most recently from 0380a12 to e323174 Compare May 7, 2024 08:06
@paolino paolino force-pushed the paolino/ADP-3305/add-send-faucet-end-point-to-local-cluster branch from ed57c88 to 92d722a Compare May 7, 2024 08:07
@paolino paolino force-pushed the paolino/ADP-3305/add-node-socket-option branch from e323174 to 33aa815 Compare May 7, 2024 10:49
@paolino paolino force-pushed the paolino/ADP-3305/add-send-faucet-end-point-to-local-cluster branch from 92d722a to bce55b8 Compare May 7, 2024 10:51
HeinrichApfelmus
HeinrichApfelmus reviewed May 7, 2024
View reviewed changes
Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus left a comment

Choose a reason for hiding this comment

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

Thank you! 😊

The code itself looks good to me, but I feel that the separation of concerns is confused. 🤔 I would have expected that the HTTP API for the local cluster sits under

Cardano.Wallet.Launch.Cluster.Http.*

and that the monitoring part is grouped under

Cardano.Wallet.Launch.Cluster.Http.Monitoring.*

while the faucet part is grouped under

Cardano.Wallet.Launch.Cluster.Http.Faucet.*
or ``Cardano.Wallet.Launch.Cluster.Http.Application.*`

It's true that the local cluster is a group of nodes that are running, but at the same time, I think it's also fair to say that "this group has an HTTP interface" (this interface happens to be served by the local-cluster executable) as opposed to "this group can be monitored by an additional process, and the monitor to has an HTTP interface". I think it's conceptually simpler to attribute the HTTP interface to "the cluster".

lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Monitoring/Http/ApiT.hs Outdated Show resolved Hide resolved
lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Monitoring/Http/Application/Server.hs Outdated Show resolved Hide resolved
lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Monitoring/Http/Client.hs Outdated Show resolved Hide resolved
lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Monitoring/Http/Control/API.hs Outdated
{-# LANGUAGE UndecidableInstances #-}
{-# OPTIONS_GHC -Wno-orphans #-}

module Cardano.Wallet.Launch.Cluster.Monitoring.Http.Control.API
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
module Cardano.Wallet.Launch.Cluster.Monitoring.Http.Control.API
module Cardano.Wallet.Launch.Cluster.Http.Monitoring.API

? 🤔

lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Monitoring/Http/Control/Client.hs Outdated Show resolved Hide resolved
lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Monitoring/Http/SendFaucetAssets.hs Outdated Show resolved Hide resolved
lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Monitoring/Http/SendFaucetAssets.hs Outdated Show resolved Hide resolved
lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Monitoring/Monitor.hs Outdated Show resolved Hide resolved
Base automatically changed from paolino/ADP-3305/add-node-socket-option to master May 9, 2024 10:09
@paolino paolino force-pushed the paolino/ADP-3305/add-send-faucet-end-point-to-local-cluster branch 2 times, most recently from f2a96ba to 0c27826 Compare May 9, 2024 14:30
@paolino paolino force-pushed the paolino/ADP-3305/add-send-faucet-end-point-to-local-cluster branch from 0c27826 to 6329afc Compare May 9, 2024 14:39
HeinrichApfelmus
HeinrichApfelmus approved these changes May 9, 2024
View reviewed changes
Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus left a comment

Choose a reason for hiding this comment

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

Excellent, thank you! 😊

A couple of suggestion to make the use of AssetMetadata more consistent, but otherwise, good to go.

lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Http/Faucet/SendFaucetAssets.hs Outdated Show resolved Hide resolved
lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Http/Faucet/SendFaucetAssets.hs Outdated Show resolved Hide resolved
lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Http/Faucet/SendFaucetAssets.hs Outdated Show resolved Hide resolved
lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Http/Faucet/SendFaucetAssets.hs Outdated Show resolved Hide resolved
lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Http/Faucet/SendFaucetAssets.hs Outdated Show resolved Hide resolved
lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Http/Faucet/SendFaucetAssets.hs Outdated Show resolved Hide resolved
lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Http/Faucet/SendFaucetAssets.hs Outdated Show resolved Hide resolved
lib/local-cluster/lib/Cardano/Wallet/Launch/Cluster/Http/Faucet/SendFaucetAssets.hs Outdated Show resolved Hide resolved
@paolino paolino force-pushed the paolino/ADP-3305/add-send-faucet-end-point-to-local-cluster branch from 6329afc to adf56ab Compare May 9, 2024 15:36
@paolino paolino enabled auto-merge May 9, 2024 15:36
@paolino paolino added this pull request to the merge queue May 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 9, 2024
@paolino paolino force-pushed the paolino/ADP-3305/add-send-faucet-end-point-to-local-cluster branch from 16aceef to 65c1da1 Compare May 9, 2024 19:08
@paolino paolino merged commit 6dc7c03 into master May 9, 2024
3 checks passed
@paolino paolino deleted the paolino/ADP-3305/add-send-faucet-end-point-to-local-cluster branch May 9, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HeinrichApfelmus HeinrichApfelmus HeinrichApfelmus approved these changes

Assignees

@paolino paolino

Labels
CI CI related Deposit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@paolino @HeinrichApfelmus