Remove orphan instance for MonadRandom

[pjones]

I have a MonadRandom instance in my application that looks like this:

instance (MonadIO m) => MonadRandom (MyTransfomer m) where
  getRandomBytes = ...

This generates an overlapping instance error due to the vagueness of the instance in hs-jose.

I'm not sure why this package needs the orphan instance but is there a way to remove it or use a newtype wrapper?

[frasertweedale]

It is needed by the test suite. I don't object to removing it (or rather, moving it to the test suite where it is actually used). However I don't plan on cutting a new release any time soon. So in the mean time, you can probably replace your MonadRandom instance with an instance of MonadTrans MyTransformer and the MonadRandom instance exported by jose will be used.

[pjones]

@frasertweedale Is there anything I can do to help? I can submit a PR and prepare a release if that's helpful.

[frasertweedale]

@pjones the change is already committed on master. I just don't think there's enough to release (removing a type class instance warrants more than a patch-level release).

Per my previous comment, you should be able to instance MonadTrans for MyTransformer instead of MonadRandom, and then be able to use the MonadRandom instance from jose.

[frasertweedale]

@pjones sorry for misleading you - I'm actually going to revert the change I made yesterday. The example programs and typical application code rely on MonadRandom. Ideally we could see the instance (MonadRandom m, MonadTrans t) => MonadRandom (t m) in cryptonite itself (where MonadRandom is defined), so that we can avoid orphan instances anywhere. I'll investigate whether that is possible.

[pjones]

Thank you for trying to find a solution to this.

The reason I can't use the MonadRandom instance from jose is because the the transformer I want to take it from has a concrete implementation via Cryptonite. If I used the instance from jose it would delegate back to the base monad which in my case doesn't have a MonadRandom instance.

For now I have a workaround, but it would be nice if I didn't have to deal with orphan instances.

[hdgarrood]

I realise this is old but I’d really appreciate if we could reconsider removing this instance from the library, for the same reason as the one given upstream in cryptonite: lifting is not the only valid implementation. Plus this is an orphan, which adds a new level of potential frustration as previously working code can stop working due to the orphan coming into scope.

[frasertweedale]

Reopening. Gonna deal with this properly, at long last.

[frasertweedale]

Seeking feedback on proposed solution: https://github.com/frasertweedale/hs-jose/commit/b163ce277602905c97a318ef5982ba6e462f3ea0. The diff in the test/ and example/ subdirectories gives an idea of what consumers will need to do to migrate. In most cases, it is sufficient to replace runExceptT with runJOSE.

[hdgarrood]

Looks good to me! I’d suggest getting rid of the -fno-warn-orphans pragmas in src/ so that you get warnings if there are any other orphans in there.

[frasertweedale]

Looks good to me! I’d suggest getting rid of the -fno-warn-orphans pragmas in src/ so that you get warnings if there are any other orphans in there.

Thanks, I updated the commit. Some of the -fno-warn-orphans pragmas are still required to suppress warnings about overlapping instances. I may try to address that in subsequent commits.

[frasertweedale]

I merged the change in commit https://github.com/frasertweedale/hs-jose/commit/b163ce277602905c97a318ef5982ba6e462f3ea0. I plan to cut a new release in about one week. In the meantime please share any more feedback about the change.