Deprecate BLS/BN short type aliases and disambiguate Fr#1714
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request removes deprecated BLS12-381 convenience type aliases and disambiguates the scalar field types Fr by introducing prefixed variants Bls12381Fr and Bn254Fr. This change addresses a potential runtime confusion where contracts might create their own aliases to shortened types (e.g., use Bn254G1Affine as G1Affine) and encounter type mismatches at runtime due to ambiguous spec mappings.
Changes:
- Removed type aliases (
G1Affine,G2Affine,Fp,Fp2,Fr,BnScalar) that mapped to BLS12-381 types - Renamed
FrtoBls12381Frin BLS12-381 module andBn254Frin BN254 module for clear disambiguation - Updated spec mappings to recognize only the prefixed type names (
Bls12381Fr,Bn254Fr) - Updated all test contracts, generated code, arbitrary helpers, and documentation to use the new type names
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| soroban-sdk/src/crypto/bls12_381.rs | Removed type aliases (G1Affine, G2Affine, Fp, Fp2) and renamed Fr to Bls12381Fr throughout, updated all method signatures and documentation |
| soroban-sdk/src/crypto/bn254.rs | Renamed Fr to Bn254Fr throughout, updated all method signatures and documentation |
| soroban-sdk/src/crypto.rs | Removed BnScalar alias export |
| soroban-sdk-macros/src/map_type.rs | Removed spec mappings for unprefixed type names (Fp, Fp2, G1Affine, G2Affine, Fr), added Bls12381Fr and Bn254Fr mappings |
| tests/bn254/src/lib.rs | Updated imports and usage from Fr to Bn254Fr |
| tests/bls/src/lib.rs | Updated imports and usage from Fr to Bls12381Fr |
| tests-expanded/test_bn254_*.rs | Updated generated code with new type names |
| tests-expanded/test_bls_*.rs | Updated generated code with new type names |
| soroban-sdk/src/testutils/arbitrary.rs | Updated arbitrary helpers to use prefixed type names |
| soroban-sdk/src/tests/crypto_bls12_381.rs | Updated unit tests to use Bls12381Fr and other prefixed types |
| soroban-sdk/src/_migrating/v25_bn254.rs | Updated migration guide documentation with new type names |
|
This seems like a breaking change which I'd be hesitant to roll out now without spanning it over multi-versions. The types are in a major version so for better or worse they're out there.
Can we not put a deprecation on the aliases? If we really need to we can inject a deprecation with the contractimpl and contracttype macros at the point of use, but I think we should be able to do this on the alias? |
I don't think I tried this. The structure of this changed a couple times during development, but I believe you should be able to deprecate the aliases. |
6f7c6d6 to
2d96fe5
Compare
FrFr
|
@sisuresh @leighmcculloch @mootz12 I've refreshed this PR to perform (backward compatible) deprecation, instead of removal of the short aliases. Please take another look. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
soroban-sdk/src/crypto/bls12_381.rs:511
- The comment still refers to “Fr construction paths” even though the type is now
Bls12381Fr. Updating the comment to the new canonical name would reduce ambiguity (especially sinceFrnow exists as a deprecated alias in multiple modules).
// Keep all Fr construction paths canonical by reducing modulo r here.
// Constructors and deserialization paths should route through this impl.
// Skip the expensive rem_euclid when value is already canonical (< r),
…, fix Bn254Fp fuzz clamping - Rename BLS12-381 Arbitrary types to match prefixed names (ArbitraryFp → ArbitraryBls12381Fp, etc.) - Add BnScalar to map_type spec mappings for backward compat - Clamp ArbitraryBn254Fp MSB to avoid panics during fuzzing - Fix G1 coordinate comment: X, Y ∈ Bn254Fp, not Bn254Fr
…format Clarify that Bn254G1Affine, Bn254G2Affine, Bls12381G1Affine, and Bls12381G2Affine are thin BytesN wrappers whose from_bytes constructors accept arbitrary bytes. Serialization requirements (curve membership, flag bits, subgroup checks) are enforced by the Soroban host at the point of use, not at construction time.
… format v57 support
5fc52b1 to
4967bae
Compare
What
bls12_381::Fr→Bls12381Frandbn254::Fr→Bn254Fr, keeping the old names as deprecated type aliases.G1Affine,G2Affine,Fp,Fp2) and thecrypto::BnScalarre-export.Bls12381FrandBn254Frto the spec type mappings inmap_type.rs(old mappings preserved).ArbitraryBn254Fpfor fuzz coverage.Why
Both
bls12_381andbn254modules exported a struct namedFr. A contract that writesuse soroban_sdk::crypto::bn254::Frand uses it as a contract type will haveFrsilently resolve to the BLS12-381 scalar in the spec mapping, causing a confusing type mismatch at runtime (see example).Disambiguating the structs and deprecating the short aliases gives existing code a clear compiler warning pointing to the new name, instead of a surprise runtime failure.
Known limitations
More verbose names, which is better than surprise behavior. Old code continues to compile — users just see deprecation warnings. We can remove the deprecated aliases in a future release.