Fix clippy and audit warnings #667
Conversation
| fn normalize_scale(&self, details: ConvertDetails) -> Result<Self::Item, ()>; | ||
| fn normalize_scale( | ||
| &self, | ||
| details: ConvertDetails, | ||
| ) -> Result<Self::Item, Box<dyn std::error::Error + 'static>>; |
There was a problem hiding this comment.
Yeah it looks like a public trait, but interledger-settlement is only 0.3 so this is much easier breaking change.
As this will be a public api going forward and regardless of anyone using it, I'd collect the common issues to an enum ConversionError if there are more than just the Overflow case? I cannot at least see one; strange that there is no underflow or the normalization returning zero.
Then again, it'd seem that this is used to unwrap..
There was a problem hiding this comment.
I agree, let's go for en enum then. I will see if there is a few possible errors and address map them in enum. If not, I will just make a struct for an ConversionError
There was a problem hiding this comment.
There is no more specific conversion errors at the moment. In the other file crypto.rs there has been a case of a two different errors being handled by the same unit error (). I have replaced those to one single error similar to here as well, because it was also used to unwrap, and there were no specific branches of different errors (because there was not any). I would omit adding different errors in this iterations, probably better in some future PR when/if we change these functions as well.
Anyway, I have just added the struct ConversionError here, and did the same in other places where the unit error was used. Hope it makes sense :)
|
@koivunej Thanks for checking! I checked |
|
Oki, I am thinking we should just merge this ahead, or have the 24h grace period as usual starting from now. I am for waiting, as there seems to be no rush. |
|
@emschwartz could you relax the merging requirements as the circleci is now defunct? Please see #669 for the next steps. |
|
An alternative to getting this in would be to make disable other than the clippy for example in circleci for now and go with a lighter, fix the build by migrating to github actions in the next PR. |
BREAKING CHANGE: changes return type
BREAKING CHANGE: rederive_secret return type
BREAKING CHANGE: change return type
|
I have included a working circleCI changes, and re-enabled the clippy and audit checks. Can I merge this one without 24h warning, so we can unblock other PRs? |
|
Yeah lets go ahead and merge, the 24h grace period already passed! |
cargo clippyandcargo audithave some new warnings since the new version. This is an attempt to simply fix those.