chainflip-io / chainflip-backend

The Chainflip backend repo, including the Chainflip Node and CFE.

48 stars 15 forks source link

Feature: Added EVM transfer/fetch limit #5237

Closed Janislav closed 1 week ago

Janislav commented 1 week ago

Pull Request

Closes: PRO-1619

Checklist

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

[x] I am confident that the code works.
[x] I have written sufficient tests.
[x] I have written and tested required migrations.
[x] I have updated documentation where appropriate.

Summary

Please include a succinct description of the purpose and content of the PR. What problem does it solve, and how? Link issues, discussions, other PRs, and anything else that will help the reviewer.

Non-Breaking changes

If this PR includes non-breaking changes, select the non-breaking label. On merge, CI will automatically cherry-pick the commit to a PR against the release branch.

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 93.57143% with 9 lines in your changes missing coverage. Please review.

Project coverage is 70%. Comparing base (f14faca) to head (2b02fcf). Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
state-chain/pallets/cf-ingress-egress/src/tests.rs	92%	3 Missing and 6 partials :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #5237 +/- ## ===================================== - Coverage 70% 70% -0% ===================================== Files 485 486 +1 Lines 86902 86898 -4 Branches 86902 86898 -4 ===================================== - Hits 61158 61093 -65 - Misses 22452 22506 +54 - Partials 3292 3299 +7 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Janislav commented 1 week ago

After a discussion with @albert-llimos we came up with the added values for ETH which are:

transfer_limit -> 50
fetches_limit -> 20
ccm_limit -> None

Unit/integration tests as well as bouncer is working. Any other ideas how we can check for any bad surprises @dandanlen ? I assume another level of verification is when it is out on a test net.

Janislav commented 1 week ago

I don't think it is tested at all at the moment. Also not for other chains. I think it makes sense then to write unit tests in the ingress/egress pallet against a mock implementation just to test the basic behaviour.

albert-llimos commented 1 week ago

LGTM

Janislav commented 1 week ago

Converted to draft for now to prevent merging. Wanted to add some final touches on the testing.

Janislav commented 1 week ago

@dandanlen are you happy with the added testing?

kylezs commented 1 week ago

Seems to have been a case of: https://linear.app/chainflip/issue/PRO-1627/fix-rpc-unavailable-upgrade-test-flakiness