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 ecoBridge infinite loop #1628

Merged
merged 8 commits into from
Jan 5, 2023
Merged

Conversation

0xVance
Copy link
Contributor

@0xVance 0xVance commented Dec 19, 2022

Summary

Fixes #1626

I think _activeBridge check was introduced to improve logging when error happends but it occured to be the error 🐛

@netlify
Copy link

netlify bot commented Dec 19, 2022

Deploy Preview for swapr ready!

Name Link
🔨 Latest commit a97727b
🔍 Latest deploy log https://app.netlify.com/sites/swapr/deploys/63ad79e77deb150008ce1bcf
😎 Deploy Preview https://deploy-preview-1628--swapr.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot temporarily deployed to 1626-fix-ecobridge-infinite-loop December 19, 2022 01:20 Destroyed
@MilanVojnovic95
Copy link
Collaborator

MilanVojnovic95 commented Dec 19, 2022

Tested and:

USDC from Ethereum to Arbitrum (Socket) ✅
USDC from Ethereum to Gnosis (Connext) ✅
USDC from Arbitrum One to Gnosis (Socket) ✅

USDC from Gnosis to Polygon ❌

On Swapr it shows as if the bridge failed:
image

But on socketscan it shows that it's still in progress:
image

USDC from Ethereum to Gnosis (OmniBridge) ❌

When user clicks on the "Collect" option, they receive the following error message:
image

Also another thing I noticed:

the status "failed" does not change, even when you go to check the transaction id status "bridge in progress" and even when "Bridge successful" on Swapr, it appears in the status "failed".

@0xVenky
Copy link
Member

0xVenky commented Dec 21, 2022

image

Hey, this needs a fix for this PR. I think this is a different issue, but still, lets fix everything at once.

The other one related to Omnibridge, lets ignore that for now and create a seperate issue for this one and will try to fix it in the coming iterations.

@berteotti
Copy link
Collaborator

I made a transaction from Gnosis to Ethereum (Connext) and couldn't claim it on Ethereum. Don't know if this is fixed in this PR :)

@0xVenky
Copy link
Member

0xVenky commented Dec 29, 2022

I made a transaction from Gnosis to Ethereum (Connext) and couldn't claim it on Ethereum. Don't know if this is fixed in this PR :)

Hey, This PR should have fixed it. :)

@github-actions github-actions bot temporarily deployed to 1626-fix-ecobridge-infinite-loop December 29, 2022 11:37 Destroyed
@adamazad adamazad added this to the Swapr Beta 18 milestone Jan 3, 2023
Copy link
Collaborator

@Mi-Lan Mi-Lan left a comment

Choose a reason for hiding this comment

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

  • This fixes it! Although I get the same error message Milan outlined that gas estimation failed

Screenshot 2023-01-03 at 15 47 54

  • We can tackle that in another issue though
  • Im also getting infinite loading when waiting for confirmation for Omni bridge

@github-actions github-actions bot temporarily deployed to 1626-fix-ecobridge-infinite-loop January 4, 2023 07:41 Destroyed
@0xVance
Copy link
Contributor Author

0xVance commented Jan 4, 2023

Tested and:

USDC from Ethereum to Arbitrum (Socket) white_check_mark USDC from Ethereum to Gnosis (Connext) white_check_mark USDC from Arbitrum One to Gnosis (Socket) white_check_mark

USDC from Gnosis to Polygon x

On Swapr it shows as if the bridge failed: image

But on socketscan it shows that it's still in progress: image

USDC from Ethereum to Gnosis (OmniBridge) x

When user clicks on the "Collect" option, they receive the following error message: image

Also another thing I noticed:

the status "failed" does not change, even when you go to check the transaction id status "bridge in progress" and even when "Bridge successful" on Swapr, it appears in the status "failed".

Imo Socket has a bug on /bridge-status endpoint.

When I bridged XDAI to USDC from Gnosis to Polygon I got this:
image
image

The reason why status becomes 'Failed' is because /bridge-status endpoint returns 500 if called right after initiating bridging (which indicates some kind of bug). Few seconds later same call returns correct result.

My suggestion for fixing this:

  • in pending tx listener for socket, when it responds with 500 don't change status to failed but just skip it for next 10s
  • on app init go through all socket transactions with 'failed' status and retry them

I'll add a commit with this briefly

@0xVance
Copy link
Contributor Author

0xVance commented Jan 5, 2023

  • This fixes it! Although I get the same error message Milan outlined that gas estimation failed
Screenshot 2023-01-03 at 15 47 54
  • We can tackle that in another issue though
  • Im also getting infinite loading when waiting for confirmation for Omni bridge

Regarding Connext claiming - everything works fine now EXCEPT:

  • our Connext bridge implementation is incomplete - every bridging that is made through it has expiry date. If user doesn't claim till then bridging goes to 'Cancelled' state on Connext and and hell knows how to get funds back (maybe directly through contract interaction, I haven't looked yet). We don't have any information nor expiration handling which will be frustrating to the users.
    image
  • as for now I have no good fix for this problem as it would require from me doing some research around Connext ecosystem and figuring out how to handle cancelled bridgings.

@github-actions github-actions bot temporarily deployed to 1626-fix-ecobridge-infinite-loop January 5, 2023 01:02 Destroyed
Copy link
Contributor

@adamazad adamazad left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@@ -44,6 +44,13 @@ const createSelectPendingTransactions = (selectOwnedTxs: ReturnType<typeof creat
return pendingTxs
})

const createSelectFailedTransactions = (selectOwnedTxs: ReturnType<typeof createSelectOwnedTransactions>) =>
createSelector(selectOwnedTxs, ownedTxs => {
const failedTxs = ownedTxs.filter(tx => tx.status === SocketTxStatus.ERROR && !tx.partnerTxHash)
Copy link
Contributor

@adamazad adamazad Jan 5, 2023

Choose a reason for hiding this comment

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

I could be wrong, but it should be || instead no?

@MilanVojnovic95
Copy link
Collaborator

Retested and still I get this
image

The only thing that has been successfully changed is that the "cancelled" status for successful transactions is no longer displayed now it's "Confirmed" status.
Everything else seems ok. 👍 ✅

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.

Bridge claiming function infinite loop
6 participants