Decode and encode Token V4 #73

2024-04-08T14:03:44Z

davidcaseria commented

2024-04-08 14:03:44 +00:00

(Migrated from github.com)

Closes #68

I have an open question on the draft NUT if secrets should be bytes instead of strings: https://github.com/cashubtc/nuts/pull/109/files#r1555859864. However, as it stands, this successfully decodes and encodes v4 tokens.

Closes #68 I have an open question on the draft NUT if secrets should be bytes instead of strings: https://github.com/cashubtc/nuts/pull/109/files#r1555859864. However, as it stands, this successfully decodes and encodes v4 tokens.

gitguardian[bot] commented

2024-04-08 14:04:23 +00:00

(Migrated from github.com)

⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secret in your pull request

GitGuardian id	GitGuardian status	Secret	Commit	Filename
-		Generic High Entropy Secret	`b1bf4506a0`	crates/cashu/src/nuts/nut00.rs	View secret

🛠 Guidelines to remediate hardcoded secrets

Understand the implications of revoking this secret by investigating where it is used in your code.
Replace and store your secret safely. Learn here the best practices.
Revoke and rotate this secret.
If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider

following these best practices for managing and storing secrets including API keys and other credentials
install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.

^{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!}

#### ⚠️ GitGuardian has uncovered 1 secret following the scan of your pull request. Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components. <details> <summary>🔎 Detected hardcoded secret in your pull request</summary> | GitGuardian id | GitGuardian status | Secret | Commit | Filename | | | -------------- | ------------------ | ------------------------------ | ---------------- | --------------- | -------------------- | | [-](https://dashboard.gitguardian.com/incidents/secrets) | | Generic High Entropy Secret | b1bf4506a0151e1b99faf9d7302b7e94f1a596c8 | crates/cashu/src/nuts/nut00.rs | [View secret](https://github.com/thesimplekid/cashu-crab/commit/b1bf4506a0151e1b99faf9d7302b7e94f1a596c8#diff-82e9399a8303c9726e14b4795951f4b3ebb5656a60bd8802cb3ce7c11dc131beL727) | </details> <details> <summary>🛠 Guidelines to remediate hardcoded secrets</summary> 1. Understand the implications of revoking this secret by investigating where it is used in your code. 2. Replace and store your secret safely. [Learn here](https://blog.gitguardian.com/secrets-api-management?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment) the best practices. 3. Revoke and [rotate this secret](https://docs.gitguardian.com/secrets-detection/secrets-detection-engine/detectors/generics/generic_high_entropy_secret#revoke-the-secret?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment). 4. If possible, [rewrite git history](https://blog.gitguardian.com/rewriting-git-history-cheatsheet?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment). Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data. To avoid such incidents in the future consider - following these [best practices](https://blog.gitguardian.com/secrets-api-management/?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment) for managing and storing secrets including API keys and other credentials - install [secret detection on pre-commit](https://docs.gitguardian.com/ggshield-docs/integrations/git-hooks/pre-commit?utm_source=product&utm_medium=GitHub_checks&utm_campaign=check_run_comment) to catch secret before it leaves your machine and ease remediation. </details> --- 🦉 [GitGuardian](https://dashboard.gitguardian.com/auth/login/?utm_medium=checkruns&utm_source=github&utm_campaign=cr1) detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. Our GitHub checks need improvements? [Share your feedbacks](https://form.typeform.com/to/KmeAPTMk)!

thesimplekid commented

2024-04-09 18:54:40 +00:00

(Migrated from github.com)

Thanks for this. Going to leave open for now as the NUT is still a draft

👍 1

lorenzolfm (Migrated from github.com) reviewed

2024-04-10 11:10:50 +00:00

crates/cashu/Cargo.toml

					
				@ -0,0 +39,4 @@

				serde_json = { workspace = true }

				serde_with = "3.4.0"

				url = { workspace = true }

				itertools = "0.12.0"

I would recommend always adding a dependency with the default-features set to false and than manually importing the features that you want to use in the project. This helps with:

Better control over dependencies
Less dependency conflicts
Reduced binary size
Improved compile times

For this specific one:

ciborium = { version = "0.2.2", default-features = false, features = ["std"] }

I would recommend always adding a dependency with the default-features set to `false` and than manually importing the features that you want to use in the project. This helps with: * Better control over dependencies * Less dependency conflicts * Reduced binary size * Improved compile times For this specific one: ```suggestion ciborium = { version = "0.2.2", default-features = false, features = ["std"] } ```

lorenzolfm (Migrated from github.com) reviewed

2024-04-10 11:29:14 +00:00

crates/cashu/src/nuts/nut00.rs

					
				@ -0,0 +363,4 @@

				                for proof in &proofs.proofs {

				                    amount += proof.amount;

				                }

				            }

should this println! be here?

should this `println!` be here?

lorenzolfm (Migrated from github.com) reviewed

2024-04-10 11:29:55 +00:00

crates/cashu/src/nuts/nut00.rs

					
				@ -0,0 +411,4 @@

				                token: token.token.into_iter().map(Into::into).collect(),

				                memo: token.memo,

				                unit: token.unit,

				            }

Is there a specific reason to use ciborium here? woudn't serde_json::from_slice be enough?

Is there a specific reason to use ciborium here? woudn't `serde_json::from_slice` be enough?

lorenzolfm (Migrated from github.com) reviewed

2024-04-10 11:36:06 +00:00

crates/cashu/src/nuts/nut00.rs

					
				@ -0,0 +363,4 @@

				                for proof in &proofs.proofs {

				                    amount += proof.amount;

				                }

				            }

            let (is_v3, s) = match (s.strip_prefix("cashuA"), s.strip_prefix("cashuB")) {
                (Some(s), None) => (true, s),
                (None, Some(s)) => (false, s),
                _ => return Err(wallet::Error::UnsupportedToken),
            };

```suggestion let (is_v3, s) = match (s.strip_prefix("cashuA"), s.strip_prefix("cashuB")) { (Some(s), None) => (true, s), (None, Some(s)) => (false, s), _ => return Err(wallet::Error::UnsupportedToken), }; ```

lorenzolfm (Migrated from github.com) reviewed

2024-04-10 11:42:02 +00:00

crates/cashu/src/nuts/nut00.rs

					
				@ -0,0 +363,4 @@

				                for proof in &proofs.proofs {

				                    amount += proof.amount;

				                }

				            }

i highly discourage using unwrap.

Recommend going with expect and adding useful log to the user

i highly discourage using `unwrap`. Recommend going with `expect` and adding useful log to the user

lorenzolfm (Migrated from github.com) reviewed

2024-04-10 11:46:23 +00:00

crates/cashu/src/nuts/nut02.rs

					
				@ -0,0 +47,4 @@

				    const STRLEN: usize = 14;

				    pub fn to_bytes(&self) -> Vec<u8> {

				        hex::decode(self.to_string()).unwrap()

maybe go with expect instead of unwrap if changing the function signature is not an option

maybe go with `expect` instead of `unwrap` if changing the function signature is not an option

thesimplekid (Migrated from github.com) reviewed

2024-04-10 12:21:15 +00:00

crates/cashu/src/nuts/nut02.rs

					
				@ -0,0 +47,4 @@

				    const STRLEN: usize = 14;

				    pub fn to_bytes(&self) -> Vec<u8> {

				        hex::decode(self.to_string()).unwrap()

i would rather we change the function signature to a result

thesimplekid (Migrated from github.com) reviewed

2024-04-10 12:26:16 +00:00

crates/cashu/src/nuts/nut00.rs

					
				@ -0,0 +363,4 @@

				                for proof in &proofs.proofs {

				                    amount += proof.amount;

				                }

				            }

I agree unwrap should only be allowed within tests. Ideally a specific and useful error is returned

thesimplekid (Migrated from github.com) reviewed

2024-04-10 12:27:15 +00:00

crates/cashu/src/nuts/nut00.rs

					
				@ -0,0 +363,4 @@

				                for proof in &proofs.proofs {

				                    amount += proof.amount;

				                }

				            }

Lets remove the print an output is needed it should be a log message not print.

thesimplekid (Migrated from github.com) reviewed

2024-04-10 12:28:07 +00:00

crates/cashu/src/nuts/nut00.rs

					
				@ -0,0 +564,4 @@

				    }

				}

				impl From<ProofV4> for Proof {

I think these can be removed now that your question on the Nut is resolved?

thesimplekid (Migrated from github.com) requested changes

2024-04-10 12:30:37 +00:00

thesimplekid (Migrated from github.com) left a comment

Just a few small changes. @lorenzolfm called out around the use of unwrap

lorenzolfm (Migrated from github.com) reviewed

2024-04-10 13:16:26 +00:00

crates/cashu/src/nuts/nut02.rs

					
				@ -0,0 +47,4 @@

				    const STRLEN: usize = 14;

				    pub fn to_bytes(&self) -> Vec<u8> {

				        hex::decode(self.to_string()).unwrap()

then maybe the function name should be try_into_bytes 🤔

then maybe the function name should be `try_into_bytes` :thinking:

davidcaseria (Migrated from github.com) reviewed

2024-04-12 18:42:37 +00:00

crates/cashu/src/nuts/nut00.rs

					
				@ -0,0 +411,4 @@

				                token: token.token.into_iter().map(Into::into).collect(),

				                memo: token.memo,

				                unit: token.unit,

				            }

The decoded bytes are CBOR not JSON.

davidcaseria (Migrated from github.com) reviewed

2024-04-12 18:47:35 +00:00

crates/cashu/src/nuts/nut02.rs

					
				@ -0,0 +47,4 @@

				    const STRLEN: usize = 14;

				    pub fn to_bytes(&self) -> Vec<u8> {

				        hex::decode(self.to_string()).unwrap()

I don't think we should change the signature because this should never fail. However, the only reason it can fail now is that we store the hex string instead of the bytes in this object. Ideally, the Id struct should hold the bytes. Should I mark this as a TODO for now and follow up with a refactor shortly?

I don't think we should change the signature because this _should_ never fail. However, the only reason it can fail now is that we store the hex string instead of the bytes in this object. Ideally, the `Id` struct should hold the bytes. Should I mark this as a TODO for now and follow up with a refactor shortly?

thesimplekid (Migrated from github.com) reviewed

2024-04-12 19:29:50 +00:00

crates/cashu/src/nuts/nut02.rs

					
				@ -0,0 +47,4 @@

				    const STRLEN: usize = 14;

				    pub fn to_bytes(&self) -> Vec<u8> {

				        hex::decode(self.to_string()).unwrap()

Yes that makes sense

davidcaseria commented

2024-04-12 20:07:33 +00:00

(Migrated from github.com)

Closing in favor of new PR given the major code refactor.

thesimplekid commented

2024-04-12 20:45:18 +00:00

(Migrated from github.com)

Closing in favor of new PR given the major code refactor.

Yeah sorry about that

> Closing in favor of new PR given the major code refactor. Yeah sorry about that

davidcaseria commented

2024-04-13 10:39:33 +00:00

(Migrated from github.com)

No worries!

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

thesimplekid

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

cashubtc/cdk!73

Rows
Columns