We seem to repeatedly do various mistakes when accepting PRs. In my experience a great way to avoid mistakes is to have a checklist.
GH even allows us to add it as PR template (may be a bit scary for newbies though). As an example LND has this.

Things that come to my mind, feel free to suggest your own or ask for rationale on things (I don't want to write the rationale now).

Checklist focus: good APIs, lack of panics, lack of obvious/common bugs (also lack of UB but that is guaranteed by the compiler :))
Non-goals: extreme performance, formal proofs of correctness

Things marked UBJ mean "use best judgement" - we don't define exact line here and will be decided as needed. The point is to not forget about considering this.
Things marked CMD mean "contributor may disagree" - if the contributor disagrees he/she MUST provide logical argument why and this is discussed. All developers involved should try to reach consensus. Worst case we vote. (Is voting really a good idea?)

Unless explicitly stated otherwise, requirements on public items are also recommendations for private items.

• If new type is added
  • This type is either private or required to be public for the change to be practical UBJ
  • If public
    • The name of the type adheres to Rust API guidelines (CMD)
    • The name of the type doesn't stutter (CMD but "I prefer stuttering" is not considered a logical argument)
    • The type does not implement traits the we could regret (mainly Copy, Eq, Ord, PartialOrd; rarely Sync, Send) UBJ
    • The type does implement obvious practical traits (Debug, probably also Clone, PartialEq, serde::{Serialize, Deserialize}) UBJ
    • The type is nameable (reexported) unless it intentionally can't be
    • If the type is obvious newtype giving read-only access to inner type it must implement AsRef<Inner> and Borrow<Inner>
      • If enum
        • Is it really, really certain that no varians will be removed? If not, hide the enum in a newtype struct.
  • If private
    • The type does not implement any traits that aren't actually used in the code. Debug is allowed though.
  • It is clearly stated whether the type enforces invariants
  • If the type enforces invariant(s)
    • Publicly observable invariants are documented
    • There is exactly one Rust-native constructor (E.g. StructName { fields }) in a method returning Result enforcing the invariant(s)
    • Rust-native constructors in infallible methods are only allowed if input types enforce the invariant(s) (presumably stricter)
    • Fields that aren't allowed to have all values for the invariant(s) to be upheld are strictly private.
    • The type is in its own private module with minimum code enforcing invariants
  • From<Inner> for T is not implemented
  • Enum or struct with all public fields has #[non_exhaustive] unless there's strong reason to believe it will never be changed UBJ
  • If the type is error type
    • Our requirements for error types are obeyed (will add them when hashed out)
  • serde impls branch on human-readable vs non-human-readable formats unless the serialization is obviously the same (e.g. numeric types)
  • serde impls use minimal serialization size unless there is widely-accepted representation (e.g. don't use base64 for things that are commonly hex even though hex is larger)
  • Default/new is only implemented for collections
  • Associated const (or a method) for ZERO is provided for non-collections. Possibly other states (ONE...)
  • If Drop is implemented
    • The implementation accesses all fields. If a field doesn't need to be accessed the dropped fields must be moved to a separate type
• If new function is added
  • Does it really have to be a method? Does it really have to be a free function? UBJ
  • The name adheres to the Rust API guidelines (CMD)
  • If the function could be easily abused it should have a scary name like dangerous_foo or foo_unchecked
  • All instances of panic!(), unwrap(), ... have explanation why they can't happen
  • All arithmetic operations either use checked_, wrapping_, saturating_ or have explanation why overflow is impossible
  • The function may only return error on bad inputs if it's a constructor enforcing invariants
  • If adding another type would be annoying for callers the function must panic on bad inputs and this panic MUST be documented in ## Panics doc section
  • For each argument the function accepts either of these holds:
    • The argument is Copy
    • Doesn't need to be owned
    • Needs to be owned and is accepted as owned (IOW no cloning of arguments in function unless required by multiple uses)
  • If the function is generic with a trivial conversion trait (AsRef, Into etc) then the function is trivial (call a non-generic helper if needed).
  • If the function is public
    • If the function returns Result the error conditions are documented in ## Errors doc section
    • impl Trait MUST NOT be used in arguments, use proper generic instead
    • Is it certain that the function will always return this type? If not either use impl Trait or a new custom type UBJ
  • The types of arguments have minimum requirements (IntoIterator is better than collection, &str is better than &String...)
  • Closure is the last argument, closure type is also the last type argument (closure return type is among the first)
  • The code has no obvious performance defects (unneeded clone(), collect::<Vec<_>>(), to_owned()...), they are acceptable in tests but discouraged
  • Tests are added, trivial obvious functions (returning or delegating to inner fields) don't need tests.
  • If the function processes untrusted inputs it must have fuzz test
• Conditional compilation is used
  • Is it truly additive?
  • Really? Are you sure? Try to "attack it" by finding a weird scenario
  • If the condition is observable in public API it must have doc attribute
• New cargo feature is added
  • The feature is documented in lib.rs
  • The feature is enabled in docs.rs
  • The feature is additive