Skip to content
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

Removed ca from credential signatures. #456

Merged
merged 2 commits into from
Jun 5, 2024

Conversation

Randy808
Copy link
Collaborator

@Randy808 Randy808 commented May 29, 2024

If consensus is reached on this change, it should be merged after #454

@Randy808 Randy808 force-pushed the remove-ca-from-signature branch from 4626c18 to c947230 Compare May 31, 2024 15:01
nepet
nepet previously approved these changes Jun 4, 2024
Copy link
Collaborator

@nepet nepet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, only some minor complaints from the formatter on credentials.rs.

Diff in /home/runner/work/greenlight/greenlight/libs/gl-client-py/src/credentials.rs at line 182:
                 let d = creds.clone().with_ca(ca);
                 let inner = UnifiedCredentials::Device(d);
                 Self { inner }
-            },
+            }
         }
     }
 }
Diff in /home/runner/work/greenlight/greenlight/libs/gl-client/src/credentials.rs at line 193:
             cert: cert.into(),
             key: key.into(),
             rune: rune.into(),
-            ca
+            ca,
         }
     }
 
Diff in /home/runner/work/greenlight/greenlight/libs/gl-client/src/credentials.rs at line 200:
-    pub fn with_ca<V>(self, ca: V) -> Self 
+    pub fn with_ca<V>(self, ca: V) -> Self
     where
         V: Into<Vec<u[8](https://github.com/Blockstream/greenlight/actions/runs/9320196028/job/25656453262?pr=456#step:4:9)>>,
     {
Diff in /home/runner/work/greenlight/greenlight/libs/gl-client/src/credentials.rs at line 248:
     fn tls_config(&self) -> TlsConfig {
         tls::TlsConfig::with(&self.cert, &self.key, &self.ca)
     }
-
 }

@nepet
Copy link
Collaborator

nepet commented Jun 4, 2024

Oh and this conflicts with the backend tests: the fixtures there need to be fixed after we merged this

@cdecker
Copy link
Collaborator

cdecker commented Jun 4, 2024

Rebasing, merging, and then fixing up the backend to match this, once we have #408 merged as well (since that's the version the backend is working against atm).

@cdecker cdecker force-pushed the remove-ca-from-signature branch from c947230 to 5803c12 Compare June 4, 2024 16:08
@cdecker cdecker marked this pull request as ready for review June 5, 2024 09:32
@cdecker cdecker merged commit 50982b6 into Blockstream:main Jun 5, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants