Bring resource and offering data structures up to latest spec #56
Conversation
KendallWeihe
requested review from
nitro-neal,
kirahsapong,
shamilovtim,
diehuxx,
decentralgabe and
frankhinek
as code owners
|
|
||
| /// Struct that interacts with an [`Offering`] [`Resource`] | ||
| pub struct Offering; | ||
|
|
||
| /// Struct for passing parameters to [`Offering::create`] | ||
| #[derive(Debug, Default)] |
There was a problem hiding this comment.
you mean to leave this Debug flag in?
There was a problem hiding this comment.
It's a good callout. The Debug attribute makes it such that the struct automatically implements the std::fmt::Debug trait. The primary use case is for print statements wherein the developer can use {:?} which will print the struct in human readable form. It's totally safe to do this even in production code. The only place we won't want to do this is in places which we know will include PII data. Even then it would be fine but we would reduce the likelihood of accidental abuse, where developers may accidentally log PII.
| pub currency_code: String, | ||
| /// Minimum amount of currency that the offer is valid for | ||
| pub min_subunits: Option<String>, | ||
| pub min: Option<String>, |
There was a problem hiding this comment.
| min | DecimalString | N | minimum amount required to use this payment method.
Is it supposed to be a DecimalString and not string? or they are one in the same?
Just for my understanding, I'm sure this has been thought of a long time ago and I missed it:
from to docs:
DecimalString
Currency amounts have type DecimalString, which is string containing a decimal amount of major currency units. The decimal separator is a period .. The currency symbol must be omitted.
I'm not sure but I have a feeling that most other financial apps use a uint or something to count the satoshi or cents or pesos at the lowest level
Chad says:
To address these issues, a better approach would typically involve using a specialized numeric type designed for handling monetary values. In Rust, you might consider using types like:
i64 or u64: These types can store monetary values as the smallest units of the currency (like cents in USD) to avoid precision issues with floating points.
There was a problem hiding this comment.
I created a ticket for this #53
Basically, it needs to be of type string, but we may be able to add a slightly more constrained type which prohibits non-decimal values
KendallWeihe
force-pushed
the
kendall/resources-latest-structs
branch
from
1874714
to
bf81765
Compare
Closes #55
Some implications in these changes which will span to subsequent changes (please object if you take issue):
Option<T>type) because that isn't how they exist in the spec. this implies that during thecreate()we will create the signatures (that is TODO, added comment linking to relevant ticket).create(options: CreateOptions)paradigm as opposed to inlining all the parametersrequired_payment_details_schemawhich may become relevant again once we address Consider strongly typing JSON Schema types, or using an existing open source lib #54 but for now it's not relevantCreated tickets for subsequent work during this work: