fix broken getInclusionStates API call #1685

acha-bill · 2019-12-16T12:12:43Z

Description

Updates getInclusionStates API call to only check the snapshot index of the given transactions.
Tips are therefore irrelevant.

Fixes #1350

Type of change

Bug fix (a non-breaking change which fixes an issue)

How Has This Been Tested?

All existing unit tests pass
API regression tests pass

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
New and existing unit tests pass locally with my changes

GalRogozinski · 2019-12-18T16:03:19Z

python-regression/tests/features/machine1/1_api_tests.feature

@@ -154,7 +154,7 @@ Feature: Test API calls on Machine 1

 		Then the response for "getInclusionStates" should return with:
 		|keys			|values			|type               |
-		|states			|False		        |bool               |
+		|states			|True		        |bool               |


really?
We will wait for @DyrellC to say whether this makes sense and wait for buildkite

Btw, @DyrellC, this is what I was talking to you about needing to to properly document the DBs, currently it changed and I have no idea why by just looking at the code.

Besides that, @acha-bill, maybe it is worth while to have another scenario (or step) in this PR for getInclusionStates. We want to have both the True and False cases.
We also want to make sure that we can accept a list of Hashes and not just one single hash.
You can wait for @DyrellC to explain to you what is going on with the DB

@galrogo this instance was actually the db that you had provided in the previous regression tests, but it does highlight this point yes.
@acha-bill since the getInclusionStates call will no longer need the tip list, you can remove that from the input side of the test. You could also add the list example that @galrogo is asking for with a 2 for 1 by having another scenario that tests inclusion state on a list of randomly generated hashes and having that return a boolList of false. That would cover testing a list scenario as well as making sure we have false cases register false as well.

@DyrellC The python lib throws expected 3 args for getInclusionStates.
So we'll leave the tips param there for now.

Hmm I am adding communicating with clients to checklist

GalRogozinski · 2019-12-18T16:04:32Z

src/main/java/com/iota/iri/service/API.java

-    private AbstractResponse getInclusionStatesStatement(
-            final List<String> transactions,
-            final List<String> tips) throws Exception {
+    private AbstractResponse getInclusionStatesStatement(final List<String> transactions ) throws Exception {


did you test what happens when someone passes tips still?
(it should work, but just checking)

works. No difference

DyrellC

Just the commentary on the test itself for adding the additional use case, i'll leave it to @galrogo discretion as to whether to add that in this issue or as a future PR.

DyrellC · 2020-01-13T22:14:54Z

python-regression/tests/features/machine1/1_api_tests.feature

@@ -154,7 +154,7 @@ Feature: Test API calls on Machine 1

 		Then the response for "getInclusionStates" should return with:
 		|keys			|values			|type               |
-		|states			|False		        |bool               |
+		|states			|True		        |bool               |


@galrogo this instance was actually the db that you had provided in the previous regression tests, but it does highlight this point yes.
@acha-bill since the getInclusionStates call will no longer need the tip list, you can remove that from the input side of the test. You could also add the list example that @galrogo is asking for with a 2 for 1 by having another scenario that tests inclusion state on a list of randomly generated hashes and having that return a boolList of false. That would cover testing a list scenario as well as making sure we have false cases register false as well.

GalRogozinski

I want to expand the scenario
Let's finish this feature correctly

GalRogozinski · 2020-01-14T11:00:02Z

python-regression/util/static_vals.py

 TEST_TIP_LIST = ["SBKWTQWCFTF9DBZHJKQJKU9LXMZD9BMWJIJLZCCZYJFWIBGYYQBJOWWFWIHDEDTIHUB9PMOWZVCPKV999"]
+TEST_HASH_LIST = ["NMPXODIWSCTMWRTQ9AI9VROYNFACWCQDXDSJLNC9HKCECBCGQTBUBKVROXZLQWAZRQUGIJTLTMAMSH999",
+                  "INVALIDWSCTMWRTQ9AI9VROYNFACWCQDXDSJLNC9HKCECBCGQTBUBKVROXZLQWAZRQUGIJTLTMAMSH999"]


I suppose the second tx is a made up transaction.
Can we also add a real tx that is not included?

I think the scenarios should include:

A bundle tail that is approved

A bundle tail that is not approved

A non-tail that is approved

A non-tail that is not apporved

a made up tx

Dropping scenario 2 and 4 as agreed with @galrogo .

GalRogozinski · 2020-01-14T11:03:27Z

python-regression/tests/features/machine1/1_api_tests.feature

@@ -154,7 +154,18 @@ Feature: Test API calls on Machine 1

 		Then the response for "getInclusionStates" should return with:
 		|keys			|values			|type               |
-		|states			|True		        |bool               |
+		| states | TEST_HASH_RES | staticValue |


Only in case it is not too much work (no more than 5-10 minutes)
I think that maybe for the responses we can put the actual list in the table instead of the static val.
I just don't like it that we mask what's going on too much.

With long hashes there is less of a choice because they are long and ugly...

If this requires work than just comment on the reply and ignore

You can just use a boolList type here instead of loading a staticValue. Bool list will make an array of bools to compare with. So all you need to do is change the line to:
|states | True | boolList |
see line 113 in python-regression/utils/test_logic/value_fetch_logic.py

GalRogozinski · 2020-01-14T11:03:47Z

python-regression/tests/features/machine1/1_api_tests.feature

+
+	Then the response for "getInclusionStates" should return with:
+	    | keys   | values        | type        |
+	    | states | TEST_RESPONSE | staticValue |


see above comment, same thing

@DyrellC add new type boolListMixed cause boolList could only convert 1 type.

Lovely 👌

DyrellC

Looks good now

DyrellC

New scenario addition looks good.

fix broken getInclusionStates API call

aeb8b5e

GalRogozinski reviewed Dec 18, 2019

View reviewed changes

GalRogozinski requested a review from DyrellC December 19, 2019 08:37

GalRogozinski added the L-Awaiting More Reviews Lifecycle - Every PR must have at least 2 reviewers label Dec 26, 2019

GalRogozinski mentioned this pull request Jan 6, 2020

Prepare IRI release v.1.9.0 #1703

Open

DyrellC suggested changes Jan 13, 2020

View reviewed changes

added scenario

4659c61

GalRogozinski suggested changes Jan 14, 2020

View reviewed changes

Merge branch 'dev' into fix-getInclusionStates

9a1de5f

lzpap mentioned this pull request Jan 15, 2020

Remove tips parameter from get_inclusion_states API call iotaledger/iota.py#288

Closed

acha-bill added 2 commits January 16, 2020 11:52

added more scenarios

de8d153

use boolList type for getInclusionState

6b8f4c7

DyrellC approved these changes Jan 16, 2020

View reviewed changes

DyrellC and others added 2 commits January 16, 2020 09:12

Merge branch 'dev' into fix-getInclusionStates

5f08167

added scenario

3546a22

GalRogozinski approved these changes Jan 17, 2020

View reviewed changes

DyrellC approved these changes Jan 17, 2020

View reviewed changes

Merge branch 'dev' into fix-getInclusionStates

fd12d04

GalRogozinski removed the L-Awaiting More Reviews Lifecycle - Every PR must have at least 2 reviewers label Jan 20, 2020

GalRogozinski merged commit 7cdc6db into iotaledger:dev Jan 20, 2020

GalRogozinski mentioned this pull request May 6, 2020

Release 1.8.6 #1851

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix broken getInclusionStates API call #1685

fix broken getInclusionStates API call #1685

acha-bill commented Dec 16, 2019

GalRogozinski Dec 18, 2019

acha-bill Dec 19, 2019

GalRogozinski Dec 19, 2019

DyrellC Jan 13, 2020

acha-bill Jan 14, 2020

GalRogozinski Jan 14, 2020

GalRogozinski Dec 18, 2019

acha-bill Dec 19, 2019

DyrellC left a comment

DyrellC Jan 13, 2020

GalRogozinski left a comment

GalRogozinski Jan 14, 2020

acha-bill Jan 16, 2020

GalRogozinski Jan 14, 2020

DyrellC Jan 14, 2020

GalRogozinski Jan 14, 2020

DyrellC Jan 14, 2020

acha-bill Jan 16, 2020

DyrellC Jan 16, 2020

DyrellC left a comment

DyrellC left a comment

fix broken getInclusionStates API call #1685

fix broken getInclusionStates API call #1685

Conversation

acha-bill commented Dec 16, 2019

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DyrellC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GalRogozinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DyrellC left a comment

Choose a reason for hiding this comment

DyrellC left a comment

Choose a reason for hiding this comment