-
Notifications
You must be signed in to change notification settings - Fork 622
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
feat: limit contract functions number #4954
Conversation
Co-authored-by: Bowen Wang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but please address @matklad comments about tests
There was a problem with reusing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, feel free to auto-merge!
#[cfg(feature = "protocol_feature_limit_contract_functions_number")] | ||
LimitContractFunctionsNumber, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ProtocolFeature
is a natural entry point for learning about implications of a particular feature, so I think comments on enum variants here have high leverage -- you can say little, but that wouldn't require ongoing maintanence, will help other people, and could be copy-pasted into the feature stabilization PR.
let result = make_simple_contract_call_with_protocol_version_vm( | ||
&code, | ||
method_name, | ||
new_protocol_version, | ||
vm_kind, | ||
); | ||
assert_eq!(result.1, None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an alternative, I'd write this as
let (_outcome, err) = ...
assert_eq!(err, None);
Explicit tuple unpacking with naming of the ignored components improves readability.
While working with #4954, initially I put new test into `contract_preload` test file, though `invalid_contracts` turned out to be a better place. Rename test files, hope this will make the choice of places for new tests in the future clearer. Test plan --------- Existing tests.
Stabilize limiting functions number in a contract to 10_000: #4954 While discussing #4826, we found that both contract compilation and function call require a lot of time, if number of wasm functions in it is huge. Even if we fix contract size, function number still makes a big difference. To mitigate this issue, we limit number of functions in a contract. This shouldn't affect current mainnet contracts because maximal number of functions is ~5k. ## Test plan * http://nayduck.near.org/#/run/2223 * existing tests
Limit max contract functions number to
200010000.See corresponding Zulip thread for more details.
Test plan
Notes: