-
Notifications
You must be signed in to change notification settings - Fork 1
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
Observer API backend #18
Conversation
4f5507d
to
e3b9cda
Compare
fbb88ec
to
394a078
Compare
394a078
to
90fb6a8
Compare
This is unfortunate, but we can get rid of the dependency later.
Those will be used for integration tests.
Also renamed a few functions to distinguish the two Wai Applications
> to pull latest hydra-chain-observer
Note that there is a lot of code that can be DRYed and the aggregation is generally very conservative, not updating data if matching heads are found even in presence of potentially different observation data. Added property tests for network and version not being updated to make that clear.
98e26a2
to
14fe146
Compare
-- into a hydra-chain package with smaller dependency footprint. | ||
import Hydra.Chain (OnChainTx (..)) | ||
|
||
-- XXX: Need to depend on hydra-tx:testlib for generators? |
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.
@locallycompact You moved the generators into a testlib
component. What is your opinion here?
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.
This could be deriving anyclass Generic
At the moment I don't mind anything. All options feel wrong for one reason or another, but given that two of the major dependencies of hydra both use hedgehog (cardano-ledger and cardano-api), it feels like the path of least resistance to just align there, switch to hedgehog and use named generators uniformly across all hydra projects in a sublibrary (either :gen or :testlib). Arbitrary classes, locally defined, feel great when everything is naturally generic, but when not it is inconsistent since we then need to rely on named generators.
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 meant not to ask about Arbitrary vs named generators nor even QuickCheck vs hedgehog.
My question is rather.. why do generators need to be a :testlib
? Doesn't it feel weird to depend on it from an application package?
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.
Sublibraries are just another kind of way that we have of selectively ignoring information. I think generators lend themselves well to being sublibraries since a) they typically have unique dependencies themselves distinct from the rest of the code, b) they are used generally only for testing, and noise otherwise when worrying about logic.
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 don't think we should engineer it such that we depend on a testlib from an application. We could never need to do that.
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.
So your suggestion is to move the Arbitrary ExplorerState
instance (for which we need this import here) into yet another hydra-explorer:testlib
component?
I see now why you brought up the hedgehog and moving away from Arbitrary
classes, because moving things into a testlib
here would be at odds with not wanting orphan instances (https://github.com/cardano-scaling/hydra/wiki/Coding-Standards#proposal-avoid-orphan-instances)
14fe146
to
0e00632
Compare
45a36ac
to
e138722
Compare
Extends `hydra-chain-observer` to submit observations to a `hydra-explorer` instance with the new observer API. See https://github.com/cardano-scaling/hydra-explorer/blob/observer-backend/README.md#architecture for an architectural overview. * 🔍 Adds `--explorer` option to `hydra-chain-observer` which submits observations using HTTP. This uses the `OnChainTx` type, so there are new roundtrip tests and golden files. * 🔍 Make `hydra-chain-observer` aware of its `--version` (also via embedding git revs using nix). * 🔍 Switch on `--node-socket` or `--blockfrost-project-path` in `hydra-chain-observer` instead of subcommands. This allows us to re-use more code and the help text becomes easier to understand. See hydra-chain-observer/README.md for how to invoke the binary now. * 🔍 Extracts a `common.yaml` from API definitions so we can reference it from `hydra-explorer` API specs / schemas * 🔍 Moves observation code from `hydra-node` to `hydra-tx`, this improves dependency closure for `hydra-explorer`, especially once we move `Hydra.Chain` out of `hydra-node` (e.g. by inverting the dependency of `hydra-chain-observer`?) - This requires * Adds orphan instances to `hydra-cardano-api` as needed by the `hydra-explorer`, see cardano-scaling/hydra-explorer#18 TODO: - [x] Client-side serialization tests against observer openapi --- * [x] CHANGELOG updated * [x] Documentation updated (README, no user guide) * [x] Haddocks updated * [x] No new TODOs
99e0e6d
to
edf490e
Compare
edf490e
to
1a8d54d
Compare
This was always including a tick from preview, but we might not be observing preview at all.
a2e3254
to
ea771f3
Compare
620ed87
to
35403fe
Compare
Allows for multiple
hydra-chain-observer
instances to report into onehydra-explorer
:NetworkId
andHydraVersion
to explorer state aggregation. The aggregation is quite conservative and does not update head state even though potential observations would tell different. This is how the aggregation works currently (we might want to change policy). Anyhow, added property tests to ensure this.TODO:
/tick
endpoint does not say which network the data is from. We should probably change it to a/networks
endpoint or so into which we aggregate all observed networks with their latest slots and blocks? @ffakenz what do you think?