Create and serialize most Message kinds #37
Conversation
diehuxx
force-pushed
the
message-kinds-1
branch
from
7f58fe2
to
0c20c25
Compare
|
|
||
| assert_eq!( | ||
| close.data.reason, | ||
| Some("I don't want to do business with you any more".to_string()) |
| /// Current status of Order that's being executed | ||
|
|
||
| order_status: String, |
There was a problem hiding this comment.
nit: empty line between comment & field can be removed
| pub struct CloseData { | ||
| /// an explanation of why the exchange is being closed/completed | ||
| reason: Option<String>, | ||
| } |
There was a problem hiding this comment.
Just realized, we might want to not serialize empty fields. I'm not sure if this behavior is defined in the current tbDEX spec or not. On first glance, I don't think there's been a decision made.
Regardless, there's two ways I know of where we could skip serializing empty optional fields:
- Annotate the optional field with
#[serde(skip_serializing_if = "Option::is_none")] - Use the
serde_withcrate, which comes with a nice #[skip_serializing_none] macro
For reference, we're using the serde_with crate over in web5-rs, as it makes the code a little bit less messy, and maintains the readability a bit nicer.
Don't think we should take action on this right now, but I'll go ahead and make a task in the backlog for this.
There was a problem hiding this comment.
Good point! IMO we should not serialize None fields. I pushed a commit adding serde_with since it was a pretty easy additional. If we change our minds I'll strip it out.
| /// ISO 3166 currency code string | ||
| pub currency_code: String, | ||
| /// The amount of currency expressed in the smallest respective unit | ||
| pub amount_subunits: String, |
There was a problem hiding this comment.
Here and elsewhere, should the struct Deserialize the json into types specific to the semantics? For example, see below, where Decimal comes from the crate described in https://crates.io/crates/rust_decimal.
| pub amount_subunits: String, | |
| pub amount_subunits: Decimal, |
If not, where should that parsing and validation be done?
There was a problem hiding this comment.
Currently, the spec defines them as strings here: https://github.com/TBD54566975/tbdex/tree/main/specs/protocol
We also define them as strings in Javascript & Kotlin
Definitely open to changing, but we might want to modify the spec if this is something we want to do. For now, should be fine though because it meets the existing spec!
There was a problem hiding this comment.
I would say this is a language dependent matter. The spec mentions it's an amount, which I agree should be further specified. But we can likely accept that amount means that it's a number. So a value of "Not A Number" shouldn't be possible in this message.
Thinking from a consumers perspective, it would sure be nice that when using the library, I can only put values that are valid. Even better would be that any mistakes are caught at compile time, instead of at runtime.
I'm going to create two tickets for this. One to clarify the spec, the second to have more discussion around this.
There was a problem hiding this comment.
That's a good point. One thing I do want to mention though is that tbDEX messages/resources will need to traverse the language boundary from Rust -> other foreign languages. Strings work really well in this regard, third-party crate types not so much.
It's definitely going to be some tradeoffs, but I think we can find a happy medium here. Looking forward to discussing this more!
There was a problem hiding this comment.
@andresuribe87 There's ongoing discussion about currency amount representation in TBD54566975/tbdex#199. For now I'm going to stick with plain ol strings until we make a decision in the spec.
Implements
Close,Quote,Order, andOrderStatusmessages as currently defined in the spec https://github.com/TBD54566975/tbdex/tree/main/specs/protocol.