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

Obtain features of a node #1062

Merged
merged 10 commits into from Nov 1, 2018

Conversation

Projects
None yet
4 participants
@kwek20
Copy link
Member

kwek20 commented Oct 16, 2018

Description

This PR adds a field called features to the getNodeInfo() API call.
It includes a list of features this node has enabled.

All features are calculated after construction of the node, and before loading of the node, through the Features enum. This enum also contains all the possible features a node can have.

Discussion: What should the actual feature list be?

Fixes #1039
Solves #854

Type of change

  • Enhancement (a non-breaking change which adds functionality)

How Has This Been Tested?

  • Ran getNodeInfo() and the expected features are send back
  • Unit tests do not break

Checklist:

  • My code follows the style guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
@chrisdukakis

This comment has been minimized.

Copy link
Member

chrisdukakis commented Oct 17, 2018

What do you think about a list of accessible commands? Simple as this:

{
    "accessibleCommands": [
        "getTransactionsToApprove",
        "attachToTangle",
        "storeTransactions",
        "etc..."
     ]
}
import com.iota.iri.conf.IotaConfig;

public enum Feature {
ATTACH_TO_TANGLE("attachToTange"),

This comment has been minimized.

Copy link
@jakubcech

jakubcech Oct 17, 2018

Contributor

attachToTange -> attachToTangle


for (String disabled : configuration.getRemoteLimitApi()) {
switch (disabled) {
case "attachToTange":

This comment has been minimized.

Copy link
@jakubcech

jakubcech Oct 17, 2018

Contributor

attachToTange -> attachToTangle

@jakubcech

This comment has been minimized.

Copy link
Contributor

jakubcech commented Oct 17, 2018

Not sure if this is expecting some more commits still. Just pointing out that this should have a form of features that are enabled, not just commands that are available. To make the contract clear, you should be able to query a node and discover that PoW is enabled in a clear way, regardless of the logic that enables/disables the remote PoW (which now is attachToTangle not in the remote limit list). Just listing that attachToTangle is available doesn't make that clear unless it's documented (which this should be as a scenario anyway). That's also why we just didn't go with a list of commands after some discussions @chrisdukakis.

Also, #1038 is about request limits.

@kwek20

This comment has been minimized.

Copy link
Member Author

kwek20 commented Oct 17, 2018

@jakubcech I just need to add comments and some test cases i think could be useful
The list of commands should definitely be useful, still don't know why we don't have that in the Api.java.
Not part of this PR though.

Good point on the Abstraction of feature names!

@iotaledger iotaledger deleted a comment from codacy-bot Oct 25, 2018

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Nice docs

Just a few changes please

* @param instance the instance of this node
* @return A list of features
*/
public static Feature[] calculateFeatures(Iota instance) {

This comment has been minimized.

Copy link
@GalRogozinski

GalRogozinski Oct 29, 2018

Member

Can this receive IotaConfig?

This comment has been minimized.

Copy link
@kwek20

kwek20 Oct 29, 2018

Author Member

It could be, but maybe some day we have to calculate features based on external factors outside of the config. Like only enable X if the database is up to date.

Or want to change to that when we have the need for it, and use config for now?

This comment has been minimized.

Copy link
@GalRogozinski

GalRogozinski Oct 29, 2018

Member

use configs for now
global scope is evil

/**
* Calculates all features for this Iota node
* @param instance the instance of this node
* @return A list of the features in readable name format

This comment has been minimized.

Copy link
@GalRogozinski

GalRogozinski Oct 29, 2018

Member

We are returning an array not a list


List<String> featureNames = Arrays.stream(features)
.map(feature -> feature.toString())
.collect(Collectors.toList());

This comment has been minimized.

Copy link
@GalRogozinski

GalRogozinski Oct 29, 2018

Member

it is better to do .toArray(Sting[]::new)

* @param instance the instance of this node
* @return A list of the features in readable name format
*/
public static String[] calculateFeatureNames(Iota instance) {

This comment has been minimized.

Copy link
@GalRogozinski

GalRogozinski Oct 29, 2018

Member

Can this receive IotaConfig?

It doesn't need the whole instance

@GalRogozinski
Copy link
Member

GalRogozinski left a comment

Can you also add the changes from commit from iotaledger/iri-regression#67 here?
The iri-regression is now part of this repository.

Thanks

Feature[] features = calculateFeatures(instance);

String[] featureNames = Arrays.stream(features)
.map(feature -> feature.toString()).toArray(String[]::new);

This comment has been minimized.

Copy link
@GalRogozinski

GalRogozinski Nov 1, 2018

Member

just a style thing I want to follow

Can you put the toArray on a new line? like it was before with collect

@@ -22,6 +22,7 @@ Feature: Test API calls on Machine 1
|appName |
|appVersion |
|duration |
|features |

This comment has been minimized.

Copy link
@GalRogozinski

GalRogozinski Nov 1, 2018

Member

indentation

kwek20 added some commits Nov 1, 2018

@iotaledger iotaledger deleted a comment from codacy-bot Nov 1, 2018

@GalRogozinski GalRogozinski merged commit bf06dfe into iotaledger:dev Nov 1, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.