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] FixedNumber private properties #4143

Conversation

RauppRafael
Copy link

Updated FixedNumber's properties to use typescript's private keyword instead of the # notation.

This fixes issue #4131.

I've kept public functions without the public keyword so I don't make unwanted changes, but maybe we should add it for consistency when we have private variables

@ricmoo
Copy link
Member

ricmoo commented Jun 13, 2023

The private keyword and private members are very different things. :)

The private keyword is just a hint to the TypeScript compiler that it should not allow other TypeScript things to access this value; however, casting can circumvent this and for anyone using JavaScript without TypeScript they are unrestricted and do not even receive a warning what they are doing will break the world.

A private member is protected by the JavaScript runtime, to prevent access and prevent tampering with the internal state that could result in unexpected behaviour.

It seems like the library wrapping the objects mentioned in #4131 needs to be updated to properly reflect values onto the correct instance… Here are the docs on proper Proxy reflection to ensure supporting instances with private members… It should be an easy change in the target library and something three would likely want to support, since private members are used in quite a few places, not just Ethers…

@ricmoo
Copy link
Member

ricmoo commented Jul 14, 2023

Closing this, as the private fields are much safer, in general. In v7, I plan to switch to using WeakMaps and manually manage private members.

@ricmoo ricmoo closed this Jul 14, 2023
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.

2 participants