Move EIP-2537 review, add new contributor #3343
Conversation
Make even with master
Make even
Make even
Make even
Make even
Even with master
EIPS/eip-2537.md
Outdated
| - Original implementation by zCash in [Rust](https://github.com/zcash/librustzcash/tree/master/pairing) | ||
| - [MCL library](https://github.com/herumi/mcl) and it's bindings in other languages | ||
| There are two fully spec compatible implementations: | ||
| - One in Rust language that is based on the EIP1962 [code](https://github.com/matter-labs/eip1962). There exists an intergration with OpenEthereum for this library |
There was a problem hiding this comment.
| - One in Rust language that is based on the EIP1962 [code](https://github.com/matter-labs/eip1962). There exists an intergration with OpenEthereum for this library | |
| - One in Rust language that is based on the EIP1962 [code](https://github.com/matter-labs/eip1962). There exists an integration with OpenEthereum for this library |
There was a problem hiding this comment.
@shamatar CI is blocked on spelling error.
|
Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):
|
|
@MicahZoltu I've moved online test vectors to the "assets", but the bot looks to be not to happy about it |
|
The bot just requires a reviewer to chime in when new files are created outside of the EIPs directory. It is fine and normal. 😄 I'll try to take another look at this later today and get it merged (unless another editor beats me to it ♥) |
EIPS/eip-2537.md
Outdated
| ## Reference Implementation | ||
|
|
||
| ## Implementation | ||
|
|
||
| There is a various choice of existing implementations of the curve operations. It may require extra work to add an ABI: | ||
| - BLS12-381 code bases for Eth 2.0 clients | ||
| - Chia's library in [C++](https://github.com/Chia-Network/bls-signatures) | ||
| - Milagro in [various languages](https://github.com/apache/incubator-milagro) | ||
| - Noble in [TypeScript/Javascript](https://github.com/paulmillr/noble-bls12-381) | ||
| - EIP1962 code bases with fixed parameters | ||
| - [Rust](https://github.com/matter-labs/eip1962) | ||
| - [Go](https://github.com/kilic/eip2537) | ||
| - [C++](https://github.com/matter-labs/eip1962_cpp) | ||
| - Original implementation by zCash in [Rust](https://github.com/zcash/librustzcash/tree/master/pairing) | ||
| - [MCL library](https://github.com/herumi/mcl) and it's bindings in other languages | ||
| There are two fully spec compatible implementations: | ||
| - One in Rust language that is based on the EIP1962 [code](https://github.com/matter-labs/eip1962). There exists an integration with OpenEthereum for this library | ||
| - One implemented specifically for Geth as a part of the current [codebase](https://github.com/ethereum/go-ethereum/tree/master/crypto/bls12381) | ||
|
|
There was a problem hiding this comment.
| ## Reference Implementation | |
| ## Implementation | |
| There is a various choice of existing implementations of the curve operations. It may require extra work to add an ABI: | |
| - BLS12-381 code bases for Eth 2.0 clients | |
| - Chia's library in [C++](https://github.com/Chia-Network/bls-signatures) | |
| - Milagro in [various languages](https://github.com/apache/incubator-milagro) | |
| - Noble in [TypeScript/Javascript](https://github.com/paulmillr/noble-bls12-381) | |
| - EIP1962 code bases with fixed parameters | |
| - [Rust](https://github.com/matter-labs/eip1962) | |
| - [Go](https://github.com/kilic/eip2537) | |
| - [C++](https://github.com/matter-labs/eip1962_cpp) | |
| - Original implementation by zCash in [Rust](https://github.com/zcash/librustzcash/tree/master/pairing) | |
| - [MCL library](https://github.com/herumi/mcl) and it's bindings in other languages | |
| There are two fully spec compatible implementations: | |
| - One in Rust language that is based on the EIP1962 [code](https://github.com/matter-labs/eip1962). There exists an integration with OpenEthereum for this library | |
| - One implemented specifically for Geth as a part of the current [codebase](https://github.com/ethereum/go-ethereum/tree/master/crypto/bls12381) |
This should either contain an inline reference implementation, a link to a reference implementation in ../assets/eip-2537/* or be removed. External links and lists (which need to be maintained) should not be included in EIPs.
EIPS/eip-2537.md
Outdated
| Test vector for all operations are expanded in this `csv` files in [repo](https://github.com/matter-labs/eip1962/tree/master/src/test/test_vectors/eip2537). | ||
| Test vector for all operations are expanded in this `csv` files in [repo](https://github.com/matter-labs/eip1962/tree/master/src/test/test_vectors/eip2537) as hex encodings of the inputs and outputs to the corresponding precompiles. |
There was a problem hiding this comment.
This should either contain an inline set of test vectors, a link to a set of test vectors in in ../assets/eip-2537/* or be removed. External links should not be included in EIPs.
EIPS/eip-2537.md
Outdated
|
|
||
| Mapping functions are implemented according to [IEFT specification](https://github.com/cfrg/draft-irtf-cfrg-hash-to-curve/blob/master/draft-irtf-cfrg-hash-to-curve.md#deterministic-mappings-mappings) version `v7`(!) using an simplified SWU method. It does NOT perform mapping of the byte string into field element that can be implemented in many different ways and can be efficiently performed in EVM, but only does field arithmetic to map field element into curve point. Such functionality is required for signature schemes. | ||
| Mapping functions are implemented according to [IEFT specification](https://github.com/cfrg/draft-irtf-cfrg-hash-to-curve/blob/master/draft-irtf-cfrg-hash-to-curve.md#deterministic-mappings-mappings) version `v7` using an simplified SWU method. Latter methods do not change a part that the corresponding precompiles implement. Mapping function does NOT perform mapping of the byte string into field element (as it can be implemented in many different ways and can be efficiently performed in EVM), but only does field arithmetic to map field element into curve point. Such functionality is required for signature schemes. |
There was a problem hiding this comment.
We shouldn't be referring to draft specifications at all, and definitely not draft specifications in external repositories. We have made a few exceptions in the past on allowing external links when linking to other standards bodies like RFC and IETF, and I can imagine adding IRTF to the list but definitely not to drafts.
If this is critical to the specification then you'll need to add it to the Specification section as either inline content or an asset in ../assets/eip-2537/*. You could make a copy of the draft specification and put it in assets if you wanted, but be aware that this spec will be bound to the version of the draft that is in the assets folder, not the version that IRTF ends up publishing so you should NOT call it an IEFT specification nor name it such. We don't want another keccak256/sha3 situation on our hands... cleaning up that mess took years (and we still deal with it today).
| There are two fully spec compatible implementations on the day of writing: | ||
| - One in Rust language that is based on the EIP1962 code and integrated with OpenEthereum for this library | ||
| - One implemented specifically for Geth as a part of the current codebase |
There was a problem hiding this comment.
While the removal of the links makes this technically valid, it suffers the same problem still which is that it is referencing stuff that may not be meaningful to future readers. For example, a similar reference to an implementation in Parity in an earlier EIP would be fairly worthless today since Parity as a product no longer exists.
EIPs are technical documents, and not an appropriate place to track lists of things (like lists of implementations). I recommend just removing this section entirely since it is optional.
EIPS/eip-2537.md
Outdated
| - [Cofactor cleanup](https://github.com/cfrg/draft-irtf-cfrg-hash-to-curve/blob/master/draft-irtf-cfrg-hash-to-curve.md#clearing-the-cofactor-cofactor-clearing) | ||
|
|
||
| Once again, [hash to field](https://github.com/cfrg/draft-irtf-cfrg-hash-to-curve/blob/master/draft-irtf-cfrg-hash-to-curve.md#hashing-to-a-finite-field-hashtofield) is NOT a part of this EIP as it can be implemented in EVM and with different strategies. | ||
| Algorithms and set of parameters for SWU mapping method is provided by a separate [document](../assets/eip-2537/field_to_curvee.md) |
There was a problem hiding this comment.
| Algorithms and set of parameters for SWU mapping method is provided by a separate [document](../assets/eip-2537/field_to_curvee.md) | |
| Algorithms and set of parameters for SWU mapping method is provided by a separate [document](../assets/eip-2537/field_to_curve.md) |
Typo in link is blocking CI.
There was a problem hiding this comment.
I was looking at TOML exception error and couldn't make sense of it
|
Merging this finally. Though, when it comes time for last call I'll probably push harder on the |
This PR addresses changes required by @MicahZoltu in a previous (closed) PR, and adds Kelly Olson as a second author so he can contribute and also champion this EIP too.