VM docs v5 #846
VM docs v5 #846
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
thanks for putting this together! I will make sure all my relevant PRs are documented here as well. to do a final docs generation you can run sorry for the "performance alert", I'm still working on stabilizing the benchmark results. |
|
Great @ryanio! Here are the search queries I used to find (most) applicable PRs: Small note: some PRs which do edit the VM did not have the VM label so removing the I have not added CI changes or changes to tests to the changelog, I don't think this matters for the end user? |
There was a problem hiding this comment.
Thanks @jochem-brouwer, these release notes look really nice and clean, that's a great basis for a release! 🎉
I won't review on completeness for now and rather merge in the spirit that this will bring us closer to the process that we generally target, so do the updates on the CHANGELOG if possible along the PR work and the use the final release PR for a final look on completeness, eventually do some prose additions to describe the releases and update the respective READMEs.
So will merge this now in.
| - `Chainstart` (a.k.a. Frontier) (v5, UNRELEASED) | ||
| - `Homestead` (v5, UNRELEASED) | ||
| - `TangerineWhistle` (v5, UNRELEASED) | ||
| - `SpuriousDragon` (v5, UNRELEASED) |
There was a problem hiding this comment.
Let's not change the READMEs for now on these PRs, then we can actually relatively gently move forward and merge these PRs independently on the way towards a release.
There was a problem hiding this comment.
For now I have added some "v5, UNRELEASED" notes here.
| - `Chainstart` (a.k.a. Frontier) (v5, UNRELEASED) | ||
| - `Homestead` (v5, UNRELEASED) | ||
| - `TangerineWhistle` (v5, UNRELEASED) | ||
| - `SpuriousDragon` (v5, UNRELEASED) | ||
| - `Byzantium` | ||
| - `Constantinople` | ||
| - `Petersburg` (default) |
There was a problem hiding this comment.
Just discovered here with some shuddering surprise that we are still on Petersburg with the VM (re-checked in the code as well). We should move this to Istanbul before a release, have added this to the VM v5 release TODO list #681 (see reasoning there why not `MuirGlacier).
Comment is somewhat review-unrelated though.
| - `Byzantium` | ||
| - `Constantinople` | ||
| - `Petersburg` (default) | ||
| - `Istanbul` | ||
| - `MuirGlacier` (only `mainnet` and `ropsten`) | ||
|
|
||
| If you are still looking for a [Spurious Dragon](https://eips.ethereum.org/EIPS/eip-607) compatible version of this library install the latest of the `2.2.x` series (see [Changelog](./CHANGELOG.md)). | ||
|
|
There was a problem hiding this comment.
We can leave this removed until release.
| @@ -192,7 +192,7 @@ export default class VM extends AsyncEventEmitter { | |||
| * reverted if an exception is raised. If it's `false`, it won't revert if the block's header is | |||
| * invalid. If an error is thrown from an event handler, the state may or may not be reverted. | |||
| * | |||
| * @param opts - Default values for options: | |||
| * @param {RunBlockOpts} opts - Default values for options: | |||
There was a problem hiding this comment.
Ah, I actually removed all these type notes on the MPT PR ethereumjs/merkle-patricia-tree#125 😀 😛
For TypeDoc this is not necessary since it is read from the function signatures - see docs - and we should actually remove this from all code since it is redundant information and increases the chance to introduce some doc inconsistencies when this would be forgotten to be updated on signature changes (and it is just plainly unnecessary extra work as well).
So we should these type additions from all or selected libraries all together along some subsequent PRs. Will not make this a blocker on this round, since this is the only real addition and this needs to be addressed more systematically anyhow.
There was a problem hiding this comment.
Update: ah, 2-3 more additions, anyhow.
| @@ -202,6 +207,7 @@ async function applyBlock(this: VM, block: any, opts: RunBlockOpts) { | |||
| * as well as gas usage and some relevant data. This method is | |||
| * side-effect free (it doesn't modify the block nor the state). | |||
| * @param {Block} block | |||
| * @param {RunBlockOpts} opts | |||
There was a problem hiding this comment.
Thanks for all these corrections on the signature documentation. 😀
Updated changelog/readme/docs of VM.
Some general remarks: it would be helpful if we would also review PRs on their documentation: if any external feature is added or changed, the docs should be updated (I found a few inconsistencies or errors). The same goes with the changelog: I had to go back to January this year to figure out what was changed or not. It would be helpful if we have some kind of procedure for this (such as adding items to an unreleased version paragraph in the changelog).