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.

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

fix/rabbitmq-wrong-interface #290

Merged
merged 5 commits into from
Jun 28, 2022

Conversation

iccicci
Copy link
Contributor

@iccicci iccicci commented Jun 16, 2022

Context

The package @cardano-sdk/rabbitmq doesn't respect all the cardano-js-sdk standards and has a severely wrong error handling, in details:

  • the Promise returned by TxSubmitWorker.start should resolve at the end of the init stage rather than when the TxSubmitWorker exits: ADP-1886
  • the TxSubmitWorker should retry only those errors which can be retried rather than all the errors: ADP-1874
  • the TxSubmitWorker should let know the RabbitMqTxSubmitProvider when a transaction is in mempool (or in case of error, propagate it) in order to let the Promise returned by RabbitMqTxSubmitProvider.submitTx to resolve when the transaction is in mempool or reject with the original error: ADP-1823

Proposed Solution

The package @cardano-sdk/rabbitmq have been completely refactored.

Important Changes Introduced

  • The required changes to implement the new @cardano-sdk/rabbitmq interface was applied to @cardano-sdk/cardano-services.
  • The test mock in @cardano-sdk/ogmios was enriched to respond with an error which can be retried.
  • Now all the time measured by the @cardano-sdk/cardano-services load test are meaningful.

@iccicci iccicci changed the title Fix/adp 1874 tx submit worker error handling fix/rabbitmq-wrong-interface Jun 16, 2022
packages/rabbitmq/src/TxSubmitWorker.ts Show resolved Hide resolved
packages/rabbitmq/src/errorsSerialization.ts Outdated Show resolved Hide resolved
packages/rabbitmq/src/rabbitmqTxSubmitProvider.ts Outdated Show resolved Hide resolved
packages/rabbitmq/src/utils.ts Outdated Show resolved Hide resolved
@iccicci iccicci force-pushed the fix/ADP-1874-tx-submit-worker-error-handling branch from 4a9de5b to d31d060 Compare June 16, 2022 16:45
packages/rabbitmq/src/utils.ts Outdated Show resolved Hide resolved
packages/rabbitmq/src/utils.ts Outdated Show resolved Hide resolved
packages/rabbitmq/src/utils.ts Outdated Show resolved Hide resolved
packages/rabbitmq/src/utils.ts Outdated Show resolved Hide resolved
@iccicci iccicci requested a review from mkazlauskas June 17, 2022 10:11
@iccicci iccicci force-pushed the fix/ADP-1874-tx-submit-worker-error-handling branch from fa40887 to 8b785c0 Compare June 20, 2022 12:37
Copy link
Member

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

This is looking very nice @iccicci 🚀
Just a couple of minor issues

packages/rabbitmq/src/TxSubmitWorker.ts Outdated Show resolved Hide resolved
@iccicci iccicci force-pushed the fix/ADP-1874-tx-submit-worker-error-handling branch 2 times, most recently from 897d266 to c9bde11 Compare June 23, 2022 14:46
mkazlauskas
mkazlauskas previously approved these changes Jun 23, 2022
Copy link
Member

@mkazlauskas mkazlauskas left a comment

Choose a reason for hiding this comment

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

Nice work @iccicci!

@rhyslbw do we want a QA review on this?

packages/rabbitmq/test/rabbitmqTxSubmitProvider.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@rhyslbw rhyslbw left a comment

Choose a reason for hiding this comment

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

Fantastic work @iccicci 🎉
Please autosquash and request final review from @ssong-iohk

@rhyslbw rhyslbw added the fix label Jun 28, 2022
@rhyslbw rhyslbw force-pushed the fix/ADP-1874-tx-submit-worker-error-handling branch from febf84e to ca16019 Compare June 28, 2022 03:54
@rhyslbw rhyslbw force-pushed the fix/ADP-1874-tx-submit-worker-error-handling branch from ca16019 to a880367 Compare June 28, 2022 03:57
@rhyslbw rhyslbw merged commit 3288649 into master Jun 28, 2022
@rhyslbw rhyslbw deleted the fix/ADP-1874-tx-submit-worker-error-handling branch June 28, 2022 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants