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

feat: more information added to AllBatchError. (PRO-1171) #4535

Merged
merged 5 commits into from
Feb 16, 2024

Conversation

syan095
Copy link
Contributor

@syan095 syan095 commented Feb 15, 2024

Pull Request

Closes: PRO-1171

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

When AllBatch build fails, an event is emitted that contains the error, this way both the pallet and the error details is visible.

When AllBatch build fails, an event is emitted that contains the error,
this way both the pallet and the error details is visible.
@syan095 syan095 requested a review from dandanlen as a code owner February 15, 2024 01:18
@syan095 syan095 requested a review from kylezs February 15, 2024 01:18
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (e0983e2) 73% compared to head (37a8998) 73%.
Report is 3 commits behind head on main.

Files Patch % Lines
state-chain/chains/src/lib.rs 13% 12 Missing and 1 partial ⚠️
state-chain/chains/src/btc/api.rs 0% 0 Missing and 2 partials ⚠️
state-chain/chains/src/dot/api.rs 0% 1 Missing ⚠️
state-chain/chains/src/eth/api.rs 50% 1 Missing ⚠️
state-chain/pallets/cf-ingress-egress/src/lib.rs 86% 1 Missing ⚠️
state-chain/traits/src/mocks/api_call.rs 50% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #4535    +/-   ##
======================================
- Coverage     73%     73%    -0%     
======================================
  Files        401     401            
  Lines      68357   68184   -173     
  Branches   68357   68184   -173     
======================================
- Hits       49832   49585   -247     
- Misses     15895   15973    +78     
+ Partials    2630    2626     -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

state-chain/chains/src/btc/api.rs Outdated Show resolved Hide resolved
state-chain/chains/src/lib.rs Outdated Show resolved Hide resolved
state-chain/chains/src/lib.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 1496 to 1497
crate::Event::<Test>::FailedToBuildAllBatchCall {
error: cf_chains::AllBatchError::Other,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this test passes because calling do_egress_scheduled_fetch_transfer outside of a transactional wrapper doesn't roll back the storage.

A better approach might be to refactor do_egress_scheduled_fetch_transfer so that it returns a Result and simply decorate the fn with #[transactional].

…igned-error

* origin/main:
  chore: fetch solana state from docker (#4538)
  feat: charge a fee for opening swap deposit addresses (#4512)
  refactor: Remove RpcAsset (PRO-1187) (#4491)
  fix: re-add version update (#4533)
  fix: pull first (#4526)
  feat: auto pick non-breaking changes (#4498)

# Conflicts:
#	state-chain/pallets/cf-ingress-egress/src/lib.rs
#	state-chain/pallets/cf-ingress-egress/src/tests.rs
@syan095 syan095 requested a review from dandanlen February 16, 2024 07:21
Copy link
Collaborator

@dandanlen dandanlen left a comment

Choose a reason for hiding this comment

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

I'll fix these and then merge. Thanks @syan095

state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-ingress-egress/src/lib.rs Outdated Show resolved Hide resolved
@dandanlen dandanlen changed the title Added more errors to AllBatchError so more information is provided. feat: more information added to AllBatchError. (PRO-1171) Feb 16, 2024
@dandanlen dandanlen enabled auto-merge (squash) February 16, 2024 11:52
@dandanlen dandanlen merged commit acd6ac4 into main Feb 16, 2024
43 checks passed
@dandanlen dandanlen deleted the chore/improve-new-unsigned-error branch February 16, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants