Switch to HTTP interface for Ogmios #1669
Conversation
marcusbfs
force-pushed
the
marcusbfs/http-ogmios
branch
from
bc3f67b to
14a6d9d
Compare
| if result ^? _Right <<< to _.status == Just (StatusCode 503) then | ||
| delay delayMs *> | ||
| ogmiosPostRequestRetryAff (Milliseconds (unwrap delayMs * 2.0)) config | ||
| body |
| resendPendingSubmitRequests | ||
| :: MkUniqueId | ||
| -> OgmiosWebSocket | ||
| -> IsTxConfirmed | ||
| -> Logger | ||
| -> (RequestBody -> Effect Unit) | ||
| -> Dispatcher | ||
| -> PendingSubmitTxRequests | ||
| -> Effect Unit | ||
| resendPendingSubmitRequests |
There was a problem hiding this comment.
This function can be removed entirely since we no longer submit transactions over WebSocket
There was a problem hiding this comment.
Removing this causes the runtime failure - likely due to Ogmios waiting for a transaction to be included in the mempool. I can try submitting these transactions over HTTP, but this might entangle the code, though.
There was a problem hiding this comment.
But since we don't submit transactions over WebSocket anymore, why do we need this code in the first place?
There was a problem hiding this comment.
Addressed in 99b98a0. Removing Ogmios' websocket initialization from the Contract initialization logic simplified the process and the runtime failure is gone.
src/Internal/Contract/Monad.purs
Outdated
| ogmiosWs <- mkOgmiosWebSocketAff uniqueId isTxConfirmed logger | ||
| (mkWsUrl ogmiosConfig) |
There was a problem hiding this comment.
There's no need to initialize a WebSocket connection to Ogmios anymore as part of the Contract initialization logic. Anyone who needs to use the Mempool API should manage the connection themselves.
There was a problem hiding this comment.
That's right. We can change this to ws :: Maybe OgmiosWebSocket and let the user handle it. Thoughts?
There was a problem hiding this comment.
Yes, that sounds reasonable.
There was a problem hiding this comment.
But I would prefer to make the core Kupmios provider entirely independent of the Mempool extension, meaning the Mempool API should reside in its own package as an optional plugin, while the core provider should not reference WebSockets in any way.
There was a problem hiding this comment.
Hopefully that makes sense, let me know what you think
There was a problem hiding this comment.
Addressed in febc66a. Now, the mempool functionality depends on Ogmios.Types and some Ctl.Internal helpers. Other than this, the module is self contained. When extracting Kupmios provider, we can refactor to decouple Kupmios and Ogmios.Mempool package.
There was a problem hiding this comment.
@marcusbfs LGTM. Please update the changelog and let's merge it.
| -- Server responded with error. | ||
| = ErrorResponse (Maybe OgmiosError) | ||
| -- Server responded with result, parsing of which failed | ||
| | ClientErrorResponse ClientError |
There was a problem hiding this comment.
ClientError is excessive here, since only one ClientDecodeJsonError constructor is actually used.
Also, it would be helpful to differentiate between a failure in decoding the RPC result and a failure in decoding the RPC error:
| | ClientErrorResponse ClientError | |
| | InvalidRpcResultResponse JsonDecodeError | |
| | InvalidRpcErrorResponse JsonDecodeError |
src/Internal/QueryM/HttpUtils.purs
Outdated
| :: forall err intermediate result | ||
| . (Affjax.Error -> err) | ||
| -- ^ Convert an Affjax error into custom error | ||
| -> (Int -> String -> err) | ||
| -- ^ Convert a non-2xx status code into custom error | ||
| -> (String -> JsonDecodeError -> err) | ||
| -- ^ Wrap aeson-parse/decode errors | ||
| -> (String -> Either JsonDecodeError intermediate) | ||
| -- ^ Parse the response body | ||
| -> (intermediate -> Either err result) | ||
| -- ^ Function from `intermediate` to `result` | ||
| -> Either Affjax.Error (Affjax.Response String) | ||
| -- ^ Argument | ||
| -> Either err result |
There was a problem hiding this comment.
This function could accept a record for better readability. Also, it might not need to be so generic, especially if we can reuse the same error type.
Closes #1575.
Ogmios still requires a WebSocket connection for mempool operations that depend on an acquired snapshot (
hasTransaction,nextTransaction,sizeOfMempool,releaseMempool). We’ve opened a discussion proposing a stateless HTTP API for mempool functionality.In the meanwhile, the plan is to extract the mempool functionality into its own package (wip).
Pre-review checklist
make format)templates/ctl-scaffold) has been updated## Unreleasedheader, using the appropriate sub-headings (### Added,### Changed,### Removed,### Fixed), and the links to the appropriate issues/PRs have been included