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(protocol): Change require to custom err in bridge contracts #13220

Merged
merged 7 commits into from
Feb 28, 2023

Conversation

adaki2004
Copy link
Contributor

As per issue:
#13057

Bridge related require messages are changed in code + unit tests.

Created a gas usage comparison between current version vs. new. Please see differences in yellow (cheaper) or purple (expensive). Left is the new, right is the old.
Képernyőfotó 2023-02-27 - 9 35 07

It might only be slightly more expensive because only called 4 times.

davidtaikocha
davidtaikocha previously approved these changes Feb 27, 2023
@cyberhorsey
Copy link
Contributor

LGTM just needs merge conflict resolved!

@adaki2004
Copy link
Contributor Author

LGTM just needs merge conflict resolved!

Thanks Jeff, resolved.

davidtaikocha
davidtaikocha previously approved these changes Feb 27, 2023
@adaki2004
Copy link
Contributor Author

Integration tests has to be adjusted.

@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #13220 (5b855c0) into main (72f9c1d) will increase coverage by 0.11%.
The diff coverage is 55.88%.

❗ Current head 5b855c0 differs from pull request most recent head 1238e4a. Consider uploading reports for the commit 1238e4a to get more accurate results

@@            Coverage Diff             @@
##             main   #13220      +/-   ##
==========================================
+ Coverage   60.72%   60.84%   +0.11%     
==========================================
  Files         116      116              
  Lines        3524     3555      +31     
  Branches      483      483              
==========================================
+ Hits         2140     2163      +23     
- Misses       1300     1307       +7     
- Partials       84       85       +1     
Flag Coverage Δ *Carryforward flag
bridge-ui 93.80% <ø> (ø) Carriedforward from 72f9c1d
protocol 51.40% <55.88%> (+0.38%) ⬆️
relayer 65.15% <ø> (ø) Carriedforward from 72f9c1d
ui 100.00% <ø> (ø) Carriedforward from 72f9c1d

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
packages/protocol/contracts/bridge/Bridge.sol 41.17% <0.00%> (+2.28%) ⬆️
...rotocol/contracts/bridge/libs/LibBridgeRelease.sol 0.00% <0.00%> (ø)
...protocol/contracts/bridge/libs/LibBridgeStatus.sol 55.55% <0.00%> (-4.45%) ⬇️
...ckages/protocol/contracts/signal/SignalService.sol 53.57% <25.00%> (-14.62%) ⬇️
packages/protocol/contracts/bridge/EtherVault.sol 75.00% <75.00%> (-1.20%) ⬇️
...rotocol/contracts/bridge/libs/LibBridgeProcess.sol 21.62% <75.00%> (+3.97%) ⬆️
...s/protocol/contracts/bridge/libs/LibBridgeSend.sol 78.57% <87.50%> (-0.60%) ⬇️
...ackages/protocol/contracts/bridge/BridgedERC20.sol 96.77% <100.00%> (+1.12%) ⬆️
...protocol/contracts/bridge/libs/LibBridgeInvoke.sol 100.00% <100.00%> (ø)
.../protocol/contracts/bridge/libs/LibBridgeRetry.sol 94.73% <100.00%> (+0.29%) ⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cyberhorsey
cyberhorsey previously approved these changes Feb 27, 2023
davidtaikocha
davidtaikocha previously approved these changes Feb 28, 2023
@dantaik dantaik added this pull request to the merge queue Feb 28, 2023
Merged via the queue into main with commit 6e8cb82 Feb 28, 2023
@dantaik dantaik deleted the custom_errors_in_brige_contracts branch February 28, 2023 01:17
@gitpoap-bot
Copy link

gitpoap-bot bot commented Feb 28, 2023

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 Taiko Contributor:

GitPOAP: 2023 Taiko Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

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.

None yet

4 participants