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

Implemented parity_set #198

Merged
merged 13 commits into from
Feb 15, 2019
Merged

Implemented parity_set #198

merged 13 commits into from
Feb 15, 2019

Conversation

jordipainan
Copy link
Contributor

  • parity_set tested on a dev node
  • everything working except parity_setTransactionsLimit (issue opened)
  • setTxGasLimit is deprecated (node ignores the request, but stills in wiki so we can remove when docs upgrade)
  • setMinGasPrice is deprecated (node ignores the request, but stills in wiki so we can remove when docs upgrade)

All methods are implemented but 1 not working and 2 deprecated. I took the approach to implement everything but maybe you think its better to remove all 3. We can remove code so its fine :)

A big thank you to @CPerezz helping me with return values ^^

Copy link
Owner

@tomusdrw tomusdrw 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, few minor issues

src/api/mod.rs Outdated
@@ -18,6 +19,7 @@ pub use self::parity_accounts::ParityAccounts;
pub use self::personal::Personal;
pub use self::web3::Web3 as Web3Api;
pub use self::traces::Traces;
pub use self::parity_set::ParitySet;
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep sorted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry! Fixed

impl<T: Transport> Namespace<T> for ParitySet<T> {
fn new(transport: T) -> Self
where
Self: Sized,
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think the bound is required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct! Works well too if bound is not present.

impl<T: Transport> ParitySet<T> {

/// Set Parity to accept non-reserved peers (default behavior)
pub fn parity_accept_non_reserved_peers(&self) -> CallFuture<bool, T::Out> {
Copy link
Owner

Choose a reason for hiding this comment

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

Can we drop the parity_ prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! With parity_ prefix its a bit repetitive :)

Copy link
Owner

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

Awesome, thank you!

@tomusdrw tomusdrw merged commit fc68ec1 into tomusdrw:master Feb 15, 2019
@HCastano
Copy link
Contributor

Hey, just as an FYI setMinGasPrice isn't deprecated anymore (see openethereum/parity-ethereum#10294).

@jordipainan
Copy link
Contributor Author

Thank you!! ^^

@blanksteer
Copy link
Contributor

Am i good to add peer listing to ParitySet or is that a function most would rather not have?

@jordipainan
Copy link
Contributor Author

jordipainan commented Mar 6, 2019

Hi! Sorry for the delay answering. In my own opinion I think peer_listing fits better on the net namespace.
I see parity_set as a namespace for configure some aspects of the client and peer_listing returns some info about the client state and it can be a bit confusing because is something regarding to the network.
What do you think @blanksteer @tomusdrw ?

@CPerezz
Copy link
Contributor

CPerezz commented Mar 6, 2019

I agree with @jordipainan . It would probably fit better on anothre namespace like net where we also have the peer_count feature.

@tomusdrw
Copy link
Owner

tomusdrw commented Mar 6, 2019

I treat namespaces as structs that correspond to the first part of the RPC method name (in this case parity_netPeers).

Since it might make sense to logically group some methods together (like parity_netPeers and parity_startNetwork, parity_netPort) but that grouping would be completely arbitrary for rust-web3. Also it makes it really confusing where the methods should be searched for.

I'd prefer to keep a simple rule of thumb where methods are placed in the codebase, moving arbitrary methods between the namespaces doesn't seem like a good idea.

@jordipainan
Copy link
Contributor Author

I understand your point of view, perhaps it is easier for me to find the methods the way I have described, but if in general you think that this approach can lead to confusion and does not match with what was previously developed, go ahead I trust in your criteria :)

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.

5 participants