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

Update circles-core dependency and use new pathfinder service #630

Merged
merged 14 commits into from
Apr 24, 2023

Conversation

llunaCreixent
Copy link
Member

@llunaCreixent llunaCreixent commented Mar 21, 2023

By default the pathfinder type is server. However, we can get same behavior as before by keeping the value to cli.
Note that in case of using the type server, the parameter hops will be ignored, otherwise (with type cli) the maxTransfers parameter will be ignored. Therefore, we should review the logic using the hops to optimize the transfer steps response (#597).

The retry loops are now conditioned to the pathfinder type. The retry logic for token.transfer and token.findTransitiveTransfer are not needed anymore.

Closes #631

Add an exception when txHash is null during transfer.

Closes #589

@llunaCreixent llunaCreixent requested a review from mikozet as a code owner March 21, 2023 12:16
@netlify
Copy link

netlify bot commented Mar 21, 2023

Deploy Preview for relaxed-snyder-612ce8 ready!

Name Link
🔨 Latest commit e4483b8
🔍 Latest deploy log https://app.netlify.com/sites/relaxed-snyder-612ce8/deploys/64467304e1934c0008439ca6
😎 Deploy Preview https://deploy-preview-630--relaxed-snyder-612ce8.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.

@netlify
Copy link

netlify bot commented Mar 21, 2023

Deploy Preview for preview-review-circles-garden ready!

Name Link
🔨 Latest commit e4483b8
🔍 Latest deploy log https://app.netlify.com/sites/preview-review-circles-garden/deploys/64467304b5cc430008d6c9ff
😎 Deploy Preview https://deploy-preview-630.review.circles.garden/
📱 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.

@netlify
Copy link

netlify bot commented Mar 21, 2023

Deploy Preview for preview-testing-circles-garden ready!

Name Link
🔨 Latest commit e4483b8
🔍 Latest deploy log https://app.netlify.com/sites/preview-testing-circles-garden/deploys/64467304190d5a0008a720c2
😎 Deploy Preview https://deploy-preview-630--preview-testing-circles-garden.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.

@llunaCreixent llunaCreixent requested a review from louilinn March 22, 2023 09:52
@louilinn
Copy link
Collaborator

Is this a draft PR until circles core 4.0.0 is ready?

@llunaCreixent
Copy link
Member Author

Is this a draft PR until circles core 4.0.0 is ready?

Correct

@llunaCreixent
Copy link
Member Author

@louilinn I removed the loops when using the pathfinder service, maybe we have to review it after our conversation today

@llunaCreixent llunaCreixent marked this pull request as ready for review March 28, 2023 15:27
@llunaCreixent
Copy link
Member Author

llunaCreixent commented Mar 28, 2023

I'm going to deploy this branch in preview-testing @mikozet @louilinn

Copy link
Collaborator

@juanenrisley juanenrisley left a comment

Choose a reason for hiding this comment

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

For me, everything is okay. Let's see what @louilinn has to say about it! 💪🏿

.env.example Outdated Show resolved Hide resolved
@mikozet
Copy link
Collaborator

mikozet commented Mar 29, 2023

I connected an issue to zenhub as it states in Development tab in github

@mikozet mikozet self-requested a review March 29, 2023 10:04
.env.example Outdated Show resolved Hide resolved
mikozet
mikozet previously approved these changes Mar 29, 2023
mikozet
mikozet previously approved these changes Mar 30, 2023
@louilinn louilinn changed the title Update circles-core dependency Update circles-core dependency and use new pathfinder service Apr 3, 2023
@louilinn
Copy link
Collaborator

Can we convert this PR back to Draft as it is not ready or in a state to be merged?

I would appreciate if we squash miko's commits (using git rebase --interactive) here into one commit before merging this PR (not before tonight but when you are not all working on the branch at once). This is to have a reference commit to know what changed in myxo. Long term we have to make this fix in the core. Miko is creating an issue for it now.

@louilinn
Copy link
Collaborator

Also please note that all changes to error messages will have to be OK:d by @triaslucia before merging.

For reference Lucia, this is the code branch that is in preview-testing and will be tested by selected business partners in a meeting tonight.
I suggest that we make a comment in this PR explicitely stating our suggested changes of the transaction errors in a before/after manner so she can easily review.
(No action necessary from your side @triaslucia until we tag you again)

@louilinn
Copy link
Collaborator

Related: CirclesUBI/circles-core#188

@triaslucia
Copy link

I am not sure if I can reproduce all scenarios to se the error msg. Is there not documentation of them as text?

I only see one msg, in all cases:
Screenshot 2023-04-20 at 15 17 47

We should correct the text on the hoover of the spinning wheel:

Screenshot from 2023-04-13 11-25-19

We are tracing the trust connections between both wallets, in order to calculate your maximum transfer limit.

locales/en.json Outdated Show resolved Hide resolved
@JacqueGM
Copy link
Contributor

JacqueGM commented Apr 20, 2023

@triaslucia - is your last comment something to be fixed before merging or can we merge opening an issue about it?

@llunaCreixent
Copy link
Member Author

Can we convert this PR back to Draft as it is not ready or in a state to be merged?

I would appreciate if we squash miko's commits (using git rebase --interactive) here into one commit before merging this PR (not before tonight but when you are not all working on the branch at once). This is to have a reference commit to know what changed in myxo. Long term we have to make this fix in the core. Miko is creating an issue for it now.

I think miko's work should have been added in a new PR. Anyway, can you please @mikozet add the description of what is done into the description of this PR?

@llunaCreixent
Copy link
Member Author

Can we convert this PR back to Draft as it is not ready or in a state to be merged?

I would appreciate if we squash miko's commits (using git rebase --interactive) here into one commit before merging this PR (not before tonight but when you are not all working on the branch at once). This is to have a reference commit to know what changed in myxo. Long term we have to make this fix in the core. Miko is creating an issue for it now.

Which commit do you want to squash?

@llunaCreixent llunaCreixent marked this pull request as draft April 21, 2023 10:53
@mikozet
Copy link
Collaborator

mikozet commented Apr 24, 2023

I think miko's work should have been added in a new PR. Anyway, can you please @mikozet add the description of what is done into the description of this PR?

"Add an exception when txHash is null" - added to description

@louilinn
Copy link
Collaborator

So this is no longer a draft or?

@louilinn louilinn force-pushed the pathfinder-service branch from f7a0474 to 66da0f3 Compare April 24, 2023 12:04
@louilinn louilinn force-pushed the pathfinder-service branch from 66da0f3 to e4483b8 Compare April 24, 2023 12:16
@JacqueGM JacqueGM marked this pull request as ready for review April 24, 2023 12:29
@louilinn
Copy link
Collaborator

I am not sure if I can reproduce all scenarios to se the error msg. Is there not documentation of them as text?

I only see one msg, in all cases: Screenshot 2023-04-20 at 15 17 47

We should correct the text on the hoover of the spinning wheel:

Screenshot from 2023-04-13 11-25-19

We are tracing the trust connections between both wallets, in order to calculate your maximum transfer limit.

this is part of the send confirm view that I'm still waiting for, we will fix that then

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.

Use pathfinder2 service Transaction of high amount are being reverted
6 participants