Looking for code review - FastOTP - TOTP application

code review
#1

I've made my first Rust project and wanted some feedback to learn about my mistakes and improve my Rust skills. I've used this project as a learning ground for Rust so I am sure there are a ton of stuff that can be improved, nonetheless I don't have anybody besides me to get feedback so I am coming to you for this.

Here's my project: https://gitlab.com/fastotp1/fastotp

It tries to make use of Robert C. Martin Clean Architecture to achieve a clean separation between logic and non-logic components and having as a consequence, hopefully, of allowing multiple GUI front-ends to be implemented.

Additionally I've also collected a bunch of questions that have arisen:

Questions:

  • Is the use of Arc + AsyncRwLock sensible for the shared types or could it be replaced with something simpler?
  • Where could owned values be replaced with borrowing, impl Trait, generics, etc.?
  • Is it ok that every struct uses String instead of &str?
  • Is the use of lifetimes around the code base adequate?
  • Which performance optimizations could be done (e.g. redundant clone()'s, boxing, lack of use of generics, etc.)
  • Are the implementations of the streams correct? How can they be improved?
  • Which changes in code organization or similar can be made to improve compile times?
  • Which traits could be implemented that could improve code quality and readability?
  • Could something be done to reduce repetition of the presenters (e.g. macros)?
  • Where could declarative macros help reducing boilerplate in fastotp_iced_gui crate?
  • Is code organization well-reasoned (e.g. modules, use of directory with mod.rs + single .rs file)?
  • Which derive clauses should be used for each struct?
  • What could be improved in the architecture?
  • Is the use of AES-GCM-SIV ok, or better something like XChaCha20-Poly1305?
  • Are the used imports the better ones?
  • What could be behind a cargo feature?
  • Is the architecture adequate to have interchangeable UI frontends (e.g. GUI, CLI, web, etc)?
  • Which documentation practices could be included?
  • Where could the use of const generics, const arrays, or in general const operations improve the code?
  • Where could the try '?' operator be used to improve code reading?

If you have experience with Gitlab, or you have feedback outside the Rust code itself, that is also welcome.

Thanks for looking by

#2

Bumping in hopes someone wants to gives some feedback :crossed_fingers:

#3

Arc is for sharing, and it's normal to see it used often in async and GUI applications. AsyncRwLock can block long-running operations, so it depends whether you want that behavior or not. You could use Arc<Mutex> or ArcSwap to quickly replace shared data, and handle async throttling elsewhere.

  • Where could owned values be replaced with borrowing, impl Trait, generics, etc.?

I see you're implementing poll yourself. async fn syntax sugar allows self-referential Futures, which allows more references in async code. If you don't need generic code, don't add it. It makes compilation slower and compiler errors more vague.

  • Is it ok that every struct uses String instead of &str?

Yes, that's good. Structs almost always need to own their data, and &str can't be used to store strings.

  • Is the use of lifetimes around the code base adequate?

If it compiles — yes :slight_smile:

  • Are the implementations of the streams correct? How can they be improved?

I use async-stream, so I don't have to worry about correctness of poll_next or pinning.

  • Which changes in code organization or similar can be made to improve compile times?

Try -Z timings:

and cargo bloat.

  • Which traits could be implemented that could improve code quality and readability?

I don't think traits help with quality/readability in general. You implement them when they're needed.

But for nitpicks about readability cargo clippy -- -W clippy::pedantic has opinions.

  • Is code organization well-reasoned (e.g. modules, use of directory with mod.rs + single .rs file)?

It is a bit of a pain to browse it in gitlab. You could use inline mod foo { mod bar; } to avoid having nearly-empty foo/mod.rs.

  • Which derive clauses should be used for each struct?
  • Copy whenever you can for private structs, and on public structs if you don't expect to add any non-trivial fields to it. It just makes working with such structs easier.
  • Eq on simple enums is useful, and often forgotten.
  • Debug on everything in the public API, since library users can't add it themselves and may want to do printf-debugging.

There's a lot, so I haven't answered all the questions.

2 Likes
#4

kornel... this is so awesome! Very grateful for your input, and your time. I'll go around the code base to see how to improve the code.

#5

Couldn't find for sure what are you referring to with this. Do you mean for example the implementation of the traits AsyncWrite or Stream? I am confused about this.

I conceived the core crate to be async-runtime agnostic, whence I implemented myself the Stream trait. But looks like I can achieve the same using Tokio's async-stream crate, is it? Or would that force the consumers of core to use Tokio runtime?

#6

I think async-stream is runtime-agnostic. It implements Stream interface for you. In my projects I happen to use tokio, so I haven't looked much into being runtime-agnostic.

#7

The async-stream crate is runtime agnostic.