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: Expose tx_hash on BroadcastSuccess event #4561

Merged
merged 9 commits into from
Feb 27, 2024
Merged

Conversation

marcellorigotti
Copy link
Contributor

@marcellorigotti marcellorigotti commented Feb 23, 2024

Pull Request

Closes: PRO-1195

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

Updated transaction_succeeded and BroadcastSuccess Event to include the tx_ref which represent the transaction hash (or an equivalent way of identifying a transaction depending on the chain)

Breaking changes

This PR is breaking, both an extrinsic used in the engine and an event changed

@marcellorigotti marcellorigotti requested review from jerryafr and zoheb391 and removed request for a team February 23, 2024 13:29
Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 32 lines in your changes are missing coverage. Please review.

Project coverage is 72%. Comparing base (38f4443) to head (5ae425a).
Report is 8 commits behind head on main.

Files Patch % Lines
state-chain/chains/src/dot.rs 27% 6 Missing and 2 partials ⚠️
state-chain/chains/src/benchmarking_value.rs 0% 7 Missing ⚠️
state-chain/chains/src/btc.rs 68% 6 Missing and 1 partial ⚠️
engine/src/witness/dot.rs 0% 6 Missing ⚠️
engine/src/witness/btc.rs 0% 1 Missing ⚠️
engine/src/witness/eth/key_manager.rs 0% 1 Missing ⚠️
state-chain/chains/src/lib.rs 0% 1 Missing ⚠️
state-chain/pallets/cf-broadcast/src/lib.rs 50% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #4561    +/-   ##
======================================
- Coverage     73%     72%    -0%     
======================================
  Files        401     401            
  Lines      66210   66224    +14     
  Branches   66210   66224    +14     
======================================
- Hits       48089   47986   -103     
- Misses     15802   15914   +112     
- Partials    2319    2324     +5     

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

Copy link
Contributor

@kylezs kylezs left a comment

Choose a reason for hiding this comment

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

Mostly naming related - seems like @dandanlen is chatting to you about this PR too, so will pause the review here :)

engine/src/witness/dot.rs Outdated Show resolved Hide resolved
state-chain/chains/src/any.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-broadcast/src/lib.rs Outdated Show resolved Hide resolved
state-chain/pallets/cf-broadcast/src/tests.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/chains/src/dot.rs Outdated Show resolved Hide resolved
state-chain/chains/src/dot.rs Outdated Show resolved Hide resolved
engine/src/witness/btc.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kylezs kylezs left a comment

Choose a reason for hiding this comment

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

Nice stuff!

@kylezs
Copy link
Contributor

kylezs commented Feb 27, 2024

Get at least a review from product here too

Copy link
Contributor

@niklasnatter niklasnatter left a comment

Choose a reason for hiding this comment

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

looks good from my side 🙂 would be very cool if we could include it in the ingress-egress-tracker next!

@kylezs
Copy link
Contributor

kylezs commented Feb 27, 2024

looks good from my side 🙂 would be very cool if we could include it in the ingress-egress-tracker next!

It should be included for free there - but I'm not sure how you guys are getting and writing the data now with the redis addition

@marcellorigotti
Copy link
Contributor Author

I'll close this PR and start working on the ingress-egress-tracker on a new one

@marcellorigotti marcellorigotti merged commit 4298db7 into main Feb 27, 2024
43 checks passed
@marcellorigotti marcellorigotti deleted the feature/pro-1195 branch February 27, 2024 11:12
syan095 added a commit that referenced this pull request Feb 28, 2024
…ero-liquidity

* origin/main:
  fix: disable try-state checks (#4576)
  chore: debug solana in CI 🐛 (#4580)
  refactor: pass tx_ref as an extrinsic parameter (#4579)
  fix: remove bounded balance check (#4575)
  Solana: update image to latest tag (#4574)
  feat: add boost lp account to bouncer and fund it on setup_swaps (#4552)
  feat: Expose tx_hash on BroadcastSuccess event (#4561)
  feat: Relative Slippage Limits (PRO-1207) (#4547)
  chore: disable localnet solana in CI ⏱️ (#4569)
  feat: store prewitnessed deposits with id (#4496)
  Feat: Scheduled Swaps Subscription (#4525)

# Conflicts:
#	state-chain/pallets/cf-pools/src/tests.rs
syan095 added a commit that referenced this pull request Feb 29, 2024
…utxo

* origin/main:
  feat: expose command for broker fee withdrawal (#4581)
  Chore/fix arbitrum deployment (#4570)
  Added Range order pool price to the pool_price_v2 rpc call (#4548)
  fix: just check that the balance after is greater than before (#4587)
  chore: add origin to ccm failed (#4586)
  fix: runtime upgrade state check uses AllPalletsWithoutSystem (#4583)
  chore: add zellic audit to repo (#4585)
  fix: disable try-state checks (#4576)
  chore: debug solana in CI 🐛 (#4580)
  refactor: pass tx_ref as an extrinsic parameter (#4579)
  fix: remove bounded balance check (#4575)
  Solana: update image to latest tag (#4574)
  feat: add boost lp account to bouncer and fund it on setup_swaps (#4552)
  feat: Expose tx_hash on BroadcastSuccess event (#4561)
  feat: Relative Slippage Limits (PRO-1207) (#4547)
  chore: disable localnet solana in CI ⏱️ (#4569)
  feat: store prewitnessed deposits with id (#4496)
  Feat: Scheduled Swaps Subscription (#4525)
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