Refactoring CRPCCommand with enum category #13945

isghe · 2018-08-11T23:21:07Z

refactoring CRPCCommand changing the type of its member category from std::string to a typedef enum ERPCCategory.
The behaviour should be exactly the same with a very very small, in any way irrelevant, speed dump.

I am doing this, in preparation to add attributes, likes visibility and state to CRPCCommand structure, to avoid using category names, for overridden informations like visibility or deprecation.

For example, from v0.16.0 the command getinfo switched its category from control to hidden, while I think the idea was to let it to control category, with attributes similar to visibility = false and state = deprecated

Thanks,
Isidoro Ghezzi

p.s:

Take care on the fact, that I had no warning assigning the enum to a std::string :-(
Also take care I'm never testing on QT. (I simply never configure nor compile on QT); anyway I was able to catch the QT command rpcNestedTest.
I tested only on regtest and macOS

EDITs:

Using enum class it's giving right error trying to assign by mistake to a std::string :-) cf190f5
I tested on regtest on ubuntu and macOS
I never tested nor compiled on windows.

isghe · 2018-08-12T00:06:54Z

CI failed because of tabs instead o spaces, sorry I am not a python (which version?!) developer; going to solve

promag

Kind of weak concept ACK, but current implementation is a bit off.

Please read developer-notes.md and update code according.

Also squash commits.

promag · 2018-08-12T00:07:05Z

src/rpc/server.h

@@ -128,13 +128,29 @@ void RPCRunLater(const std::string& name, std::function<void(void)> func, int64_

 typedef UniValue(*rpcfn_type)(const JSONRPCRequest& jsonRequest);

+typedef enum{


Use enum class.

Also, if you use enum class, get rid of the eRPCCategory_ prefix (since then it will be namespaced already).

Please don't add bitcoin-specific things (such as these categories) in rpc/server.h, the base RPC code is supposed to be independent of any specific use.

Using enum class in cf190f5

promag · 2018-08-12T00:07:20Z

src/rpc/server.h

@@ -128,13 +128,29 @@ void RPCRunLater(const std::string& name, std::function<void(void)> func, int64_

 typedef UniValue(*rpcfn_type)(const JSONRPCRequest& jsonRequest);

+typedef enum{
+	eRPCCategory_blockchain = 0,


Don't use tabs.

isghe · 2018-08-12T01:03:05Z

now CI it is failing because of:

0.04s$ test/lint/check-rpc-mappings.py .
Traceback (most recent call last):
  File "test/lint/check-rpc-mappings.py", line 158, in <module>
    main()
  File "test/lint/check-rpc-mappings.py", line 98, in main
    cmds += process_commands(os.path.join(root, fname))
  File "test/lint/check-rpc-mappings.py", line 58, in process_commands
    assert m, 'No match to table expression: %s' % line
AssertionError: No match to table expression:     { eRPCCategory_control,            "help",                   &help,                   {"command"}  },
The command "test/lint/check-rpc-mappings.py ." exited with 1.

I don't know how to instruct test/lint/check-rpc-mappings.py teaching to him that a field should be an enum, not a std::string.

EDIT: should be solved (now using enum class) with 4d56e9b

DrahtBot · 2018-08-12T01:19:59Z

Note to reviewers: This pull request conflicts with the following ones:

Index for BIP 157 block filters #14121 (Index for BIP 157 block filters by jimpo)
Add address-based index (attempt 4?) #14053 (Add address-based index (attempt 4?) by marcinja)
Utxoscriptindex #14035 (Utxoscriptindex by mgrychow)
Additional utility RPCs for PSBT #13932 (Additional utility RPCs for PSBT by achow101)
[Tools] bitcoin-wallet - a tool for creating and managing wallets offline #13926 ([WIP] [Tools] bitcoin-wallet-tool by jnewbery)
RPC: Add new 'clearmempool' method #13836 (RPC: Add new 'clearmempool' method by domob1812)
Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency #13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by 251Labs)
wallet/rpc: sendrawtransaction maxfeerate #13541 (wallet/rpc: sendrawtransaction maxfeerate by kallewoof)
[rpc] Add getnodeaddresses RPC command #13152 ([rpc] Add getnodeaddresses RPC command by chris-belcher)
RPC: Support addnode onetry without making the connection priviliged #12674 (RPC: Support addnode onetry without making the connection priviliged by luke-jr)
[Wallet] [RPC] Remove deprecated wallet rpc features from bitcoin_server #12490 ([Wallet] [RPC] Remove deprecated wallet rpc features from bitcoin_server by jnewbery)
Refactor: separate wallet from node #10973 (Refactor: separate wallet from node by ryanofsky)
Multiprocess bitcoin #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

domob1812 · 2018-08-12T09:31:12Z

src/rpc/server.cpp

+        "wallet",
+        "zmq",
+        "test"
+    };


This implementation means that the list here has to be in sync with the enum definition. I think it would be much more robust and readable if you had here a big switch instead.

Done cf190f5

domob1812 · 2018-08-12T09:32:17Z

src/rpc/server.h

@@ -128,13 +128,29 @@ void RPCRunLater(const std::string& name, std::function<void(void)> func, int64_

 typedef UniValue(*rpcfn_type)(const JSONRPCRequest& jsonRequest);

+typedef enum{


Also, if you use enum class, get rid of the eRPCCategory_ prefix (since then it will be namespaced already).

laanwj · 2018-08-12T11:40:15Z

A drawback of this is that it centralizes knowledge of the command categories in the source code; right now, a module such as the wallet can register its own commands in its own category, without having to update a central file. This is a good form of decoupling.

This is not as bad for categories as it would be for, say, commands themselves, but still.

isghe · 2018-08-12T17:07:33Z

A drawback of this is that it centralizes knowledge of the command categories in the source code; right now, a module such as the wallet can register its own commands in its own category, without having to update a central file. This is a good form of decoupling.

This is not as bad for categories as it would be for, say, commands themselves, but still.

@laanwj the target of this refactoring, is right to centralise the knowledge of the command categories, to avoid misuse of categories, like it was done for hidden category.

isghe · 2018-08-12T18:45:34Z

I am going to squash commits, let me know if everything is ok

ken2812221

Concept ACK. A few nits.

ken2812221 · 2018-08-12T21:31:02Z

src/rpc/rpccategory.cpp

+namespace rpccategory
+{
+
+std::string Label (const RPCCategory category);


Remove this line

do you mean the empty line (line 10) between { and std::string?

No, I mean removing std::string Label (const RPCCategory category);, you already declare it at rpccategory.h

done 189bdae

ken2812221 · 2018-08-12T21:32:55Z

src/rpc/rpccategory.h

+namespace rpccategory
+{
+
+extern std::string Label (const RPCCategory category);


remove extern

hm… extern (ok I'm old style C developer) means declared elsewhere, infact the prototype is declared in src/rpc/rpccategory.cpp, and the definition also in src/rpc/rpccategory.cpp. What's wrong?

extern on function is redundant, it is already extern if you don't implement it.

done 189bdae

ken2812221 · 2018-08-12T21:33:42Z

src/rpc/rpccategory.cpp

+
+std::string Label (const RPCCategory category);
+
+std::string Label (const RPCCategory category)


Please remove extra space between Label and (. There are many other places need to do this.

done 477b083

ken2812221 · 2018-08-12T21:35:21Z

src/rpc/rpccategory.cpp

+std::string Label (const RPCCategory category)
+{
+    #ifdef HANDLE_CASE_RETURN
+    #error "HANDLE_CASE_RETURN already defined"


I don't think this is necessary

you will miss it, in case of unpredictable mistakes ;-)

ken2812221 · 2018-08-12T21:40:04Z

src/rpc/rpccategory.cpp

+            HANDLE_CASE_RETURN (RPCCategory, test);
+            // if you are missing a case, you'll have a warning here
+    }
+    #undef HANDLE_CASE_RETURN


I think we don't need to undef this

ok right now, it's not necessary, but i meant to define a local MACRO: define and use as you like, and undefined when you don't need anymore.

ken2812221 · 2018-08-12T21:40:47Z

src/rpc/rpccategory.h

@@ -0,0 +1,33 @@
+// Copyright (c) 2010 Satoshi Nakamoto
+// Copyright (c) 2009-2018 The Bitcoin Core developers


done in c9f15c1

ken2812221 · 2018-08-12T21:41:20Z

src/rpc/rpccategory.cpp

@@ -0,0 +1,39 @@
+// Copyright (c) 2009-2010 Satoshi Nakamoto
+// Copyright (c) 2009-2018 The Bitcoin Core developers


done in c9f15c1

ken2812221 · 2018-08-12T21:41:58Z

src/rpc/server.cpp

@@ -158,7 +158,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
    std::vector<std::pair<std::string, const CRPCCommand*> > vCommands;

    for (const auto& entry : mapCommands)
-        vCommands.push_back(make_pair(entry.second->category + entry.first, entry.second));
+        vCommands.push_back(std::make_pair(rpccategory::Label (entry.second->category) + entry.first, entry.second));


nit: emplace_back

can you explain better? thanks

Since you're touching this line, you can use c++11 emplace_back to have better performance: vCommands.emplace_back(rpccategory::Label(entry.second->category) + entry.first, entry.second);

done c13fe25

ken2812221 · 2018-08-12T21:42:36Z

src/rpc/server.h

@@ -17,6 +17,8 @@

 #include <univalue.h>

+#include <rpc/rpccategory.h>


Move this after #include <rpc/protocol.h>

doing in the next commit, thanks

done 7e5453d

maflcko · 2018-08-12T21:53:04Z

src/rpc/rpccategory.h

+
+enum class RPCCategory
+{
+    blockchain = 0,


I don't think you have to set this to 0, also compile time constants should be ALL_UPPER_CASE.

yes, it's not necessary infact, I am going to remove the zero initialization (it was necessary before, when I was using an array to transform enum to std::string);
(done in 7659882)

About ALL_UPPER_CASE for compile time constants, should be doesn't mean must and as it is written in https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md
I think this is near the case:

are not required when doing so would need changes to significant pieces of existing code.

and worst and more: consider the fact that, in the case those constants to be uppercase, an extra runtime (!) code would be neccessary to the function rpccategory::Label to make lowercase the result.

But ok, because of that, I'll explain in a comment in enum class RPCCategory that the constant identifiers are used exactly as the output

isghe · 2018-08-12T22:50:40Z

wip: please don't merge

isghe · 2018-08-13T01:01:30Z

wip: waiting more suggestions, before new squash

isghe · 2018-08-13T01:10:15Z

Now I don't understand why CI is failing. But I have to go to sleep

EDIT: it was for missing #include <cassert> c1418aa

ken2812221 · 2018-08-13T00:52:21Z

src/rpc/rpccategory.cpp

+namespace rpccategory
+{
+
+std::string Label (const RPCCategory category);


No, I mean removing std::string Label (const RPCCategory category);, you already declare it at rpccategory.h

ken2812221 · 2018-08-13T00:58:59Z

src/rpc/server.cpp

@@ -158,7 +158,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
    std::vector<std::pair<std::string, const CRPCCommand*> > vCommands;

    for (const auto& entry : mapCommands)
-        vCommands.push_back(make_pair(entry.second->category + entry.first, entry.second));
+        vCommands.push_back(std::make_pair(rpccategory::Label (entry.second->category) + entry.first, entry.second));


Since you're touching this line, you can use c++11 emplace_back to have better performance: vCommands.emplace_back(rpccategory::Label(entry.second->category) + entry.first, entry.second);

ken2812221 · 2018-08-13T01:10:05Z

src/rpc/rpccategory.h

+namespace rpccategory
+{
+
+extern std::string Label (const RPCCategory category);


extern on function is redundant, it is already extern if you don't implement it.

ken2812221 · 2018-08-13T01:13:52Z

src/rpc/rpccategory.cpp

+            HANDLE_CASE_RETURN (RPCCategory, test);
+            // if you are missing a case, you'll have a warning here
+    }
+    assert (0); // never fall down here


You need to #include <cassert>

done c1418aa

isghe · 2018-08-13T15:54:20Z

I am going to squash commits.
I think there is only one open point: the compile time constants in enum class RPCCategory, are lowercase instead of (should use to be) uppercase.

The point is that if, I’ll make those identifiers uppercase, I have to make them lowercase in same place; and I have these options:

Hard coding lowercase in the source code, in the function rpccategory::Label;
e.g. somethings similar to case RPCCategory::BLOCKCHAIN: return “blockchain”;, and there is the risk for example to introduce typos, and an extra maintenance step, for new categories.
Lowercasing them at runtime in function rpccategory::Label; somethings similar to:

#include <boost/algorithm/string/case_conv.hpp>

static std::string LabelPrivate (const RPCCategory category){
	/* exactly the actual code of rpccategory::Label */
}

std::string Label (const RPCCategory category){
	std::string ret = LabelPrivate (category);
	boost::algorithm::to_lower (ret);
	return ret;
}

Lowercasing them at runtime in function CRPCTable::help

Another option could be, to leave them in upper case, for example:

$ /src/bitcoin-cli help
== BLOCKCHAIN ==
getbestblockhash

But we cannot know the impact and the implications, such modification in the output, could have in external software.

Because of those points, I think it should be better to leave them in lowercase.

Anyway let, me know

isghe · 2018-08-13T17:05:40Z

squashed

ken2812221 · 2018-08-14T09:50:52Z

utACK 4eec7a1c3421acc9d31c4c0f93ca6ec376e0847a

isghe · 2018-08-21T23:45:48Z

Hi @promag thanks for your review.

I would say, the only place where I spoke about performance, were together with the adjective irrelevant in this PR: this PR should be related only on the organisation, of the informations, related to the actual CRPCCommand::category, and now (without this PR) is sparse and not measurable (it's a string…).

This PR, title starts with Refactoring and (the good idea) it should not change the behaviour (I hope) in Bitcoin.

Which categories and how many we have now in Bitcoin RPC commands? Actually it's not easy to respond in deterministic way to the above question; maybe the only way, is to run ./bitcoin-cli help and parsing the results (missing the "hidden" category)!

Adding a global variable for each possible properties (hidden, deprecated, etc… properties) as you suggest, can be a good solution, but it can be a nightmare for maintenance, or maybe (if necessary) it can be done, in next steps, not now.

Like an example, consider also, that without this enum RPCCategory PR, it will be very hard (or very tedious and prone to errors) to delete the dependancy of src/rpc/server.cpp from the unwanted (for security locale reasons) "*.to_upper".

Without this PR; how Bitcoin will handle the next "blockhain" category? (take care about the typo).
Or, security reasons: how Bitcoin will handle a category (now it's just a string) sendtoaddress…?

Thanks :-)

promag · 2018-08-21T23:56:33Z

Without this PR; how Bitcoin will handle the next "blockhain" category? (take care about the typo).
Or, security reasons, how Bitcoin will handle a category (now it's just a string) sendtoaddress…?

You can write a test that asserts the possible categories.

promag · 2018-08-21T23:58:32Z

Actually it's not easy to respond in deterministic way to the above question; maybe the only way, is to run ./bitcoin-cli help and parsing the results!

And documentation. Or the above test.

isghe · 2018-08-22T00:54:17Z

@promag do you really think that adding a PR documentation similar to this, will help?

Joking-Bitcoin documentation:
just for developer: please take care on writing the string field of `CRPCCommand::category`; take care about typos, and  [SQL, script, bash, awk , regex or others] injections.
Also take care that categories should be in space definition, we don't know how to determinate; anyway it's delegated to an external tools (our unit test tools), we cannot verify if it is fully synchronized with the real source code.

just joking… do you? :-p

EDIT: renamed to Joking-Bitcoin documentation for security reasons.

isghe · 2018-08-22T20:47:21Z

Another consideration from another perspective: right now in branch master 271b379 searching for the hard coded string "wallet" we have 61 occurrences found, and are all (we are lucky) related to CRPCCommand. So a good programming practice, I think should be to use a constant, instead of 61 hard coded strings, e.g.:

namespace rpccommand
{
    const std::string WALLET="wallet";
} // namespace "rpccommand"

and substitute the 61 hard coded "wallet" with rpccommand::WALLET

Am I wrong?

This is a squash of the below commits: - enum ERPCCategory, CRPCCommand::Label - wip: CRPCCommand.category from type string to enum - wip: CRPCCommands construct with enum - refactoring with enum ok - missing test category in qt for cmd rpcNestedTest - wip: text formatting: tabs to spaces. - wip: refactor using `enum class` C++11. Update CRPCCommand::Label () using a switch case - wip: check-rpc-mappings.py fix regex to new format Now it's catching the format: ' { RPCCategory::control, "help", &help, {"command"} },' before was catching: ' { "control", "help", &help, {"command"} },' - wip: add src/rpc/rpccategory.h|cpp - removed CRCPCommand::Label for rpccategory::Label - moved `#include <rpc/rpccategory.h>` - removed unneccessary RPCCategory initializiation - comments and one assert(0) - // Copyright (c) 2018 The Bitcoin Core developers for files: src/rpc/rpccategory.h|cpp as suggested from github user ken2812221 - wip: missing `#include <cassert>` - using `emplace_back` C++11 - remove old-style C `extern` - remove spaces after function names - remove unnecessary call to RPCCategory::Label (cherry picked from commit 4eec7a1c3421acc9d31c4c0f93ca6ec376e0847a) enum RPCCategory uppercase, rpccategory:Label lowercase fix test/lint/check-rpc-mappings.py to uppercase remove unnecessary comments, and false instead of 0 std::string category to RPCCategory category Using boost::optional<RPCCategory> remove src/rpc/server.cpp Capitalize() call Capitalization is done by hand in rpccategory::Label()

removed also an un unnecessary forward declaration

promag · 2018-09-01T08:03:05Z

and substitute the 61 hard coded "wallet" with rpccommand::WALLET

@isghe that's unrelated to this PR, and IMO this can be closed.

laanwj · 2018-09-01T08:31:58Z

Sorry, there seems to be no agreement to do this, in which case we prefer not making a change to the code.
Closing.

isghe · 2018-09-02T00:25:33Z

Thanks for your reviews; I am happy because this PR, inspired #14020 , and I did learn new things on C++ programming (emplace and optional), and about test in python.
But I think that Refactoring CRPCCommand with enum category (this PR) should help (at compile time) security for Bitcoin.
If don't you see that, it's not my problem.

promag reviewed Aug 12, 2018

View reviewed changes

domob1812 reviewed Aug 12, 2018

View reviewed changes

laanwj added the RPC/REST/ZMQ label Aug 12, 2018

isghe force-pushed the refactoring-CRPCCommand-with-enum-category branch from 5604cfb to cbfa3a0 Compare August 12, 2018 20:37

ken2812221 reviewed Aug 12, 2018

View reviewed changes

maflcko reviewed Aug 12, 2018

View reviewed changes

ken2812221 reviewed Aug 13, 2018

View reviewed changes

isghe force-pushed the refactoring-CRPCCommand-with-enum-category branch from 854672b to 4eec7a1 Compare August 13, 2018 16:36

DrahtBot mentioned this pull request Aug 14, 2018

[wallet] Kill accounts #13825

Closed

DrahtBot added the Needs rebase label Aug 15, 2018

This was referenced Aug 21, 2018

[Tools] bitcoin-wallet - a tool for creating and managing wallets offline #13926

Merged

Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency #13751

Closed

promag mentioned this pull request Aug 22, 2018

Add tests for RPC help #14020

Merged

This was referenced Aug 22, 2018

qa: Add TestNode::assert_debug_log #14024

Merged

Remove accounts rpcs #14023

Merged

This was referenced Aug 23, 2018

Utxoscriptindex #14035

Closed

Add address-based index (attempt 4?) #14053

Closed

build: generate MSVC project files via python script #14062

Merged

DrahtBot added the Needs rebase label Aug 26, 2018

isghe force-pushed the refactoring-CRPCCommand-with-enum-category branch from 506b2ee to 89a497d Compare August 27, 2018 10:52

DrahtBot added Needs rebase and removed Needs rebase labels Aug 27, 2018

isghe force-pushed the refactoring-CRPCCommand-with-enum-category branch from 89a497d to 733ee98 Compare August 27, 2018 18:59

DrahtBot added Needs rebase and removed Needs rebase labels Aug 27, 2018

isghe added 2 commits August 30, 2018 16:50

class CRPCCommand to struct CRPCCommand

04e70f2

removed also an un unnecessary forward declaration

isghe force-pushed the refactoring-CRPCCommand-with-enum-category branch from 733ee98 to 04e70f2 Compare August 30, 2018 14:53

DrahtBot removed the Needs rebase label Aug 30, 2018

DrahtBot mentioned this pull request Aug 31, 2018

Index for BIP 157 block filters #14121

Merged

laanwj closed this Sep 1, 2018

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

		@@ -128,13 +128,29 @@ void RPCRunLater(const std::string& name, std::function<void(void)> func, int64_

		typedef UniValue(*rpcfn_type)(const JSONRPCRequest& jsonRequest);

		typedef enum{


		std::string Label (const RPCCategory category);

		std::string Label (const RPCCategory category)

		@@ -0,0 +1,33 @@
		// Copyright (c) 2010 Satoshi Nakamoto
		// Copyright (c) 2009-2018 The Bitcoin Core developers

		@@ -0,0 +1,39 @@
		// Copyright (c) 2009-2010 Satoshi Nakamoto
		// Copyright (c) 2009-2018 The Bitcoin Core developers

		@@ -17,6 +17,8 @@

		#include <univalue.h>

		#include <rpc/rpccategory.h>

Refactoring CRPCCommand with enum category #13945

Refactoring CRPCCommand with enum category #13945

Uh oh!

Conversation

isghe commented Aug 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isghe commented Aug 12, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isghe commented Aug 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 12, 2018

Uh oh!

isghe commented Aug 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isghe commented Aug 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ken2812221 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isghe Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

isghe commented Aug 11, 2018 •

edited

Loading

isghe commented Aug 12, 2018 •

edited

Loading

DrahtBot commented Aug 12, 2018 •

edited

Loading

isghe commented Aug 12, 2018 •

edited

Loading

isghe commented Aug 12, 2018 •

edited

Loading

isghe Aug 13, 2018 •

edited

Loading

isghe Aug 12, 2018 •

edited

Loading

isghe commented Aug 13, 2018 •

edited

Loading