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(ui): apply diamond architecture in the credit page #809

Closed
wants to merge 19 commits into from

Conversation

bojan07
Copy link
Contributor

@bojan07 bojan07 commented Oct 6, 2023

Resolves #786

@ubiquibot
Copy link

ubiquibot bot commented Oct 6, 2023

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 7, 2023

Files that is applied camel case failed in the ci process
image

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 7, 2023

file names that is applied both camel case and pascal case are failing in the github ci process

Copy link
Member

@0x4007 0x4007 left a comment

Choose a reason for hiding this comment

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

You're using "!" Everywhere which should have been strictly forbidden as part of our lint settings.

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 7, 2023

fixed all again

@molecula451
Copy link
Member

please, deploy the contracts locally, provide a bit of QA to your code

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 8, 2023

please, deploy the contracts locally, provide a bit of QA to your code

What do you mean "QA"?

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 8, 2023

please, deploy the contracts locally, provide a bit of QA to your code

Warnings in browser console was generated from Uniswap widget.. not from our codebase
image

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 8, 2023

Also added some logs in usePrices component, and tested
image

@rndquu
Copy link
Member

rndquu commented Oct 9, 2023

@bojan07 Could you fix this error?

Screenshot 2023-10-09 at 11 41 02

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 9, 2023

@bojan07 Could you fix this error?

Screenshot 2023-10-09 at 11 41 02

I thought that it was generated from my network status.. so ignored
What do you think that this error generated from?

@rndquu
Copy link
Member

rndquu commented Oct 9, 2023

@bojan07 Could you fix this error?
Screenshot 2023-10-09 at 11 41 02

I thought that it was generated from my network status.. so ignored What do you think that this error generated from?

It is caused by the <UcrRedeem twapInteger={twapInteger} /> component but I haven't investigated in depth the root cause

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 9, 2023

@bojan07 Could you fix this error?
Screenshot 2023-10-09 at 11 41 02

I thought that it was generated from my network status.. so ignored What do you think that this error generated from?

It is caused by the <UcrRedeem twapInteger={twapInteger} /> component but I haven't investigated in depth the root cause

Yeah, thanks for your time to check it out

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 9, 2023

@bojan07 Could you fix this error?

Screenshot 2023-10-09 at 11 41 02

Just checked in depth. This issue was generated from Uniswap alpha router implementation.. need to check more in depth

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 9, 2023

Uniswap alpha router implementation.

It's not easy to fully understand how it works? any help for this?

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 9, 2023

Uniswap alpha router implementation.

It's not easy to fully understand how it works? any help for this?

Seems now understood the logic a bit

@rndquu
Copy link
Member

rndquu commented Oct 9, 2023

Uniswap alpha router implementation.

It's not easy to fully understand how it works? any help for this?

Seems now understood the logic a bit

Related issue Uniswap/smart-order-router#277

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 9, 2023

Uniswap alpha router implementation.

It's not easy to fully understand how it works? any help for this?

Seems now understood the logic a bit

Related issue Uniswap/smart-order-router#277

image
After updating uniswap router version to the latest, I'm getting new error.. have you met this kind of error before? I guess that it was because uniswap router only support certain networks(chains), not support local network(chainID 31337)

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 9, 2023

Uniswap alpha router implementation.

It's not easy to fully understand how it works? any help for this?

Seems now understood the logic a bit

Related issue Uniswap/smart-order-router#277

Uniswap alpha router implementation.

It's not easy to fully understand how it works? any help for this?

Seems now understood the logic a bit

Related issue Uniswap/smart-order-router#277

So, this can be fixed on our codebase? or just uniswap router package issue?

@rndquu
Copy link
Member

rndquu commented Oct 9, 2023

I guess that it was because uniswap router only support certain networks

Or some API has changed. I don't see any reasons why local networks can't be supported.

So, this can be fixed on our codebase? or just uniswap router package issue?

Hard to say, pls find the root cause of the error and fix it (if possible)

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 9, 2023

I guess that it was because uniswap router only support certain networks

Or some API has changed. I don't see any reasons why local networks can't be supported.

So, this can be fixed on our codebase? or just uniswap router package issue?

Hard to say, pls find the root cause of the error and fix it (if possible)

Oh.. already spending time on it.. feeling big issue 🤷‍♀️

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 9, 2023

This uniswap router issue wasn't generated from diamond architecture implementation.. seems original codebase related to uniswap router had some problem.. hope to handle this in a separated issue

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 9, 2023

This uniswap router issue wasn't generated from diamond architecture implementation.. seems original codebase related to uniswap router had some problem.. hope to handle this in a separated issue

@rndquu what do you think?

@rndquu
Copy link
Member

rndquu commented Oct 10, 2023

This uniswap router issue wasn't generated from diamond architecture implementation.. seems original codebase related to uniswap router had some problem.. hope to handle this in a separated issue

@rndquu what do you think?

The credits page must open without errors. Otherwise there is no sense in the current PR as we will need to fix the credits page ourselves.

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 10, 2023

This uniswap router issue wasn't generated from diamond architecture implementation.. seems original codebase related to uniswap router had some problem.. hope to handle this in a separated issue

@rndquu what do you think?

The credits page must open without errors. Otherwise there is no sense in the current PR as we will need to fix the credits page ourselves.

Hmm.. I've already checked that error. But not easy to find the root cause. Hope one who has experience in it solve this

@rndquu
Copy link
Member

rndquu commented Oct 10, 2023

This uniswap router issue wasn't generated from diamond architecture implementation.. seems original codebase related to uniswap router had some problem.. hope to handle this in a separated issue

@rndquu what do you think?

The credits page must open without errors. Otherwise there is no sense in the current PR as we will need to fix the credits page ourselves.

Hmm.. I've already checked that error. But not easy to find the root cause. Hope one who has experience in it solve this

@molecula451 Could you help with this error?

@molecula451
Copy link
Member

This uniswap router issue wasn't generated from diamond architecture implementation.. seems original codebase related to uniswap router had some problem.. hope to handle this in a separated issue

@rndquu what do you think?

The credits page must open without errors. Otherwise there is no sense in the current PR as we will need to fix the credits page ourselves.

Hmm.. I've already checked that error. But not easy to find the root cause. Hope one who has experience in it solve this

@molecula451 Could you help with this error?

yes I will take a look as a part of the review

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 10, 2023

yes I will take a look as a part of the review

Any luck with this?

@bojan07
Copy link
Contributor Author

bojan07 commented Oct 10, 2023

While updating from several times of reviews, the work related to credit page was completely reflected to PR804 through the recently added a few commits. so this can be closed to avoid complexity

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.

UI: apply diamond to credits page
4 participants