issues 2721 #3122
issues 2721 #3122
Conversation
|
Hello @MerryPoppins92 ! |
|
Hello Adrien,
Thank you for reaching me and hope you are doing well!
I receive an email from github saying that the merge is not possible due to
some issues. I would like to do a code review with someone if it's possible
excellente journee
Merry
…On Mon, Oct 10, 2022 at 3:07 PM Adrien LF ***@***.***> wrote:
Hello @MerryPoppins92 <https://github.com/MerryPoppins92> !
Thanks for your contribution! Is your PR ready to be merged ?
—
Reply to this email directly, view it on GitHub
<#3122 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AQGCWHVRMSHSSZOCUANKX33WCRLM7ANCNFSM6AAAAAAQ5S5BUA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I think you need to merge the main branch (at least). |
There was a problem hiding this comment.
I don’t see any modification on the behavior of the bootstrap or anything. The issue ask that the bootstrap server use a whitelist to allow or not people to bootstrap from this server. Your code just modify the list that is used to bootstrap as a client.
Also the code have a lot of debug prints and dead code that should not be in a PR ready to review. The code doesn’t compile because not all modifications is pushed. The code isn’t formatted also and not clippy compatible
| @@ -23,9 +23,11 @@ use massa_pos_exports::{test_exports::assert_eq_pos_selection, PoSFinalState, Se | |||
| use massa_pos_worker::start_selector_worker; | |||
| use massa_signature::KeyPair; | |||
| use massa_time::MassaTime; | |||
| // use massa_node::load_file; | |||
There was a problem hiding this comment.
This code is dead and should be remove before merging (same for all the other dead code)
| @@ -205,10 +217,6 @@ async fn test_bootstrap_server() { | |||
| } | |||
| }); | |||
|
|
|||
| let sent_peers = wait_peers().await; | |||
There was a problem hiding this comment.
This change should be reverted as it's a fix since the change in #3109
| @@ -13,7 +13,7 @@ use crate::tests::tools::{ | |||
| use crate::BootstrapConfig; | |||
| use crate::{ | |||
| get_state, start_bootstrap_server, | |||
| tests::tools::{assert_eq_bootstrap_graph, get_bootstrap_config}, | |||
| tests::tools::{assert_eq_bootstrap_graph, get_bootstrap_config, get_bootstrap_config_for_wl}, | |||
There was a problem hiding this comment.
You imported this function that doesn't exist in our codebase you should push the modifications on tools.rs that has this change.
| @@ -197,6 +192,60 @@ pub struct ProtocolSettings { | |||
| pub max_endorsements_propagation_time: MassaTime, | |||
| } | |||
|
|
|||
| pub fn load_file() -> Vec<(SocketAddr, PublicKey)> { | |||
There was a problem hiding this comment.
This function seems not used and have a second version before so I think it should be removed.
| } | ||
|
|
||
| pub fn load_file2() -> Vec<SocketAddr> { | ||
| use std::fs::File; |
There was a problem hiding this comment.
Imports must be done at the top of the file
| @@ -180,15 +180,10 @@ pub struct ProtocolSettings { | |||
| /// Maximum number of batches in the memory buffer. | |||
| /// Dismiss the new batches if overflow | |||
| pub operation_batch_buffer_capacity: usize, | |||
| /// Maximum number of operations in the announcement buffer. | |||
| /// Immediately announce if overflow. | |||
| pub operation_announcement_buffer_capacity: usize, | |||
There was a problem hiding this comment.
This shouldn't be deleted
| @@ -601,6 +607,8 @@ struct Args { | |||
| /// Wallet password | |||
| #[structopt(short = "p", long = "pwd")] | |||
| password: Option<String>, | |||
| #[structopt(short = "b", long = "boot")] | |||
| boot: Option<String>, | |||
There was a problem hiding this comment.
Passing an argument to use a config file is complex for user. The most easy way for the user to do it it's like our config.toml. We create a file by default in your case whitelist: [] and then the user can edit it.
Sadly this doesn't match the requirements of the issue that ask this list to be live editable. I think a way to do it live executable is to use the API.
| @@ -649,6 +657,17 @@ async fn main(args: Args) -> anyhow::Result<()> { | |||
| .with(tracing_layer) | |||
| .init(); | |||
|
|
|||
| let x = match args.boot { | |||
| None => { | |||
There was a problem hiding this comment.
You can use is_some : https://doc.rust-lang.org/std/option/enum.Option.html#method.is_some to perform this conversion
|
|
||
| let bootstrap_list = match boot { | ||
| true => { | ||
| let x = load_file2(); |
There was a problem hiding this comment.
You are not using the return of the function that read the file you are just using the number of entries in it and you are generating new public key instead of using the ones in the file.
| @@ -0,0 +1,8 @@ | |||
| whitelist: | |||
There was a problem hiding this comment.
We use toml for our config file I think we should keep toml for consistency.
No description provided.