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 command for broker fee withdrawal #4581

Merged
merged 8 commits into from
Feb 29, 2024
Merged

Conversation

marcellorigotti
Copy link
Contributor

Pull Request

Closes: PRO-1219

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

Expose command for broker fee withdrawal

Non-Breaking changes

Copy link

codecov bot commented Feb 28, 2024

Codecov Report

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

Project coverage is 72%. Comparing base (c3393b8) to head (e89f3f4).

Files Patch % Lines
api/lib/src/lib.rs 0% 49 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #4581   +/-   ##
=====================================
- Coverage     73%     72%   -0%     
=====================================
  Files        401     401           
  Lines      66745   66794   +49     
  Branches   66745   66794   +49     
=====================================
+ Hits       48415   48416    +1     
- Misses     15982   16032   +50     
+ Partials    2348    2346    -2     

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

@dandanlen dandanlen self-requested a review February 28, 2024 13:59
@kylezs kylezs changed the title feat: expose command fro broker fee withdrawal feat: expose command for broker fee withdrawal Feb 28, 2024
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.

Thanks this is looking good, just a few changes.

I think naming-wise, calling it just WithdrawFees is better, instead of WithdrawFeeAsset.

clean_foreign_chain_address(asset.into(), &destination_address)?,
)
.await
.map(|tx_hash| format!("{tx_hash:#x}"))?)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As discussed, better to return the hash here and rely on Serialize rather than converting to a string explicitly.

}))
.await
.until_in_block()
.await
Copy link
Collaborator

Choose a reason for hiding this comment

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

At this point we can also get the egress details, we should return these too.

Comment on lines 381 to 383
egress_amount: _,
egress_fee: _,
destination_address: _,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not return all of the information? All of this is interesting to the broker.

Comment on lines 89 to 93
println!(
"Withdrawal request successfull submitted. Tx hash: {:#x}",
withdraw_details.tx_hash
);
println!("Egress id: {:?}", withdraw_details.egress_id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
println!(
"Withdrawal request successfull submitted. Tx hash: {:#x}",
withdraw_details.tx_hash
);
println!("Egress id: {:?}", withdraw_details.egress_id);
println!(
"Withdrawal request successfully submitted: {}",
withdraw_details
);

(and implement Display for WithdrawalDetails.

} else {
bail!("No WithdrawalRequested event was found");
}
// Ok(tx_hash)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: stale comment

@marcellorigotti
Copy link
Contributor Author

marcellorigotti commented Feb 29, 2024

I left the impl of Deserialize for EncodedAddress as unimplemented, is that ok?
I needed to implement it to satisfy the trait bounds but we never really use it if I am correct

where
D: serde::Deserializer<'de>,
{
unimplemented!()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rather we didn't have this, seems a bit dangerous.

The implementation is just clean_foreign_chain_address from the api crate, but refactoring and moving all this is beyond the scope of this PR.

I'll make the change and then we can merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

@dandanlen
Copy link
Collaborator

Thanks @marcellorigotti

@dandanlen dandanlen enabled auto-merge (squash) February 29, 2024 16:38
@dandanlen dandanlen merged commit f05ac0f into main Feb 29, 2024
43 checks passed
@dandanlen dandanlen deleted the feature/pro-1219 branch February 29, 2024 17:10
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)
syan095 added a commit that referenced this pull request Feb 29, 2024
…ero-liquidity

* 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)

# Conflicts:
#	Cargo.lock
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