added review comments

src/lib.rs

@@ -6,24 +6,40 @@ use base64::{engine::general_purpose, Engine as _};
 #[derive(Clone, Deserialize, Debug, Serialize)]
 pub struct Token {
     pub user_id: String,
-    pub scope: String // I don't really know what this means?
+ 
+    // The concept here is that scope is a permission
+    // This can be better handled by an enum
+    pub scope: String
 }
 
 pub struct SignedToken {
     pub token: Token,
     pub issued_at: u64,
-    pub signature: String, // why not just store the bytes?
+    // we are storing the String, which is good for transport,
+    // but not for in-memory use.
+    // instead use Vec<u8> and convert to String for serialization
+    pub signature: String,
 }
 
 impl Token {
+    // todo: weird, new should return Self. Let's not mix parsing with domain model
     pub fn new(json: &str) -> Result<Token,serde_json::Error> {
         serde_json::from_str(json)
     }
 }
 
+// should Signer abstraction so we can encapsulate signing data, like private_key
+// Allow policy evolution without API breakage
 pub fn sign(token: &Token, private_key: &SigningKey) -> SignedToken {
-    let issued_at = 1710000000; // example timestamp
+    let issued_at = 1710000000; // todo: get now timestamp
+
+    // we didn't include any kind of versioning here, which might be helpful
+    // for schema changes in the protocol
     let data = (&token, issued_at);
+
+    // Canonicalization problem
+    // here we are signing the serialization of JSON!
+    // which could be unstable across versions or client/server implementations
     let bytes = serde_json::to_vec(&data).unwrap();
     let signature = private_key.sign(&bytes);
     SignedToken {
@@ -34,16 +50,21 @@ pub fn sign(token: &Token, private_key: &SigningKey) -> SignedToken {
     }
 }
 
+// return type is not ideal for error handling
+// Use error type enum for more structured handling
+// Also, use Verifier abstraction for similar reasons as Signer abstraction
 pub fn verify(signed_token: &SignedToken, public_key: &VerifyingKey) -> Result<(),String> {
     let data = (&signed_token.token, signed_token.issued_at);
-    let bytes = serde_json::to_vec(&data).unwrap();
+    let bytes = serde_json::to_vec(&data).unwrap(); // todo: handle, likely to panic
+    
+    // todo: forgot to validate issued_at
 
     // converts from base64
     let sig_bytes = general_purpose::STANDARD
         .decode(&signed_token.signature)
         .map_err(|e| e.to_string())?;
 
-    // can we guarantee length of 64?
+    // Ed25519, signatures are always 64 bytes
     let sig_array: [u8; 64] = sig_bytes
         .try_into()
         .map_err(|_| "invalid signature length".to_string())?;