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

Cleanup on external API exposure #925

Merged
merged 3 commits into from
Oct 23, 2020
Merged

Cleanup on external API exposure #925

merged 3 commits into from
Oct 23, 2020

Conversation

holgerd77
Copy link
Member

@holgerd77 holgerd77 commented Oct 23, 2020

Closes #924

Ok, not the big sweeping blow I had hoped for but at least some substantial cleanup on what is exposed to the external API. Renamed some clearly internal members to underscore to better differentiate from the officially exposed functionality, also added protected keywords where possible so that these members are then not documented in the API docs.

Following reasoning and classifications for the respective member changes:

Categories

  • 1 ^= Not meant to be public at all -> _ and private
  • 2 ^= Not meant to be public but might be used in custom implementations -> _ and protected
  • 3 ^= Should be part of the official API
  • 4 ^= Not part of the official API, but is externally used -> _ and no explicit visibility
Name Category Assignment Change ->
opts 2 index.ts  _opts
allowUnlimitedContractSize  2 index.ts _allowUnlimitedContractSize
isInitialized  2 index.ts _isInitialized
 _opcodes  2  index.ts  _opcodes, added protected
 stateManager  3  index.ts  added comment for docs
blockchain  3 (?) index.ts  added comment for docs
 _common  4  index.ts  heavily used in tests
 _emit  1  index.ts  needs to be kept public due to code construction, added @hidden comment
_mcl  1  index.ts  needs to be kept public due to code construction, added @hidden comment

I also fixed the docs generation accidentally including the tests along the way and regenerated documentation.

@codecov
Copy link

codecov bot commented Oct 23, 2020

Codecov Report

Merging #925 into master will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Impacted file tree graph

Flag Coverage Δ
#block 75.79% <ø> (-0.22%) ⬇️
#blockchain 80.33% <ø> (-0.39%) ⬇️
#common 91.79% <ø> (ø)
#ethash 82.08% <ø> (ø)
#tx 88.37% <ø> (ø)
#vm 87.17% <100.00%> (+0.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member Author

Note: _emit needs to be kept public as well, had this wrong in the first version of the initial comment (in case you read on mail), have updated.


/**
* The StateManager used by the VM
*/
stateManager: StateManager
Copy link
Member

@jochem-brouwer jochem-brouwer Oct 23, 2020

Choose a reason for hiding this comment

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

Shouldn't stateManager, blockchain, common be marked readonly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good point, have added in e50af80. Realized that I actually had a wrong understanding of the Readonly modifier.

Copy link
Member

@jochem-brouwer jochem-brouwer left a comment

Choose a reason for hiding this comment

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

Looks great! 👍

@holgerd77 holgerd77 merged commit ce6c493 into master Oct 23, 2020
@holgerd77 holgerd77 deleted the fix-instance-exposures branch October 23, 2020 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VM exposes too much stuff
2 participants