stabilizing ed25519_verify host function #8098
Conversation
|
Unsure on how to properly bump the protocol version |
There was a problem hiding this comment.
@blasrodri mostly looks right, one important change to be made on the WASM imports
Further, runtime::config_store::tests::test_json_unchanged fails. You need to update the insta snapshots to include the new parameters.
Finally, you will need to rebase. I have done significant refactoring around some of the lines changed here in the meantime (sorry about that). Therefore, please go against common practice to rebase after the review and instead rebase now so that I can do the final review on the rebased code. (Hint: If you run into many conflicts, it might even be easier to start from master:HEAD again.)
blasrodri
force-pushed
the
blas/ed25519-stabilization
branch
from
572b416 to
be1223b
Compare
There was a problem hiding this comment.
Hey @blasrodri
This PR has 4 failing tests in the near-primitives crate due to observable changes in the gas profiles and runtime configuration. This is expected, as we are adding a new cost. You need to update the snapshots accordingly.
Best is to cargo install cargo-insta and run tests with cargo insta test -p near-primitives followed by cargo insta review. Otherwise you have to update snapshots one at the time and rerun tests for each protocol version. (see also insta.rs)
Also, could you please add an entry to the changelog that announces the new feature?
blasrodri
force-pushed
the
blas/ed25519-stabilization
branch
from
3a33ee7 to
f9b2641
Compare
core/primitives/src/version.rs
Outdated
| @@ -235,7 +234,6 @@ impl ProtocolFeature { | |||
| ProtocolFeature::FixStakingThreshold => 126, | |||
| #[cfg(feature = "protocol_feature_fix_contract_loading_cost")] | |||
| ProtocolFeature::FixContractLoadingCost => 129, | |||
| #[cfg(feature = "protocol_feature_ed25519_verify")] | |||
| ProtocolFeature::Ed25519Verify => 131, | |||
There was a problem hiding this comment.
Ah, this will need a lower protocol number now so that it is included in the stable version. Please move it up to the stable features and update the number accordingly.
Note that we have already bumped the protocol version to 58 in #8360, so you can just set the version to 58 and don't have to bump again.
Co-authored-by: Jakob Meier <mail@jakobmeier.ch>
There was a problem hiding this comment.
Looks good, will merge it now.
Thanks @blasrodri for your tireless work from start to finish! 🙏
Feature stabilization for `ed25519_verify`
Feature stabilization for
ed25519_verify