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/18 #298

Closed
wants to merge 1 commit into from
Closed

fix/18 #298

wants to merge 1 commit into from

Conversation

Jean-Grimal
Copy link
Contributor

@Jean-Grimal Jean-Grimal commented Oct 17, 2023

@Jean-Grimal Jean-Grimal changed the title fix Issue 18 : rename wNativeToken to weth fix/18 Oct 17, 2023
Copy link
Collaborator

@Rubilmax Rubilmax left a comment

Choose a reason for hiding this comment

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

I'm not convinced we need to do this just yet. Yes, wrapped native on other chains doesn't require to be a fork of WETH. But:

  1. Everyone has incentives to do so because the WETH implementation is formally verified and battle-tested
  2. we can just define "wrapped native" as a fork of WETH on other chains and then our naming is correct, but the code is incompatible with unknown implementations of wrapped native

So IMO it's just a matter of definitions, but not a matter of naming

@MerlinEgalite
Copy link
Contributor

Indeed it works for polygon at least.

I'm not convinced we need to do this just yet.

Do you propose an alternative?

@Rubilmax
Copy link
Collaborator

Rubilmax commented Oct 18, 2023

Do you propose an alternative?

I'm suggesting to acknowledge and precise that what we name "wrapped native" actually targets forks of WETH on other chains! Thus we know that the code doesn't work with any wrapped native, but we think that no other implementation than WETH are worth adapting

@Jean-Grimal
Copy link
Contributor Author

I'm suggesting to acknowledge and precise that what we name "wrapped native" actually targets forks of WETH on other chains! Thus we know that the code doesn't work with any wrapped native, but we think that no other implementation than WETH are worth adapting

So should I do another PR with just this acknowledgement ?

@Rubilmax
Copy link
Collaborator

I'm suggesting to acknowledge and precise that what we name "wrapped native" actually targets forks of WETH on other chains! Thus we know that the code doesn't work with any wrapped native, but we think that no other implementation than WETH are worth adapting

So should I do another PR with just this acknowledgement ?

Best would be to have a consensus first

@MerlinEgalite
Copy link
Contributor

I'm ok with your suggestion @Rubilmax

@Jean-Grimal
Copy link
Contributor Author

I'm ok with your suggestion @Rubilmax

Same

@Rubilmax
Copy link
Collaborator

So I don't think it's necessary to have a comment in the code but if you feel like it let's go but the most important is to explain to cantina

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.

3 participants