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

Set execution params to presets #2702

Merged
merged 2 commits into from
Nov 2, 2021
Merged

Set execution params to presets #2702

merged 2 commits into from
Nov 2, 2021

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Nov 1, 2021

Spec builder fixes:

  1. The execution params should have been moved to "presets" since they are defined in preset files.
  2. Add spec builder assertions to avoid Update max transaction size in setup.py #2701 issue happening again
    • It will add assert MAX_BYTES_PER_TRANSACTION == uint64(1073741824) to the spec.
    • updated: reverted it

@hwwhww
Copy link
Contributor Author

hwwhww commented Nov 1, 2021

Second thought: the assertion would be incorrect if minimal preset value is different from the mainnet preset value. I can revert it if they are likely to be changed.

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

nice work!

i agree w/ you that we would need to extend the hardcoded constants handling to respect presets and in my opinion doing that work doesn't pull its own weight

the other change should definitely be merged in -- nice catch!

@@ -64,7 +63,7 @@ Additionally, this upgrade introduces the following minor changes:
| `Transaction` | `ByteList[MAX_BYTES_PER_TRANSACTION]` | either a [typed transaction envelope](https://eips.ethereum.org/EIPS/eip-2718#opaque-byte-array-rather-than-an-rlp-array) or a legacy transaction|
| `ExecutionAddress` | `Bytes20` | Address of account on the execution layer |

## Constants
## Preset
Copy link
Member

Choose a reason for hiding this comment

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

ah nice catch on these!

setup.py Outdated Show resolved Hide resolved
@djrtwo djrtwo mentioned this pull request Nov 1, 2021
3 tasks
@hwwhww hwwhww changed the title Set execution params to presets and add builder checks Set execution params to presets Nov 2, 2021
@hwwhww
Copy link
Contributor Author

hwwhww commented Nov 2, 2021

@ralexstokes Thanks for the review! I reverted the setup.py changes.

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

thanks for the update!

lgtm!

@ralexstokes ralexstokes merged commit 79d005e into dev Nov 2, 2021
@djrtwo djrtwo deleted the merge-preset branch November 2, 2021 20:04
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.

None yet

2 participants