Add FromRaw impl for some curve points

[maurges]

This is a small change, but I never contributed to generic-ec-core, so I'm taking suggestions on the whole design

[maurges]

The checks are failing because one dependency now requires "fontconfig", what

WTF happened here, I can't find yeslogic-fontconfig-sys in transitive dependencies

[survived]

It is strange to refer to the generic_ec::TryFromRaw trait which is located in upstream library. Maybe NoInvalidPoints should be in generic_ec?

[maurges]

If it's in generic-ec, then I can't refer to it to impl it in generic-ec-curves, right?

[survived]

You can, as it is a trait first defined in generic_ec

[survived]

If you define a trait in your crate, you can implement it on whatever structs you want. Other crates, however, will only be able to implement your trait only on the types defined in those crates.

[maurges]

You mean I would impl it in generic-ec as well? I thought for an unsafe trait it's better to keep it closer to the curves definitions, and also to have less of #[cfg(feature)] pollution

[survived]

Yes, curves crate can't possibly implement trait from generic-ec as it's upstream, it would cause circular dependency.

I thought for an unsafe trait it's better to keep it closer to the curves definitions

I think, generally it's a good practice, but I also think it's acceptable to do otherwise in this case

[survived]

and also to have less of #[cfg(feature)] pollution

That's just one #[cfg(feature)] per each impl

[survived]

I mean, you can keep it in the core if you prefer, I just suggest to rewrite/update the comment to not refer to a trait in upstream crate

[maurges]

I agree with you, it is a bit silly to ask the implementers to look at the downstream crate. I'll think about it a bit

[survived]

You need to set up sign-off for lockness repos, see https://github.com/LFDT-Lockness/generic-ec/pull/50/checks?check_run_id=34596977449

[survived]

What is that?

generic-ec/.github/workflows/rust.yml

Lines 40 to 48 in 535f719

	check-all:
	runs-on: ubuntu-latest
	steps:
	- uses: actions/checkout@v3
	- uses: Swatinem/rust-cache@v2
	with:
	cache-on-failure: "true"
	- name: Check everything
	run: cargo check --all-targets

It should be cargo check --all-features...

[maurges]

You need to set up sign-off for lockness repos

Done with a force push

[survived]

Ah okay, --all-targets means like all tests and examples... I was thinking about all targets like linux and ios

[survived]

I'm also curious why in the CI we see that deps are being updated. Did you forget to commit Cargo.lock?

Run cargo test --all-features --workspace
    Updating crates.io index
     Locking 205 packages to latest compatible versions
      Adding generic-array v0.14.7 (available: v1.1.1)
      Adding phantom-type v0.4.2 (available: v0.5.2)
      Adding serde_with v2.3.3 (available: v3.11.0)
      Adding tabled v0.15.0 (available: v0.17.0)
   Compiling generic-array v0.14.7
   Compiling phantom-type v0.4.2
   Compiling yeslogic-fontconfig-sys v6.0.0
   Compiling serde_with v2.3.3

[maurges]

I'm also curious why in the CI we see that deps are being updated. Did you forget to commit Cargo.lock?

Well I found out yesterday that we never actually had a lockfile in this repo at all. So now we can't even know what broke, our dependencies or ubuntu-latest in the runner

[survived]

ah well, can you commit your cargo lock? Maybe it will help

[survived]

Or you did a fresh build?

[maurges]

I did a fresh build. You can add your own cargo-lock to this PR

[survived]

Let's see if that helps

[maurges]

Nope, still misses fontconfig. I guess I'll try downgrading ubuntu image next

[survived]

You can also install a package

[maurges]

Or I could do that. I didn't because all the ways to do it are stupid and waste compute, or overcomplicated

[maurges]

So I see two easy choices: either install the missing package, or explicilty use ubuntu-22.04 instead of ubuntu-latest - according to docs, until recently 22.04 was the "latest", and I can confirm that CI passes with it selected

[survived]

I'm fine with whatever option!

[survived]

Probably it's also a good idea to re-export NoInvalidPoints trait in generic_ec::traits?

generic-ec/generic-ec/src/lib.rs

Lines 219 to 228 in fafa3ed

	pub mod traits {
	#[doc(inline)]
	pub use crate::core::{One, Reduce, Samplable, Zero};
	/// Trait that allows you to check whether value is zero
	pub trait IsZero {
	/// Checks whether `self` is zero
	fn is_zero(&self) -> bool;
	}
	}

[maurges]

Probably it's also a good idea to re-export NoInvalidPoints trait in generic_ec::traits?

I don't think we should, this is a low-level trait for implementing, while for usage there exists the corresponding high-level trait FromRaw

[survived]

wouldn't it be helpful to be able to write E: Curve + NoInvalidPoints in generic bounds?