Mural Pay integration #4520
Mural Pay integration #4520
Conversation
aecsocket
force-pushed
the
boris/muralpay
branch
from
847a50c to
9944f69
Compare
| #[serde(default)] | ||
| pub details: Vec<String>, | ||
| #[serde(default)] | ||
| pub params: HashMap<String, serde_json::Value>, |
There was a problem hiding this comment.
Using a foldhash::HashMap for a map that is not in the hot path (API errors are not expected to be that common) and will have a handful of entries at most is, in my view, a quite premature optimization, especially because it requires an extra dependency that's only ever used here. Over the long term, the maintenance overhead of keeping the dependency somewhat under control and up to date will most likely not be worth the nanoseconds saved.
There was a problem hiding this comment.
I agree that it's premature optimization, and it may even be less correct since it's less HashDoS resistant. I'll switch this to a std hashmap.
| pub fn deserialize<'de, D: serde::Deserializer<'de>>( | ||
| deserializer: D, | ||
| ) -> Result<CountryCode, D::Error> { | ||
| struct Visitor; | ||
|
|
||
| impl serde::de::Visitor<'_> for Visitor { | ||
| type Value = CountryCode; | ||
|
|
||
| fn expecting( | ||
| &self, | ||
| formatter: &mut std::fmt::Formatter, | ||
| ) -> std::fmt::Result { | ||
| write!(formatter, "an ISO 3166 alpha-2 country code") | ||
| } | ||
|
|
||
| fn visit_str<E>(self, v: &str) -> std::result::Result<Self::Value, E> | ||
| where | ||
| E: serde::de::Error, | ||
| { | ||
| rust_iso3166::ALPHA2_MAP.get(v).copied().ok_or_else(|| { | ||
| E::custom("invalid ISO 3166 alpha-2 country code") | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| deserializer.deserialize_str(Visitor) | ||
| } |
There was a problem hiding this comment.
You can greatly simplify this without introducing any additional copying by deserializing to a Cow<'_, str> with <Cow<'_, str>>::deserialize(deserializer), and then converting that to a CountryCode as you do in the visitor.
There was a problem hiding this comment.
I don't understand what you mean here. A CountryCode consists of
pub struct CountryCode {
///English short name
pub name: &'static str,
///Alpha-2 code
pub alpha2: &'static str,
///Alpha-3 code
pub alpha3: &'static str,
///Numeric code
pub numeric: i32,
}We map the &str to a code using rust_iso3166::ALPHA2_MAP which is a Map<&str, CountryCode>. It's not feasible to construct our own country code here.
| macro_rules! display_as_serialize { | ||
| ($T:ty) => { | ||
| const _: () = { | ||
| use std::fmt; | ||
|
|
||
| impl fmt::Display for $T { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| let value = | ||
| serde_json::to_value(self).map_err(|_| fmt::Error)?; | ||
| let value = value.as_str().ok_or(fmt::Error)?; | ||
| write!(f, "{value}") | ||
| } | ||
| } | ||
| }; | ||
| }; | ||
| } |
There was a problem hiding this comment.
This macro is only ever used once, so its presence introduces more things to parse through when reading the code (and opportunities for IDEs and other tooling to fail, as macro support can be a bit spotty at times) for no code boilerplate reduction benefit. I'd suggest removing this, as it's also very likely that no hypothetical developer interested in this will look at a package named muralpay and say "definitely this one has the macro display helper macro I'm looking for".
There was a problem hiding this comment.
I only used it once because I needed it for one specific purpose (FiatAndRailCode), but really this macro should be used on all of the C-like enums, since their Display impls should follow the same way they're serialized with serde.
3 participants
muralpaycrate for Mural Pay APIPOST /payout/feesto calculate fees for a payout - this is now the backend's responsibility