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

fix(bridge-ui): fix claim eth message processed #13656

Merged

Conversation

jscriptcoder
Copy link
Contributor

Fixed message already processed when claiming ETH

@jscriptcoder jscriptcoder changed the base branch from main to major_protocol_upgrade_alpha3 April 26, 2023 08:49
@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #13656 (cc8278e) into major_protocol_upgrade_alpha3 (eee153c) will increase coverage by 0.05%.
The diff coverage is 50.00%.

@@                        Coverage Diff                        @@
##           major_protocol_upgrade_alpha3   #13656      +/-   ##
=================================================================
+ Coverage                          44.82%   44.88%   +0.05%     
=================================================================
  Files                                132      132              
  Lines                               3527     3529       +2     
  Branches                             361      362       +1     
=================================================================
+ Hits                                1581     1584       +3     
+ Misses                              1845     1844       -1     
  Partials                             101      101              
Flag Coverage Δ *Carryforward flag
bridge-ui 95.61% <50.00%> (-0.20%) ⬇️
eventindexer 82.08% <ø> (ø) Carriedforward from 85374b5
protocol 0.00% <ø> (ø) Carriedforward from 85374b5
relayer 62.55% <ø> (+0.13%) ⬆️ Carriedforward from 85374b5
ui 100.00% <ø> (ø) Carriedforward from 85374b5

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

Impacted Files Coverage Δ
packages/bridge-ui/src/bridge/ERC20Bridge.ts 88.88% <ø> (ø)
packages/bridge-ui/src/bridge/ETHBridge.ts 87.32% <50.00%> (-1.09%) ⬇️

... and 1 file with indirect coverage changes

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

@@ -100,7 +100,7 @@ export class ETHBridge implements Bridge {
opts.msgHash,
);

if (messageStatus === MessageStatus.Done) {
if ([MessageStatus.Done, MessageStatus.Failed].includes(messageStatus)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If not mistaken when MessageStatus.Failed is the status, then it is not processed yet. There need to be a 'releaseEther()'

Copy link
Contributor

Choose a reason for hiding this comment

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

If Claimed called after releaseEther it's fine i guess - donno when / how automatized it is on the relayer side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should then throw a different error message here?, In the UI when the status is Failed we show the Release button, claiming will no longer be available, so theoretically the user shouldn't be able to call this function when messageStatus is Failed... Let me make that change

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, yes, so basically its kinda not possible for regular users, so then has not too much effect anyways.

Copy link
Contributor Author

@jscriptcoder jscriptcoder Apr 26, 2023

Choose a reason for hiding this comment

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

I made a change to throw different error when the message has failed. One could argue though that the message has failed and therefore already processed, but I might be wrong here. @cyberhorsey, what do you think?

In V2 I'd like to use error codes instead of messages

@jscriptcoder jscriptcoder self-assigned this Apr 26, 2023
@cyberhorsey cyberhorsey merged commit 57f06cb into major_protocol_upgrade_alpha3 Apr 26, 2023
@cyberhorsey cyberhorsey deleted the fix_claim_eth_message_processed branch April 26, 2023 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants