Network testing: Nicaea proposals- leader management and mint capacity

[gleb-urvanov]

implementation for following issues:
#683
#684
#685
#657

Depends on:
#782
#919

[mnaamani]

can you merge nicaea to make this easier to review also

[mnaamani]

I've left some comments and request for changes.
Overall the tests are running through till the end successfully.
I noticed that the nicaea tests don't cover the content working directory proposals. It is still part of nicaea runtime so they should not be left out. Please correct me if I'm wrong.

[mnaamani]

I think passing in all values to the constructor would be a better pattern than depending on user to call each individueal set method. They may forget to set some values. Unless ofcourse you need to set the values at different times, in which case its probably not a good idea to bundle all the values in this single class.

[gleb-urvanov]

Going to disagree with that point- one of the reasons why we have introduced a data transfer object is to mitigate the risk of passing the values in the wrong order: #782 (comment)

If all the values will be passed to the constructor, that goal will not be achieved- instead of risk having the wrong order in the function call, we will have the same risk but for the constructor call.

[mnaamani]

Yes as in the original comment you linked to I wasn't implying re-introducing a long list of arguments. But this approach here of depending on methods might leave some of the private fields un-initialized?

[gleb-urvanov]

Yes, it might. Normally typescript won't allow that, that is why assignment assertion (exclamation sign) is used. What is the best approach you can suggest?

[mnaamani]

Yes so to this point exactly, I guess the approach here mainly solves problem of passing arguments in wrong order.
So passing an object was the correct solution, except now this new object can have null or underfined fields. So wouldn't simply passing arguments like so be the optimal approach:

// original approach which I advised against
function withHugeArgsList(a, b, c, ...., z) {

}

// suggested method, takes a single object instead
function theMethod({ arg_a: A, arg_b: B, arg_c: C }) {
   do_something_with_args()
}

// calling the method
theMethod({arg_a: valueOfTypeA, arg_b: valueOfTypeB, ...})

This way will insure no incorrect ordering of arguments, and that all args are provided (optionally give some args a default value if you want them to be optional)

[gleb-urvanov]

Ok, will do.

[mnaamani]

in all these expectSomeEvent() methods, there is no need for async/await the way it is written, since we are relying on callback and then calling resolve(). Infact I think the api.query.system.events() method returns a promise for a function to unsubscribe from listening to events, which we should probably use before resolve(). As promises work it will only ever resolve to the first event found, but there is unnecessary events still being processed up until the api disconnects.

[gleb-urvanov]

Done in #1001 , although it is worth to look at the solution. With unsubscribe introduction we have to await for unsubscription promise, thus the await is gaining sense. Please correct me if im wrong.

[mnaamani]

Well yes if you are actually going to use it, then you need the await :)
And I see you did refactor it in #1001 as you mentioned. 👍

[mnaamani]

my typescript knowledge is a bit rusty, does the ! mean required ? If so isn't it a problem that you can construct the class without giving all these fields a value at construction time?

[gleb-urvanov]

With ! we asserting the value will be assigned before it will be used. As alternative, we could initialize the values with default values, but the object is not intended to be used without all fields being initialized.

[mnaamani]

ok thanks for clarification. So we are infact using the ! incorrectly, the fields should then be initialized in the constructor. No ?

[gleb-urvanov]

With initialization in the constructor, the assignment assertion will be excessive (since the valued are guaranteed to be initialized). So, when the values will be initialized in the constructor, ! will be removed.

[mnaamani]

I see now the mechanism you mentioned at one point for getting results out of a test. It works well and I suppose its the only way really. But it can be improved. I think we need to do a better approach of encapsulating the calls, context and results of executing them to make things cleaner.

What would be really nice to see is something like this:

const context = { apiWrapper, keys, ... }
const params = { applicationStake, roleStake, ... } 
const expectations = { expectFailure, expectedValue/Events }

// asserts that state of runtime is what is expected for execution and prepares the execution to run
// in a tap.test call
const createOpeningFixture = new CreateWorkingGroupLeaderOpeningFixture(context, params)

// runs and asserts on expectations
tap.test('opening', createOpeningFixture.runner(expectations))

// at this point the createOpeningFixture has within it the result of execution, events, the porposalId for example.
// that can be used as inputs for the following tests.
createOpeningFixture.result 
createOpeningFixture.events
createOpeningFixture.openingId 

This is what I would have really liked to see as a pattern throughout. This makes constructing and writing scenarios very reusable.

[gleb-urvanov]

Made a note for a future refactoring in Iznik

[mnaamani]

I'm going to merge this now to have a the tests in the nicaea branch, but please take a note of the suggested refactoring in the next iteration.