-
Notifications
You must be signed in to change notification settings - Fork 25
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
Introduce Hydra Provider #230
Conversation
🦋 Changeset detectedLatest commit: 718ed7b The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Cool stuff.
We should later explore using a parsing library on the message events so the unknown type casting is safe. We can ignore that for this PR because we haven't done it elsewhere in blaze. |
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.
Thanks for contributing this here. We should make a mental note that the typescript interfaces here would be a good starting point for a hydra-api typescript package to be shared across web-based hydra clients.
return true; | ||
} | ||
} | ||
} |
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.
Instead of polling /snapshot/utxo
you could be waiting for a SnapshotConfirmed
websocket message that contains the transaction id. IIRC it should be given for each transaction in .snapshot.confirmed[].txId
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.
The most robust way would be to keep a list of all txids that were reported confirmed through the websocket and just poll that list. That way you can't "miss" the confirmation, due to race conditions with submission.
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 is a good point, I guess I could technically miss a confirmed tx if a new snapshot confirms during polling. I will think about the best way to handle fetching UTxOs without maintaining a constantly growing list
_additionalUtxos: TransactionUnspentOutput[], | ||
): Promise<Redeemers> { | ||
throw new Error("Unsupported by the Hydra API"); | ||
} |
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 provided. Anyone, please feel free to open a request here: https://github.com/cardano-scaling/hydra/issues
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.
Really we should eliminate this from all providers once web compatible vms are stable.
txHash: string, | ||
idx: number, | ||
output: any, | ||
): TransactionUnspentOutput { |
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.
Has the TransactionUnspentOutput
used here a blaze-specific JSON encoding or can we find a common denominator between Hydra and Blaze? We long wanted to adopt a widely accepted standard (We like https://github.com/CardanoSolutions/cardanonical/blob/main/cardano.json), but have been following the format used by cardano-cli
- which likely is not the majority format!?
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.
It's inherited from https://github.com/input-output-hk/cardano-js-sdk
| DecrementTx | ||
| CloseTx | ||
| ContestTx | ||
| FanoutTx; |
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.
Not sure if it's worthwhile to properly type postChainTx
. From an API client's point of view, these are very much "internal server errors" and keeping them in an unparsed / untyped form is likely okay and breaks less in case we fix the abstraction leak.
* @param datumHash | ||
* @returns Promise<PlutusData> | ||
*/ | ||
async resolveDatum(datumHash: DatumHash): Promise<PlutusData> { |
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.
Is this the same behavior as blaze would expect from other providers? AFAIK its often full-fledged indexers that we have providers for and those would probably index datums of all transaction bodies (as auxiliary or witness).
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.
Providers are abstractly nodes with indexers (which is a design inherited from lucid). E.g ogmios alone can't be a provider (this may have changed if ogmios has updated) because it can't resolve datums, so you have Kupmios. Every major hosted service utilises indexers. I'm surprised the node doesn't support datum resolution natively.
The transaction builder fills holes so the builder only needs to provide limited data. This is a convenience for DevEx. We could change that design and make resolveDatum, others unnecessary, but I think it would be better if the node could look those up.
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.
Yeah, that is one reason my hydra provider doesn't quite line up with the concept of providers in blaze largely. However, If we want to use blaze to build and submit transactions to hydra, I do have to have a hydra provider to communicate with the node.
Perhaps we need to break up a provider into smaller pieces? @micahkendall
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.
FWIW kupo does support Hydra https://cardanosolutions.github.io/kupo/#section/Getting-started/-hydra-host-hostname-hydra-port-port-number
So we could be using kupo + transaction submission directly into Hydra?
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.
Yes
Thank you! I totally agree. This was literally just lift from the place I'm using it in another project to here, and do a bit of clean up. I find it strange that the library would manage the websocket connection at all, perhaps we can provide another service for that and pass in the connection via DI, so that people can substitute that state management whatever they want. I also hate The way the This is definitely not production ready, just a first step. |
I have been pointing some users already onto this work-in-progress. Is it realistic that we can get this into the hands of people? |
I'm gonna give it a merge but consider it experimental / unsupported for now. |
@micahkendall Thanks, that makes me use |
Sorry, I've been very busy with some other work (and trying to get some time away from the screen when I can). This is definitely a messy implementation, but happy to work with you to improve it where I can! |
This PR introduces a Hydra Provider that allows users to use blaze to seamlessly interact with an existing Hydra head. This has been adapted from some work I did for Hydra DOOM, so it is not quite perfect.
There are some anomalies from the other providers here, namely:
HydraMessageHandler
that allows users to consume events from the hydra head.Perhaps because of these differences introducing this feature as a single provider doesn't make a ton of sense. Perhaps it makes more sense to separate the HydraConnection logic from the HydraProvider logic, and pass the connection as a constructor argument to the HydraProvider.
I am very open to discussion and adjustment to make it the easiest to interact with.
TODO: