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: Add channel opening fee to *DepositAddressReady Events #4609

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

marcellorigotti
Copy link
Contributor

Pull Request

Closes: PRO-1233

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

Added channel_opening_fee field to the events:

  • SwapDepositAddressReady
  • LiquidityDepositAddressReady

Breaking changes

@@ -51,7 +51,8 @@ pub trait Chainflip: frame_system::Config {
+ FixedPointOperand
+ MaybeSerializeDeserialize
+ Bounded
+ Sum<Self::Amount>;
+ Sum<Self::Amount>
+ Into<u128>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it ok if I add this trait bound here?
I find this the easiest way to accomplish the conversion, but maybe there is some other way around without having to change this.

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 the way it's done elsewhere is to add and Amount associated type to the (DepositApi) trait then it's possible to express in the type constraints that the types are equal (ie. in the pallet config would be DepositApi<Amount = T::Amount>.

Copy link

codecov bot commented Mar 5, 2024

Codecov Report

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

Project coverage is 72%. Comparing base (b25e4b0) to head (6d3aa36).
Report is 5 commits behind head on main.

Files Patch % Lines
api/lib/src/lib.rs 0% 2 Missing ⚠️
state-chain/runtime/src/chainflip.rs 50% 2 Missing ⚠️
state-chain/pallets/cf-lp/src/lib.rs 67% 1 Missing ⚠️
state-chain/pallets/cf-swapping/src/lib.rs 67% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main   #4609    +/-   ##
======================================
- Coverage     72%     72%    -0%     
======================================
  Files        401     402     +1     
  Lines      66787   66943   +156     
  Branches   66787   66943   +156     
======================================
+ Hits       48412   48464    +52     
- Misses     16026   16137   +111     
+ Partials    2349    2342     -7     

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

Comment on lines 562 to 564
ChannelOpeningFeePaid {
fee: T::Amount,
},
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 it's worth keeping the event here too.

@@ -51,7 +51,8 @@ pub trait Chainflip: frame_system::Config {
+ FixedPointOperand
+ MaybeSerializeDeserialize
+ Bounded
+ Sum<Self::Amount>;
+ Sum<Self::Amount>
+ Into<u128>;
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 the way it's done elsewhere is to add and Amount associated type to the (DepositApi) trait then it's possible to express in the type constraints that the types are equal (ie. in the pallet config would be DepositApi<Amount = T::Amount>.

@dandanlen
Copy link
Collaborator

I can fix the above and get this merged. Keen for it to be included in v1.3.

@@ -65,7 +65,7 @@ impl<C: Chain, T: Chainflip> MockDepositHandler<C, T> {
),
ForeignChain::Bitcoin => todo!("Bitcoin address"),
},
1u128,
Default::default(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is change in behaviour - If we expected this to be one 1 anywhere, we could use One::one .

@dandanlen dandanlen merged commit 4540356 into main Mar 6, 2024
43 checks passed
@dandanlen dandanlen deleted the feature/pro-1233 branch March 6, 2024 14:06
syan095 added a commit that referenced this pull request Mar 7, 2024
…ncoding

* origin/main: (21 commits)
  chore: update chainspecs (#4615)
  feat: Add channel opening fee to *DepositAddressReady Events (#4609)
  refactor: pass out CFE incompatibility exit information to main (#4563)
  feat: Introduce tx fee multiplier storage item (#4594)
  fix: debug cli  (#4605)
  fix: patch 1.2 broker test (#4607)
  feat: add block height and deposit details to PrewitnessedDeposit (#4606)
  chore: add myself as codeowner to upgrade-test (#4603)
  fix: RUSTSEC-2024-0019 (#4604)
  fix: upgrade test pnpm install from commit (#4600)
  chore: add notification on failed release build (#4589)
  chore: add Zellic audit to Security.md (#4599)
  update changelog for 1.2.1 (#4597)
  fix: use correct pnpm deps for upgrade-test (#4596)
  chore(bouncer): Use stable sdk version (#4595)
  chore(bouncer): Update sdk to 1.2.1 (#4582)
  log meaningful message if docker login fails (#4553)
  Added bouncer command to download a runtime update from a proposal (#4592)
  chore: add asset to withdrawal event (#4590)
  ingress-egress-tracker: add tx_ref to redis (#4573)
  ...

# Conflicts:
#	state-chain/runtime/src/lib.rs
syan095 added a commit that referenced this pull request Mar 10, 2024
…utxo

* origin/main:
  fix "get_proposed_runtime" command (#4624)
  fix: allow OldAsset to support unambiguously encoding Arb USDC and Eth USDC, while maintaining backcompat (PRO-1237) (#4614)
  fix: start localnet using correct commit (#4623)
  chore: migration housekeeping (#4612)
  feat: add arbitrum support (PRO-1154) (#4486)
  fix: swap subscription amounts as hex (#4611)
  feat: use swapping queue when swapping network fee for burn (#4584)
  chore: update chainspecs (#4615)
  feat: Add channel opening fee to *DepositAddressReady Events (#4609)
  refactor: pass out CFE incompatibility exit information to main (#4563)
  feat: Introduce tx fee multiplier storage item (#4594)
  fix: debug cli  (#4605)
  fix: patch 1.2 broker test (#4607)
  feat: add block height and deposit details to PrewitnessedDeposit (#4606)
  chore: add myself as codeowner to upgrade-test (#4603)
  fix: RUSTSEC-2024-0019 (#4604)

# Conflicts:
#	state-chain/pallets/cf-environment/src/lib.rs
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.

2 participants