-
Notifications
You must be signed in to change notification settings - Fork 217
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
SMASH Integration #1884
SMASH Integration #1884
Conversation
d0a8191
to
7b9e38e
Compare
c7748df
to
dd00d12
Compare
Although fallback is removed I see there is a fallback going on in case of SMASH with expired SSL:
|
There is minor issue in the log, missing
|
28b9d57
to
17fb68a
Compare
Fixed the URL in the log (was missing a trailing slash in the Also, I've double-checked about the fallback and I do observe the correct behavior: --pool-metadata-proxy https://smash.shelley-testnet.dev.cardano.org/api/v1/metadata
--pool-metadata-proxy https://expired.badssl.com/
|
{ uriPath = "/" <> intercalate "/" (pathSegments baseUrl ++ [hash]) | ||
} | ||
where | ||
hash = T.unpack $ T.decodeUtf8 $ convertToBase Base16 bytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hash = T.unpack $ T.decodeUtf8 $ convertToBase Base16 bytes | |
hash = B8.unpack $ convertToBase Base16 bytes |
-- Try each builder in order, but only if the previous builder led to a | ||
-- "ConnectionFailure" exception. Other exceptions mean that we could | ||
-- likely reach the server. | ||
fromFirst (builder:rest) = do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this a little confusing. Can we separate the function which makes a request and reads at most 512 bytes from the response from the function which tries each builder and uses the first which succeeds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try that 👍
lib/cli/src/Cardano/CLI.hs
Outdated
poolMetadataProxyOption | ||
:: Parser URI | ||
poolMetadataProxyOption = option (eitherReader reader) $ mempty | ||
<> long "pool-metadata-proxy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not use the word "proxy" where it could be confused with actual HTTP proxies.
We could just say --smash-server
.
And what about making the CF smash server the default, with an option to disable the metadata aggregator?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not use the word "proxy" where it could be confused with actual HTTP proxies.
Fair point. Although I dislike the "--smash-server" naming for this because in principle, smash is one implementation of a metadata registry. There could be others by the community that aren't smash. Hence why I wanted to keep the name rather "generic". Perhaps --pool-metadata-registry
?
And what about making the CF smash server the default, with an option to disable the metadata aggregator?
There's no CF server yet as far as I know 😬 ... plus, with all the networks and candidates at the moment, it's sort of hard to know what a sensible default would be (other than, no default). Long-term, I do however agree, once the dust settle down a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought "smash" was a generic name, or at least something that implements the smash API.
I have asked for clarification input-output-hk/smash#22.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, I'll for "--smash-server" since it's more transparent for now. We can always change later.
Indeed, I must have not updated the branch, sorry. Ok, small thing, but maybe we would like to consider allowing only
which results in:
Just a thought. |
The idea is the following: - We have a list of "URL builders" that will be tried in order, but only if the first builder replied with a connection failure (so, only if the server is somewhat down) - We can always have our current behavior by using the 'identityUrlBuilder'. - Then, we can augment this behavior if given a specific registry URL via the command-line options, which will add a "registryUrlBuilder" which takes predecence over the identity builder.
Going for a command-line option so that this can easily be turned on and off at will in case of any issue. Plus, not everyone might agree with the idea of going through a registry so hard-coding the registry URL is a no go. cardano-wallet-jormungandr uses an ad-hoc environment variable for this, but a documented option in the command-line is definitely cleaner.
…side_ the fetcher So that we can clearly log fallback attempts and keep it clean.
f7453c3
to
9bc10de
Compare
runExceptTLog | ||
:: ExceptT String IO StakePoolMetadata | ||
-> IO (Maybe StakePoolMetadata) | ||
runExceptTLog action = runExceptT action >>= \case | ||
Left msg -> | ||
Nothing <$ traceWith tr (MsgFetchPoolMetadataFailure hash msg) | ||
|
||
Right meta -> | ||
Just meta <$ traceWith tr (MsgFetchPoolMetadataSuccess hash meta) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously I have combined Success/Failure log messages into a single message type, e.g.
| MsgFetchPoolMetadataResult StakePoolMetadataHash (Either String StakePoolMetadata)
This way is no less expressive for logging, and means you can use a single traceWith
statement and avoid needing to use constructs like runExceptTLog
.
I added this @piotr-iohk so that the error can be caught earlier. |
bors r+ |
1884: SMASH Integration r=rvl a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #1883 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - d790880 📍 **allow different ways of building metadata URL from metadata information** The idea is the following: - We have a list of "URL builders" that will be tried in order, but only if the first builder replied with a connection failure (so, only if the server is somewhat down) - We can always have our current behavior by using the 'identityUrlBuilder'. - Then, we can augment this behavior if given a specific registry URL via the command-line options, which will add a "registryUrlBuilder" which takes predecence over the identity builder. - 48884cd 📍 **try fetching metadata from the next builder on HTTP exception** - c133609 📍 **add command-line option for enabling a pool metadata proxy** Going for a command-line option so that this can easily be turned on and off at will in case of any issue. Plus, not everyone might agree with the idea of going through a registry so hard-coding the registry URL is a no go. cardano-wallet-jormungandr uses an ad-hoc environment variable for this, but a documented option in the command-line is definitely cleaner. - e9dbf56 📍 **revise logging around pool metadata fetching to have more insight _inside_ the fetcher** So that we can clearly log fallback attempts and keep it clean. - cf00db6 📍 **Regenerate nix** - 0830589 📍 **use 'Either HttpException' as a base monad instead of 'ExceptT'** The underlying URL builder relies on the 'MonadThrow' class to send errors. Yet, when using 'ExceptT', the error is thrown _in the base monad_!! So, using 'Either' instead in conjunction with `except` to catch the error as expected in the loopl, instead of throwing hard in the parent context. - dd00d12 📍 **be more specific when interpreting responses from the aggregation server** - 28b9d57 📍 **remove fallback when a pool metadata proxy is supplied.** The fallback made sense in the early days, but we can now make it a proper switch: it's either fetching from pools, or it's fetching from the registry # Comments <!-- Additional comments or screenshots to attach if any --> - [ ]⚠️ TODO: show that it works. So far, on the shelley testnet: Some quick manual / sanity testing: ```console $ cardano-wallet-shelley serve \ --node-socket node.socket \ --testnet shelley_testnet-genesis.json \ --pool-metadata-proxy https://smash.shelley-testnet.dev.cardano.org/api/v1/metadata [...] [2020-07-08 19:57:09.29 UTC] Fetching metadata with hash c9ed7096 from https://smash.shelley-testnet.dev.cardano.orgapi/v1/metadata/c9ed70969c964284ff854d61e43c2591b67ceb7e978347cb1b50ba3ed1d3edb4 [2020-07-08 19:57:09.45 UTC] Successfully fetched metadata with hash c9ed7096: StakePoolMetadata {ticker = StakePoolTicker {unStakePoolTicker = "IOHK1"}, name = "IOHK1", description = Just "IOHK Pool", homepage = "https://iohk.io"} ``` ```console $ cardano-wallet-shelley serve \ --node-socket node.socket \ --testnet shelley_testnet-genesis.json \ --pool-metadata-proxy patate option --pool-metadata-proxy: Invalid URI. Make sure the URI is well formed, with a protocol and a host. ``` <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <[email protected]> Co-authored-by: IOHK <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>
Build failed |
The underlying URL builder relies on the 'MonadThrow' class to send errors. Yet, when using 'ExceptT', the error is thrown _in the base monad_!! So, using 'Either' instead in conjunction with `except` to catch the error as expected in the loopl, instead of throwing hard in the parent context.
The fallback made sense in the early days, but we can now make it a proper switch: it's either fetching from pools, or it's fetching from the registry
… the retrying plumbing.
7130352
to
838d045
Compare
Forgot a cabal dep, oops. bors r+ |
1884: SMASH Integration r=rvl a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #1883 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - d790880 📍 **allow different ways of building metadata URL from metadata information** The idea is the following: - We have a list of "URL builders" that will be tried in order, but only if the first builder replied with a connection failure (so, only if the server is somewhat down) - We can always have our current behavior by using the 'identityUrlBuilder'. - Then, we can augment this behavior if given a specific registry URL via the command-line options, which will add a "registryUrlBuilder" which takes predecence over the identity builder. - 48884cd 📍 **try fetching metadata from the next builder on HTTP exception** - c133609 📍 **add command-line option for enabling a pool metadata proxy** Going for a command-line option so that this can easily be turned on and off at will in case of any issue. Plus, not everyone might agree with the idea of going through a registry so hard-coding the registry URL is a no go. cardano-wallet-jormungandr uses an ad-hoc environment variable for this, but a documented option in the command-line is definitely cleaner. - e9dbf56 📍 **revise logging around pool metadata fetching to have more insight _inside_ the fetcher** So that we can clearly log fallback attempts and keep it clean. - cf00db6 📍 **Regenerate nix** - 0830589 📍 **use 'Either HttpException' as a base monad instead of 'ExceptT'** The underlying URL builder relies on the 'MonadThrow' class to send errors. Yet, when using 'ExceptT', the error is thrown _in the base monad_!! So, using 'Either' instead in conjunction with `except` to catch the error as expected in the loopl, instead of throwing hard in the parent context. - dd00d12 📍 **be more specific when interpreting responses from the aggregation server** - 28b9d57 📍 **remove fallback when a pool metadata proxy is supplied.** The fallback made sense in the early days, but we can now make it a proper switch: it's either fetching from pools, or it's fetching from the registry # Comments <!-- Additional comments or screenshots to attach if any --> - [ ]⚠️ TODO: show that it works. So far, on the shelley testnet: Some quick manual / sanity testing: ```console $ cardano-wallet-shelley serve \ --node-socket node.socket \ --testnet shelley_testnet-genesis.json \ --pool-metadata-proxy https://smash.shelley-testnet.dev.cardano.org/api/v1/metadata [...] [2020-07-08 19:57:09.29 UTC] Fetching metadata with hash c9ed7096 from https://smash.shelley-testnet.dev.cardano.orgapi/v1/metadata/c9ed70969c964284ff854d61e43c2591b67ceb7e978347cb1b50ba3ed1d3edb4 [2020-07-08 19:57:09.45 UTC] Successfully fetched metadata with hash c9ed7096: StakePoolMetadata {ticker = StakePoolTicker {unStakePoolTicker = "IOHK1"}, name = "IOHK1", description = Just "IOHK Pool", homepage = "https://iohk.io"} ``` ```console $ cardano-wallet-shelley serve \ --node-socket node.socket \ --testnet shelley_testnet-genesis.json \ --pool-metadata-proxy patate option --pool-metadata-proxy: Invalid URI. Make sure the URI is well formed, with a protocol and a host. ``` <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>
Build failed( |
838d045
to
bed67b8
Compare
bors r+ |
1884: SMASH Integration r=rvl a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> #1883 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - d790880 📍 **allow different ways of building metadata URL from metadata information** The idea is the following: - We have a list of "URL builders" that will be tried in order, but only if the first builder replied with a connection failure (so, only if the server is somewhat down) - We can always have our current behavior by using the 'identityUrlBuilder'. - Then, we can augment this behavior if given a specific registry URL via the command-line options, which will add a "registryUrlBuilder" which takes predecence over the identity builder. - 48884cd 📍 **try fetching metadata from the next builder on HTTP exception** - c133609 📍 **add command-line option for enabling a pool metadata proxy** Going for a command-line option so that this can easily be turned on and off at will in case of any issue. Plus, not everyone might agree with the idea of going through a registry so hard-coding the registry URL is a no go. cardano-wallet-jormungandr uses an ad-hoc environment variable for this, but a documented option in the command-line is definitely cleaner. - e9dbf56 📍 **revise logging around pool metadata fetching to have more insight _inside_ the fetcher** So that we can clearly log fallback attempts and keep it clean. - cf00db6 📍 **Regenerate nix** - 0830589 📍 **use 'Either HttpException' as a base monad instead of 'ExceptT'** The underlying URL builder relies on the 'MonadThrow' class to send errors. Yet, when using 'ExceptT', the error is thrown _in the base monad_!! So, using 'Either' instead in conjunction with `except` to catch the error as expected in the loopl, instead of throwing hard in the parent context. - dd00d12 📍 **be more specific when interpreting responses from the aggregation server** - 28b9d57 📍 **remove fallback when a pool metadata proxy is supplied.** The fallback made sense in the early days, but we can now make it a proper switch: it's either fetching from pools, or it's fetching from the registry # Comments <!-- Additional comments or screenshots to attach if any --> - [ ]⚠️ TODO: show that it works. So far, on the shelley testnet: Some quick manual / sanity testing: ```console $ cardano-wallet-shelley serve \ --node-socket node.socket \ --testnet shelley_testnet-genesis.json \ --pool-metadata-proxy https://smash.shelley-testnet.dev.cardano.org/api/v1/metadata [...] [2020-07-08 19:57:09.29 UTC] Fetching metadata with hash c9ed7096 from https://smash.shelley-testnet.dev.cardano.orgapi/v1/metadata/c9ed70969c964284ff854d61e43c2591b67ceb7e978347cb1b50ba3ed1d3edb4 [2020-07-08 19:57:09.45 UTC] Successfully fetched metadata with hash c9ed7096: StakePoolMetadata {ticker = StakePoolTicker {unStakePoolTicker = "IOHK1"}, name = "IOHK1", description = Just "IOHK Pool", homepage = "https://iohk.io"} ``` ```console $ cardano-wallet-shelley serve \ --node-socket node.socket \ --testnet shelley_testnet-genesis.json \ --pool-metadata-proxy patate option --pool-metadata-proxy: Invalid URI. Make sure the URI is well formed, with a protocol and a host. ``` <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Assign the PR to a corresponding milestone ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: KtorZ <[email protected]> Co-authored-by: Rodney Lorrimar <[email protected]>
Build failed |
err :: String | ||
err = | ||
"Invalid URI. Make sure it is a well-formed \ | ||
\ absolute http or https URI." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
lib/shelley/test/integration/Main.hs
Outdated
, PoolConfig {retirementEpoch = Just 3} | ||
-- This pool should retire, but not within the duration of a test run: | ||
, PoolConfig {retirementEpoch = Just 100_000} | ||
, PoolConfig {retirementEpoch = Just 1_000} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arf! I think there has been a wrong rebase / conflict resolution here! Hence the integration failure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops
bed67b8
to
b59361b
Compare
bors r+ |
Build succeeded |
Issue Number
#1883
Overview
d790880
📍 allow different ways of building metadata URL from metadata information
The idea is the following:
We have a list of "URL builders" that will be tried in order, but only
if the first builder replied with a connection failure (so, only if the
server is somewhat down)
We can always have our current behavior by using the
'identityUrlBuilder'.
Then, we can augment this behavior if given a specific registry URL
via the command-line options, which will add a "registryUrlBuilder"
which takes predecence over the identity builder.
48884cd
📍 try fetching metadata from the next builder on HTTP exception
c133609
📍 add command-line option for enabling a pool metadata proxy
Going for a command-line option so that this can easily be turned on and off at will in case of any issue. Plus, not everyone might agree with the idea of going through a registry so hard-coding the registry URL is a no go. cardano-wallet-jormungandr uses an ad-hoc environment variable for this, but a documented option in the command-line is definitely cleaner.
e9dbf56
📍 revise logging around pool metadata fetching to have more insight inside the fetcher
So that we can clearly log fallback attempts and keep it clean.
cf00db6
📍 Regenerate nix
0830589
📍 use 'Either HttpException' as a base monad instead of 'ExceptT'
The underlying URL builder relies on the 'MonadThrow' class to send errors. Yet, when using 'ExceptT', the error is thrown in the base monad!!
So, using 'Either' instead in conjunction with
except
to catch the error as expected in the loopl, instead of throwing hard in the parent context.dd00d12
📍 be more specific when interpreting responses from the aggregation server
28b9d57
📍 remove fallback when a pool metadata proxy is supplied.
The fallback made sense in the early days, but we can now make it a proper switch: it's either fetching from pools, or it's fetching from the registry
Comments
Some quick manual / sanity testing: