Assigns created accounts in the hello world test templates to actual variables

[ryanleecode]

• What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) Assigns created accounts in the hello world test templates to actual variables.

• What is the current behavior? (You can also link to an open issue here) Created accounts are injected as global variables

• What is the new behavior (if this is a feature change)? variables are assigned to their respected self fields

• Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?) Users will need to prefix self or cls before their variables

• Other information: Was this a design decision to make the accounts global variables? I'm using pylint and its complaining about undefined variables... assigning them to class variables seems to make everything work the same and also gives the benefit of code navigation.

[rkintzi]

Thanks for your contribution. The changes you proposed cure symptoms, not real causes. The real problem (as you noticed) is that create_account and create_master_account functions are using global variables. I don't fully understand (in fact, I don't understand at all) what was the reason behind such a design, but I agree this should be changed.

Would you like to work on this and maybe make a more significant contribution to EOSFactory?

[rkintzi]

@stefanzarembinski Could you please tell us what was the intention/reason for using global variables here?

[ryanleecode]

Thanks for your contribution. The changes you proposed cure symptoms, not real causes. The real problem (as you noticed) is that create_account and create_master_account functions are using global variables. I don't fully understand (in fact, I don't understand at all) what was the reason behind such a design, but I agree this should be changed.

Would you like to work on this and maybe make a more significant contribution to EOSFactory?

I could take a stab at it. Lets see what stefan has to say first.

[stefanzarembinski]

The global character of the account variables is a fundamental design decision, having many aspects:

• simplicity of the code of intent applications,
• safety against a sort of user mistakes,
• let me call it 'philosophycal' for now,
• aesthetics,
• ...

Let me have time to write explanations that, I hope, can be convincing to you. Waiting for the explanations, let us consider how can be the code changed in order not to satisfy pylint complaining about undefined variables.

[stefanzarembinski]

Correcting a mistake: Waiting for the explanations, let us consider how can be the code changed in order to satisfy pylint complaining about undefined variables.

[stefanzarembinski]

Why EOS accounts are represented with Python globals?

Once upon a time, all program variables were global.

Now, the locals are of the first choice. Especially with Python that even discourages using globals by forcing the special notation. But Python cannot do without globals. There is hundreds of them -- hidden as EOSFactory accounts are -- in any program.

Local variables, global variables

Local variables added a lot of strength to programming languages: greatly increased scalability.

However, there are computable problems that can benefit from global variables. If a program represents fixed objects, they can be properly represented with globals variables or singletons.

For example, if a computer program is about the Earth, it is a trouble if the Earth is represented as independent local objects in different threads of the program.

EOSFactory is about EOS accounts. Once created, they last forever. (Well, it is not so with a local testnet, but none serious work is limited to local testnets.) As the accounts are unique and eternal, they are best represented with globals.

If you write a test, treating a problem of many account-roles, it is natural to name the accounts adequately to their roles.

For example, if in the first test of a series an account is locally named buyer, and one meter farther in the program the same account is represented as the local variable purchaser, then a reader -- possibly a law professionalist (see the next section), yet not trained in computer programing, who supervises a business-oriented smart-contract project and wants to get an insight into test results -- is, certainly, confused.

Well, the programmer can be careful not to alter the names. But computer-writing is difficult enough, even if programming tools help as much a they can.

We want EOSFactory to be helpful, as much as we can.

Readability of the scripts

People also ask why Python, and not javascript. We have chosen Python because its inherent drive to readability by a layman: it is possible to write Python code closely resembling natural english.

Our idea is that plainly written machine tests could be included into Ricardian Contract that are obligatory for serious EOS smart-contract works. Then the human-readable Ricardian contracts, addressed to law professionals, could gain their precision, still remaining readable by inteligent, yet without IT training, professionals. Please, see docs.cases.account.md in the EOSFactory repository, and execute the bash command eosfactory/pythonmd.sh docs/cases/account.md -- still in the EOSFactory repository directory. You can get the idea.

Globals in EOSFactory gain readability. Compare:

create_account("postman", post_office_manager)

and

self.postman = create_account(post_office_manager)

With the local self.postman you would have to explain the meaning of the self prependix in the Ricardian Contract.

Compactness of the scripts, and functionality

• You might note that the wallet that is an essential element of the EOS account functionality, is invisibly transparent for the test script developer and for a script reader. Hence, the script is shorter by numerous lines of code, necessary for explicit wallet operations.

• Also note that all the responses from the testnet, including internal names of the accounts and their public keys, are automatically translated into the corresponding names of the variables involved.

• You can see that account variables are restored between test sessions.

Valuable functionalities of EOSFactory result from mapping between global account variables and their names. This mapping could not be possible (well, noting is impossible with a computer) if account variables were local.

Consider:

self.postman = create_account(post_office_manager)
function(create_account(post_office_manager))

You cannot know the name of the account variable self.postman without unreliable hacking. And you can never know the name of the (temporal) account variable used as the argument in the function.

If you do not care, you can do it in this way, in your scripts:

self.postman = create_account("postman", post_office_manager)
function(create_account("postman1", post_office_manager))

But then you have to live with the sin of redundant information. And still you have globals undercover.

This mean codding is possible now, as the account factory functions, create_account and create_master_account, return the globals that they create. We do not document this feature, used internally, since we consider it anaesthetic and misleading.

But, finally, what is really wrong with the global accounts?

• In computer programing, wrong is possibility of misunderstanding the code. Can you point such a case? We show the opposite.
• Wrong is if EOSFactory is unable to express any practical test scenario. Can show such a scenario?
• Wrong is if you can write a better expressed test script if account variables are local. Can you show such an example?
• Wrong is automatic disgust at globals. This can be cured by reasoning.
• What else is wrong?
• Wrong is if EOSFactory makes the pylint nervous. It is so. We have to correct this inconvenience.

[stefanzarembinski]

@drdgvhbh: Now, as I have tried 'pylint`, I see your problem: account variables are underlined. For me, it is a bonus: account are beautifully distinquished.

[ryanleecode]

It can cause some issues who those who rely on CI tools or even git commit tools that rely on the linter actually passing.

By default, I think would expect most people to be using pylint (or any linter really) and having red lines in generated template code might scare away some folks.

For the sake of general adoption, we should either go with the solution I proposed above or find some other way to resolve the errors.

On Fri, Feb 8, 2019, 11:03 AM stefanzarembinski notifications@github.com wrote:

@drdgvhbh https://github.com/drdgvhbh: Now, as I have tried 'pylint`, I see your problem: account variables are underlined. For me, it is a bonus: account are beautifully distinquished.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tokenika/eosfactory/pull/133#issuecomment-461852076, or mute the thread https://github.com/notifications/unsubscribe-auth/AUYr0GxXOrqIXremkJ8VfUOSch3xuVCqks5vLZ_RgaJpZM4aXH60 .

[rkintzi]

Thanks, Stefan, for sharing your thoughts. Here is what I think.

Global variables are by definition neither good nor bad. The worst thing in the discussed design is that it goes against the expectations and habits of developers. The truth is that Python developers do not generally use global variables. They use linter to detect errors in the code (and not to highlight variables representing EOS accounts). They do this also in automated tests. It is very common that code that does not pass linter tests cannot be merged.

In my opinion, users should be treated as customers. In the case of EOS Factory, users are developers. Their convenience is more important than the mentioned readability (for non-programmers) or brevity. It is also not a good idea to try to convince the community to change their habits. It is a hopeless task.

As a second argument consider the following code. It is an artificial test case, but please imagine a program consisting of hundreds of source files, that are developed by dozens of developers. I were in such setup and - believe me - no one knew each and every line of code. And I really would not want to debug why this program stops working after creating a user account.

import unittest

import eosfactory.eosf as eosf

class AccountsTestCase(unittest.TestCase):

    @classmethod
    def setUpClass(cls):
        eosf.reset()
        eosf.create_master_account("master")

    @classmethod
    def tearDownClass(cls):
        eosf.stop()

    def test_master_variable_is_not_overwritten(self):
        self.assertTrue(master is master_)

class Master(object):
    '''this is somehow useful class'''
    pass

if __name__ == '__main__':
    master = Master() # this is a singleton instance
    master_ = master # just for test purposes
    unittest.main()

Let's discuss possible solutions.

Consider the following snippet that illustrates how EOS Factory is used now:

create_master_account("master")
create_account("host", master)

and compare with two other possible solutions:

master_account = create_master_account("master")
create_account("host", master_account)

create_master_account("master")
create_account("host", "master")

First solution does not need global variables because, we make the user responsible to keep references to accounts.

In the second one, EOS Factory can store accounts variables in its module namespace (possibly in a dictionary that maps names to objects).

In my opinion, in every case, we get precisely the same readability and conciseness of cases. Besides, each of new solutions seems to be more consistent, because it avoids the situation in which the account name is represented once using a string, and a moment later with the help of an identifier (representing a global variable).

[stefanzarembinski]

Account objects reside in the global namespace

Account objects reside in the global namespace (of the script module). This fact raises instinct reaction of python practitioners as they know that globals are evil.

I am determined to defend the design decision.

The globals are considered bad in any programming language. But global constants are perfectly fine to use. The account object variables are constants of a smart contract tests because the physical EOSIO accounts, that they represent, are eternal.

The reason that globals are bad is that they allow functions to have hidden and thus hard to understand side effects, if they change any of module globals.

In Python, there is no syntactic difference between constants and variables. With Python, adhering to good practices is then the only general method of reliable programming. With constants, good practice is to make constants uppercase. Then, assigning a value to an uppercase variable is a conscious decision.

Also, it helps that Python globals are local to their modules. Then, it is impossible that one programmer sets a constant variable to a value of an EOSFactory account object, and another programmer in another module redefines this constant just accidentally.

With EOSFactory, a special argument holds. As EOSFactory is for tests, scripts written with it should be composed of functions enclosing a complete live-cycle of a set of EOSIO actors. Thus, each such a function starts with resetting all the account objects (command eosf.reset()). Then, the programmer has to concentrate themselves only within the scope of the current test function.

I can propose EOSFactory good practice: if you do not want global constants in your program, do not use them. Account object global constants, marked with the upper case treat as constants are to be treated -- newer rebind those introduced with EOSFactory create_ factory functions.

Finally, I admit that there are methods for having constant account objects immutable, thus closing the controversy at the expense of decorating these objects with qualifiers, while our method results in clear and compact scripts (account variables not obscured with namespace qualificators). We believe that clearness is the first paradigm of the Python, and this is the reason that we have chosen Python. However, if the community insists, I will implement their way in the next editions.

[stefanzarembinski]

Back to the the original proposal by drdgvhbh. Thank you for your contribution, it was my mistake that I have not used linter (I work with Windows WSL bash and linter is not obvious there.)

The simple solution satisfying the linter is to declare the global constants:

import unittest
from eosfactory.eosf import *

verbosity([Verbosity.INFO, Verbosity.OUT, Verbosity.TRACE, Verbosity.DEBUG])

CONTRACT_WORKSPACE = "_wslqwjvacdyugodewiyd"

# Actors of the test:
MASTER = None
HOST = None
ALICE = None
CAROL = None
BOB = None

class Test(unittest.TestCase):

    def run(self, result=None):
        super().run(result)

    @classmethod
    def setUpClass(cls):
        SCENARIO('''
        Create a contract from template, then build and deploy it.
        ''')
        reset()
        create_master_account("MASTER")

        COMMENT('''
        Create test accounts:
        ''')
        create_account("ALICE", MASTER)
        create_account("CAROL", MASTER)
        create_account("BOB", MASTER)

    def test_functional(self):
        COMMENT('''
        Create, build and deploy the contract:
        ''')
        create_account("HOST", MASTER)
        contract = Contract(HOST, project_from_template(
            CONTRACT_WORKSPACE, template="hello_world", 
            remove_existing=True))
        contract.build()
        contract.deploy()

        COMMENT('''
        Test an action for ALICE, including the debug buffer:
        ''')
        HOST.push_action(
            "hi", {"user":ALICE}, permission=(ALICE, Permission.ACTIVE))
        self.assertTrue("ALICE" in DEBUG())

        COMMENT('''
        Test an action for CAROL, including the debug buffer:
        ''')
        HOST.push_action(
            "hi", {"user":CAROL}, permission=(CAROL, Permission.ACTIVE))
        self.assertTrue("CAROL" in DEBUG())

        COMMENT('''
        WARNING: This action should fail due to authority mismatch!
        ''')
        with self.assertRaises(MissingRequiredAuthorityError):
            HOST.push_action(
                "hi", {"user":CAROL}, permission=(BOB, Permission.ACTIVE))

    @classmethod
    def tearDownClass(cls):
        stop()

if __name__ == "__main__":
    unittest.main()

However, suggestion by drdgvhbh is inspiring and result with an alternative synthax, supported with the new edition of EOSFactory. There we also explain why we prefer sticking to the standard one.

See an example of the new synthax:

import unittest
from eosfactory.eosf import *

verbosity([Verbosity.INFO, Verbosity.OUT, Verbosity.TRACE, Verbosity.DEBUG])

CONTRACT_WORKSPACE = "_wslqwjvacdyugodewiyd"

class Test(unittest.TestCase):

    @classmethod
    def setUpClass(cls):
        SCENARIO('''
        Create a contract from template, then build and deploy it.
        ''')
        reset()

    def test_functional(self):
        master = new_master_account()
        COMMENT('''
        Create test accounts:
        ''')
        alice = new_account(master)
        carol = new_account(master)
        bob = new_account(master)

        COMMENT('''
        Create, build and deploy the contract:
        ''')
        host = new_account(master)
        contract = Contract(host, project_from_template(
            CONTRACT_WORKSPACE, template="hello_world", 
            remove_existing=True))
        contract.build()
        contract.deploy()

        COMMENT('''
        Test an action for Alice, including the debug buffer:
        ''')
        host.push_action(
            "hi", {"user":alice}, permission=(alice, Permission.ACTIVE))
        self.assertTrue("alice" in DEBUG())

        COMMENT('''
        Test an action for Carol, including the debug buffer:
        ''')
        host.push_action(
            "hi", {"user":carol}, permission=(carol, Permission.ACTIVE))
        self.assertTrue("carol" in DEBUG())

        COMMENT('''
        WARNING: This action should fail due to authority mismatch!
        ''')
        with self.assertRaises(MissingRequiredAuthorityError):
            host.push_action(
                "hi", {"user":carol}, permission=(bob, Permission.ACTIVE))

    @classmethod
    def tearDownClass(cls):
        stop()

if __name__ == "__main__":
    unittest.main()

[ryanleecode]

One thing it might also be important to consider is adding typings to the accounts so that VSCode/Pycharm/Somethingelse knows that the account object is an account and will provide code completion for it. By assigning these constants to initially None, the IDE won't be able to infer the type.

Consider the first solution

# Actors of the test:
MASTER = None
HOST = None
ALICE = None
BOB = None
CAROL = None

This is the code completion I get in VSCode.

Consider now the following.

from eosfactory.shell.account import Account

# Actors of the test:
MASTER: Account = None
HOST: Account = None
ALICE: Account = None
BOB: Account = None
CAROL: Account = None

Heres what I get

This is really useful as a developer because I don't need to guess the methods or parameters, or even have to look them up in the documentation. Its also useful because I can click on "Go to Definition" and it will bring me to the source code.

However these changes will require changing the dependency of this module from Python 3.5 to Python 3.6 and installing typing_extensions

Note this is not like typescript where it doesn't compile if you put in something that gives syntatic errors. It will still run all the same.

[stefanzarembinski]

These feature comes naturally with the new synthax, explained in my previous comment.

[stefanzarembinski]

You can test the new style: branch pypi-ready.

[ryanleecode]

Yes of course. I'm only trying to state why I would prefer using the new syntax over the constant syntax. But if the constant syntax had those autocomplete features, I think I would prefer using it over the new syntax because the new syntax is more cumbersome when creating/using accounts in different methods.

[ryanleecode]

Alternatively it might be better to leave up to the user whether he wants to using typings or not. My suggestion would be to document this capability somewhere and also export Account in the core eosfactory.eosf package so I don't need to import it from eosfactory.shell.account.

[stefanzarembinski]

I agree, you are right. I appreciate your remarks. Now, declarations are typed, for example:

MASTER = MasterAccount()
HOST = Account()
ALICE = Account()
CAROL = Account()
BOB = Account()

Or, they are not typed:

MASTER = None
HOST = None
ALICE = None
CAROL = None
BOB = None

Also, they can be omitted. Linter can be silesed with the following entry in the VSCode setup.json:

    "python.linting.pylintArgs": [
        "--disable=undefined-variable",
    ]