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.

Sign up for GitHub

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

Jump to bottom

Fix readme implementation addresses #874

Merged
merged 1 commit into from
Jul 6, 2023
Merged

Fix readme implementation addresses #874

merged 1 commit into from
Jul 6, 2023

Conversation

makcandrov
Copy link
Contributor

No description provided.

Rubilmax
Rubilmax approved these changes Jul 4, 2023
View reviewed changes
Copy link
Collaborator

@Rubilmax Rubilmax left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it's a good practice to keep implementation addresses in the README, because these addresses:

  • are available in a single click via Etherscan
  • should not be trusted by integrators and rather accessed via an RPC call getStorageAt
  • can be outdated (as is the case here)

@MerlinEgalite
Copy link
Contributor

I don't think it's a good practice to keep implementation addresses in the README

Can you share the reasoning please?

@Rubilmax
Copy link
Collaborator

Rubilmax commented Jul 4, 2023 •
edited
Loading

Can you share the reasoning please?

edited (its the second time implementation addresses are outdated)

MathisGD
MathisGD reviewed Jul 4, 2023
View reviewed changes
Copy link
Contributor

@MathisGD MathisGD left a comment •
edited
Loading

Choose a reason for hiding this comment

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

These address can change, thus not be accurate, but they can all be retrieved from the proxy's one (that will not change). Isn't it easier to just remove them from the README ?

Edit I just saw that Romain has the same opinion, sorry for the duplicate.

pakim249CAL
pakim249CAL approved these changes Jul 5, 2023
View reviewed changes
Copy link
Contributor

@pakim249CAL pakim249CAL left a comment

Choose a reason for hiding this comment

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

I agree with romain and mathis, but im still approving because this is less wrong than before.

@makcandrov
Copy link
Contributor Author

makcandrov commented Jul 6, 2023 •
edited
Loading

forgot to sign the ccommit 😅

@makcandrov makcandrov merged commit 5236e88 into main Jul 6, 2023
@makcandrov makcandrov deleted the fix/readme branch July 6, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MathisGD MathisGD MathisGD left review comments

@Rubilmax Rubilmax Rubilmax approved these changes

@MerlinEgalite MerlinEgalite MerlinEgalite approved these changes

@pakim249CAL pakim249CAL pakim249CAL approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@makcandrov @MerlinEgalite @Rubilmax @pakim249CAL @MathisGD