-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chainmail: implement save/unsave message #855 #856
base: main
Are you sure you want to change the base?
Conversation
…table definition out of the service mod
…ST handler rework for msg retrieval by id
doc/psidk/src/development/services/rust-service/reference/chicken-scratch.md
Outdated
Show resolved
Hide resolved
…nted; doing check-in before restructuring query call function for better partitioning and maintenance
…iting before extending deser to datetime field
@cool-ant There is an unhandled TODO in |
subject: msg.subject.clone(), | ||
body: msg.body.clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is clone needed here? msg
goes out of scope right after this, so you should be able to move from it.
/// | ||
/// Unsaving a message removing it from state, exposing it to pruning | ||
fn unsave(msg_id: u64) -> Result<(), Error> { | ||
let msg = get_msg_by_id(msg_id).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why call unwrap
instead of ?
?
serde_structs::TempMessageForDeserialization, | ||
}; | ||
|
||
pub fn get_msg_by_id(msg_id: u64) -> Result<TempMessageForDeserialization, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the struct is called TempMessageForDeserialization
, I would use it only for deserialization and immediately convert it to Message
for use everywhere else.
if msg.len() == 1 { | ||
let msg = msg.get(0).unwrap(); | ||
return Ok(msg.clone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary clone. pop
or remove
can extract an item from a Vec
without copying it.
if archived_requested { | ||
endpoint += "archived=true&"; | ||
} | ||
if sender.is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if let Some(sender) = &sender
is better if you need to both test and unwrap.
} | ||
|
||
let mut r_clause = String::new(); | ||
let r_opt = params.get(&String::from("receiver")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to create a new String
here.
} else { | ||
"" | ||
}, | ||
if params.contains_key("id") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have both id
and either sender
or receiver
? I think that combination will produce AND AND
.
/// - msg_id: the message's rowid (from the events table) | ||
/// | ||
/// Archiving is equivalent to deleting, given message can't be truly deleted. | ||
/// Archiving is a proactive action equivalent to a node pruning an message (an message's historical event) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would probably be better if these comments were in the wit document. Docs in wit
are actually extractable from the raw wasm, and therefore can be used to display docs for a plugin without source code access
@@ -6,7 +6,8 @@ interface types { | |||
receiver: string, | |||
sender: string, | |||
subject: string, | |||
body: string | |||
body: string, | |||
datetime: u32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This datetime
should be converted to a utc string before returning it to the UI, so the UI can easily construct a Date
with it. You can check the invite plugin to see how I already did this over there:
let expiry = DateTime::from_timestamp(invite.expiry as i64, 0)
.ok_or(DatetimeError.err("decode_invite"))?
.to_string();
|
||
let itr = query.split("&"); | ||
for p in itr { | ||
let kv = p.split_once("=").unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using expect
here to give just a little more info in case the user serializes the request wrong
datetime: u32, | ||
) { | ||
check( | ||
get_sender() == receiver, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check doesn't actually work. A client could simply spoof the receiver and we can't verify it since we can't read the events table. The implication here is that anyone can save anything, including fake/altered messages.
In the future we could address this by having emails be signed, which would allow anyone to verify that the sender's signing key was used to sign the saved email.
/// Unsave a message | ||
/// `unsave` releases the state storage for a previously saved message | ||
#[action] | ||
fn unsave(msg_id: u64, sender: AccountNumber, subject: String, body: String, datetime: u32) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, this can be removed from the action interface, and simply done as part of the archive action
pub body: String, | ||
#[serde(deserialize_with = "deserialize_timepoint")] | ||
pub datetime: u32, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, is it possible to deserialize with either:
- The
SavedMessage
struct in the service, or - The generated
Message
object from your bindings?
Just seems a bit strange to essentially have three definitions of a Message type... Maybe it's needed though?
Implements/fulfills #855