Suggestion: Make group traits more "rusty"

[dignifiedquire]

I went through the current group traits and this is what I came up with, how they could fit better into the rust trait system

/// Element represents an element of a group with the additive notation
/// which is also equipped with a multiplication transformation.
/// Two implementations are for Scalar which forms a ring so RHS is the same
/// and Point which can be multiplied by a scalar of its prime field.
pub trait Element<'a, RHS = Self>:
    Clone
    + fmt::Display
    + fmt::Debug
    + Eq
    + std::ops::AddAssign<&'a Self>
    + std::ops::MulAssign<&'a RHS>
{
    /// Returns the zero element of the group.
    fn zero() -> Self;
    /// Returns the one element of the group.
    fn one() -> Self;

    /// Generates a new random element in the group.
    fn random<R: RngCore>(rng: &mut R);
}

/// Scalar can be multiplied by only a Scalar, no other elements.
pub trait Scalar<'a>: Element<'a> + Encodable + From<u64> + std::ops::SubAssign<&'a Self> {
    fn inverse(&self) -> Option<Self>;
    fn negate(&mut self);
}

// Could be replaced with `serde::Serialize and Deserialize`
pub trait Encodable {
    fn marshal_len() -> usize;
    fn marshal(&self) -> Vec<u8>;
    fn unmarshal(&mut self, data: &[u8]) -> Result<(), Box<dyn Error>>;
}

Let me know what you think

[kobigurk]

I think that would be a welcome change, although not an urgent one. Also, does element need the RHS type? Can we use Self directly instead?

One other comment - I usually prefer having an enum for the errors rather than a Box<dyn Error>.

[dignifiedquire]

Also, does element need the RHS type? Can we use Self directly instead?

Unclear to me, it seemed that there was some wish to make this more abstract.

One other comment - I usually prefer having an enum for the errors rather than a Box<dyn Error>.

Unfortunately that is not really a good way though for traits, as traits are meant to be open, to be implemented, but an enum is closed.

[nikkolasg]

Also, does element need the RHS type? Can we use Self directly instead?

Yes a Point takes a Scalar to multiply and a Scalar takes a Scalar :)

One other comment - I usually prefer having an enum for the errors rather than a Box.

I wanted to use that in the first place but it ended up being very difficult with all the different traits and so on. And to be honest, I've taken inspiration from how zexe code did it and it does like that also.

@dignifiedquire Even though statistically it does seem complicated, can we have a way though to detect which type an error is dynamically ?

[kobigurk]

Ah, right! Of course Element needs a type parameter. Then I'm curious about the choice of having a default of Self :)

[nikkolasg]

Just to avoid specifying it for Scalar ; no big reasons :D

[gakonst]

Unfortunately that is not really a good way though for traits, as traits are meant to be open, to be implemented, but an enum is closed.

You can use an associated type in the error which is generally the standard approach to this. Zexe's usage of the Box<dyn Error> can be improved as well

Also I recommend using thiserror and add #[from] variants for bubbling up errors.

[gakonst]

Concrete error types are implemented in #30, cc @nikkolasg @dignifiedquire

[gakonst]

• Encodable has been removed
• pick has been changed to rand (which returns the value instead by requiring a mutable reference)

[gakonst]

I'll be adding implementations for the `std::ops traits tomorrow.

[gakonst]

FWIW implementing this for bls12-381 would require wrapping the types and implementing the traits, since we cannot implement a foreign trait for a foreign type. Doing it for bls12-377 is not an issue since the types are already wrapped.

[dignifiedquire]

@gakonst I have been wanting to add std::ops traits to bls12-381 for a while. If we add them to https://github.com/filecoin-project/group it should work.