Resolve "Remove use of consensus controller in API, route requests via massa node." - [merged] #615
Comments
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo I still have tests to fix, but hopefully the channel magick is ok. I messed up a little bit the use of command senders is mostly reverting some parts of the first one. If there are some tips to make it clean, I would be happy to give it a try. @gterzian can you give me some feedback on event_tx usage ? Thanks |
|
In GitLab by @gterzian Commented on api/src/filters.rs line 811 Is the |
|
In GitLab by @gterzian Commented on api/src/filters.rs line 1026 Same question on |
|
In GitLab by @gterzian Commented on massa-node/src/main.rs line 72 nit: I would say it would be clearer to assign the first response to a variable, and then send it on the response sender. |
|
In GitLab by @gterzian Oui super! Juste quelque petit commentaire.... |
|
In GitLab by @AureliaDolo Commented on api/src/filters.rs line 811 No, it isn't. Removed it. |
|
In GitLab by @AureliaDolo Commented on api/src/filters.rs line 1026 Here it is necessary because it is also borrowed later when retrieving next slots for staker. |
|
In GitLab by @AureliaDolo Commented on massa-node/src/main.rs line 72 From what I understood on another occasion according to @damip it is better for performances to avoid intermediate variables. |
|
In GitLab by @AureliaDolo Commented on api/src/filters.rs line 811 changed this line in version 3 of the diff |
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @damip Commented on massa-node/src/main.rs line 72 Modern compilers should optimize it away, but I've heard from many programmers (C++ admittedly) that when readability is not hampered too much (i.e not too much nesting), it is a good practice to avoid intermediate variables. response_sender_tx.send(
consensus_command_sender
.get_active_block(hash)
.await
.expect("could not retrieve block")
).expect("could not send block");note that all error messages should be lowercase without a capital letter at the start and no punctuation at the end: rust-lang/rust#437 |
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo added 1 commit
|
|
In GitLab by @AureliaDolo Commented on api/src/tests/filter_tests.rs line 1079 I'm not sure at all with this pattern but it fixed the |
|
In GitLab by @AureliaDolo added 2 commits |
|
In GitLab by @AureliaDolo requested review from @gterzian |
|
In GitLab by @AureliaDolo marked this merge request as ready |
|
In GitLab by @AureliaDolo marked this merge request as draft |
|
In GitLab by @damip Commented on api/src/tests/filter_tests.rs line 1079 the cloned_xxx before an async move is completely acceptable :) |
|
In GitLab by @damip Commented on api/src/filters.rs line 1026 the functions retrieve_graph_export and similar should take a reference to event_tx, and not a moved version of it. This will eliminate the need for clone() here |
|
In GitLab by @gterzian Commented on massa-node/src/main.rs line 72 Even if it were slower to add a variable, how do you know if this would actually matter(be in the "hot path")? It might be a kind of premature optimization at this stage? On the other hand readability does matter at this stage, so it's up to you to decide what you prefer. I'm fine with the current code, although I would probably write it with the extra variables and I don't think it matters much. |
|
In GitLab by @gterzian Ok very good. The last things to do:
Regarding 2, I suggest you first try this on a copy of this branch, since I assume you haven't done this before. It goes like this:
|
|
In GitLab by @gterzian approved this merge request |
|
In GitLab by @gterzian Tiens pour le squash tu peux le faire automatiquement: https://docs.gitlab.com/ee/user/project/merge_requests/squash_and_merge.html |
|
In GitLab by @damip Commented on massa-node/src/main.rs line 72 I guess it boils down to personal preference in this simple case, because to me both options are just as readable. |
|
In GitLab by @gterzian Commented on api/src/filters.rs line 1026 Ok we can either file an issue to change Follow-up issue for things like that are quite nice, especially when we are open-source since this would be a great "good first issue" for beginners. |
|
In GitLab by @AureliaDolo Du coup, c'est bien toi qui va squasher pour de vrai ? |
|
In GitLab by @AureliaDolo Commented on api/src/filters.rs line 1026 I wrote a follow up issue #133. |
|
In GitLab by @damip approved this merge request |
|
In GitLab by @damip Commented on api/src/filters.rs line 1026 ok |
|
In GitLab by @damip marked this merge request as ready |
In GitLab by @AureliaDolo
Merges 127-remove-use-of-consensus-controller-in-api-route-requests-via-massa-node-2 -> master
Closes #127
The text was updated successfully, but these errors were encountered: