-
Notifications
You must be signed in to change notification settings - Fork 94
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
chore(release): v1.0.7-beta #1936
Changes from all commits
d4d5add
80f7e6f
7b29553
e333635
867a01a
54dce3c
92372cb
3cbb54d
483f04c
9d5ab11
410eda2
e4b091b
51c44f6
9a71744
96a53ce
1538564
1b10a06
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ use std::path::{Path, PathBuf}; | |
|
||
use crate::adex_proc::SmartFractPrecision; | ||
use crate::helpers::rewrite_json_file; | ||
#[cfg(unix)] use crate::helpers::set_file_permissions; | ||
use crate::logging::{error_anyhow, warn_bail}; | ||
|
||
const PROJECT_QUALIFIER: &str = "com"; | ||
|
@@ -22,6 +23,8 @@ const VOLUME_PRECISION_MIN: usize = 2; | |
const VOLUME_PRECISION_MAX: usize = 5; | ||
const VOLUME_PRECISION: SmartFractPrecision = (VOLUME_PRECISION_MIN, VOLUME_PRECISION_MAX); | ||
const PRICE_PRECISION: SmartFractPrecision = (PRICE_PRECISION_MIN, PRICE_PRECISION_MAX); | ||
#[cfg(unix)] | ||
const CFG_FILE_PERM_MODE: u32 = 0o660; | ||
|
||
pub(super) fn get_config() { | ||
let Ok(adex_cfg) = AdexConfigImpl::from_config_path() else { return; }; | ||
|
@@ -151,7 +154,12 @@ impl AdexConfigImpl { | |
let adex_path_str = cfg_path | ||
.to_str() | ||
.ok_or_else(|| error_anyhow!("Failed to get cfg_path as str"))?; | ||
rewrite_json_file(self, adex_path_str) | ||
rewrite_json_file(self, adex_path_str)?; | ||
#[cfg(unix)] | ||
{ | ||
set_file_permissions(adex_path_str, CFG_FILE_PERM_MODE)?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this advised? Wouldn't it be better/safer to assume config file has correct permission setting as opposed to implementing a de-facto "chmod" into mm2? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I advised him to this as it's creating the config JSON that is used by a separate process, mm2. There could be a better solution, but without this, the seed will be readable by any user on the system. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my original comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, it was adviced. The command: cc: @Alrighttt There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not totally necessary. Maybe a warning while creating this configuration would suffice since the target audience of this app is presumably power users or at least users familiar with a terminal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} | ||
Ok(()) | ||
} | ||
|
||
fn set_rpc_password(&mut self, rpc_password: String) { self.rpc_password.replace(rpc_password); } | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ use serde::{Deserialize, Serialize}; | |
use std::fs; | ||
use std::io::Write; | ||
use std::ops::Deref; | ||
#[cfg(unix)] use std::os::unix::fs::PermissionsExt; | ||
use std::path::Path; | ||
|
||
use crate::error_anyhow; | ||
|
@@ -22,6 +23,15 @@ where | |
writer | ||
.write(&data) | ||
.map_err(|error| error_anyhow!("Failed to write data into {file}: {error}"))?; | ||
|
||
Ok(()) | ||
} | ||
|
||
#[cfg(unix)] | ||
pub(crate) fn set_file_permissions(file: &str, unix_mode: u32) -> Result<()> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. duplicate feedback from above: is this advised? Wouldn't it be better/safer to assume config file has correct permission setting as opposed to implementing a de-facto "chmod" into mm2? |
||
let mut perms = fs::metadata(file)?.permissions(); | ||
perms.set_mode(unix_mode); | ||
fs::set_permissions(file, perms)?; | ||
Ok(()) | ||
} | ||
|
||
|
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.
cc @DeckerSU @Alrighttt
@rozhkovdmitrii why two diff versions? (we seem using a total of 3 diff across codebase,
0.19.1
,0.20.4
and0.20.8
)cli lockfile:
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.
jfyi:
dangerous_configuration
: this feature enables a dangerous() method on ClientConfig and ServerConfig that allows setting inadvisable options, such as replacing the certificate verification process. Applications requesting this feature should be reviewed carefully.assume this is for self-signed / local cert handling? cc @shamardy
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.
Yes. To disable certificate verification from cli side
dangerous_configuration
has to be usedkomodo-defi-framework/mm2src/adex_cli/src/transport.rs
Lines 106 to 108 in 867a01a
P.S.
dangerous_configuration
is not used from the https server side in mm2.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.
Thank you for pointing it out )
Originally the version of
rustls
was not constrained and "0.20.8" was used inadex-cli
.Version "0.19.1" is used as subdependency of `mm2_net`
On 03.08 I had to start using
rustls
as explicit dependency and I was oriented on usingrustls
0.20.4. It was a version which of mm2 was dependent on. Perhaps I had to strongly tie adex-cli on 0.20.4 to be able to manage versions manually.Now using both version "0.19.1" and "0.20.8" looks appropriate in my honest opinion
Concerning the question why
dangerous_configuration
feature was utilized - that was to make it able to connect tomm2
that issues self signed certificate.Thank you
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.
mm2 can be initialized using a certificate file too, it doesn't have to be self-signed but will be in most cases. It would be good to allow the cli user to disable certificate verification themselves like it's done in some other clients (e.g. postman provides this, it doesn't disable it by default). Please open an issue for this @rozhkovdmitrii and it can be done later as it's not urgent at all.
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.
done
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.
I just wanted to mention something about the versions of
hyper-rustls
andrustls
. From what I understand, the version ofrustls
that we use should exactly match the version ofrustls
thathyper-rustls
depends on. For example, if we are usinghyper-rustls 0.23
, we should also userustls 0.20.8
. This is important because if there is a version mismatch, such as usinghyper-rustls 0.23
andrustls 0.21.7
, we may encounter unexpected errors like below:To avoid any potential compatibility issues, it may be better to specify the exact versions of the crates using the
=
symbol. This way, we can ensure that the versions ofhyper-rustls
andrustls
are precisely matched, reducing the chances of encountering any compatibility problems. Perhaps I may have slightly overestimated the significance of the "issue", but I have personally encountered package version mismatches during some of my own tests. Anyway, JFYI.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.
Thank you for pointing it out 🙏, solved
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.
@rozhkovdmitrii can you please make this fix and the one here #1936 (comment) in a seperate PR? The whole release will be blocked until #1932 is sec reviewed and QA tested otherwise.
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.
done