fpriv / factom-harmony-connect-python-sdk Goto Github PK

There may be some issues incorporating @sambarnes's changes (he added Anchors and Receipts clients in PR #21), which are now part of master, and it is likely that there will be bugs that show up in the unit tests. We should confirm that all of the tests are passing now that we've added receipts and anchors.

Steps:

1. Rebase to master to get anchors and receipts code
2. Incorporate **kwargs and the client override arguments (that work has been done on the existing methods as part of #22) into the new methods as outlined in factom_sdk/client/anchors_client.py and factom_sdk/client/receipts_client.py
3. confirm that all unit tests are still working

Adapted from FPriv/factom-harmony-connect-js-sdk#21

Each Resource method should have its own methods ending simply in get() or create(). The only non-REST generic methods would be search(), get_first() and get_last().

So for example:

First, we instantiate our chain by doing, instead of...

my_chain = factom_sdk.chain('123456')

...we do...

my_chain = factom_sdk.chain.get('123456')

...so no more Chain constructor for the user to think about, we roll it all into the .get() method.

From there, continuing the pattern, we change this...

my_chain.create_entry(...)

...to be...

my_chain.entries.create(...)

Going one level deeper, this...

my_chain.get_first_entry()

...becomes...

my_chain.entry.get_first() # defaults to `True` for checking validation
my_chain.entry.get_first(signature_validation=False) # doesn't check validation

AND...

my_entries = my_chain.entries.get()
my_entry_search_results = my_entries.search_entries_of_chain(...)

...becomes...

my_entries = my_chain.entries.get()
my_entry_search_results = my_entries.search(...)

This is simply to allow for optional settings in our methods in the future.

By adding **kwargs as the final argument in any of these functions, we can reserve that space for any optional properties we could add, without having to worry about the order of any existing properties.

So the task here to to move all optional arguments to go under the **kwargs object, and then pull those **kwargs properties out into the body of the method.

https://github.com/FactomProject/factom-harmony-connect-python-sdk/blob/d0e231a5d19f9f0e1df429bbfe28acf7df8cd8a5/factom_sdk/client/identities_client.py#L68

These were introduced in python 3.6. We either need to use string's .format() function if we want to support version >= 3.5 like we claim. There might be more occurrences than this.

While creating a signed chain, the timestamp of the signing is added to the external ids. Suppose I create a chain with Ext IDs - ['A', 'B', 'C'] and I sign it. Since the timestamp is added, I can keep creating chains with Ext IDs - ['A', 'B', 'C'] since each time it'll have a unique list of Ext IDs thanks to the timestamp.

I think the timestamp should be moved out of the Ext IDs and into the content of the first entry. This way I can't keep creating a chain with the same Ext IDs.

The Ext IDs should describe the chain, if I want to add an entry to a chain with Ext IDs - ['A', 'B', 'C'] - I could have potentially 100s of chains each with a different timestamp.

Everywhere we use kwargs, we don't need to take out the parameters to pass them onto the next function if they're just gonna have the same name again. Just repass kwargs up the chain

Here for instance:
https://github.com/FactomProject/factom-harmony-connect-python-sdk/blob/master/factom_sdk/client/chains_client.py#L30

base_url = kwargs.get("base_url")
app_id = kwargs.get("app_id")
app_key = kwargs.get("app_key")
if not chain_id:
    raise Exception("chain_id is required.")
response = self.request_handler.get("/".join([factom_sdk.utils.consts.CHAINS_URL, chain_id]), base_url=base_url, app_id=app_id, app_key=app_key)

can be reduced to:

if not chain_id:
    raise Exception("chain_id is required.")
response = self.request_handler.get("/".join([factom_sdk.utils.consts.CHAINS_URL, chain_id]), **kwargs)

This should clean up the code a fair amount, and only care about taking parameters out when they are going to be acted on

UPDATE (5/16/19): After doing some more research, we've decided NOT to put overriding properties on a separate client_overrides object. Instead, we will simply be allowing developers to place those properties as additional individual arguments in our methods.

NOTE: It would probably be better to tackle #20 before doing this one...

There are certain instances where we'd like to allow the user to specify override parameters that were set in the instantiation of the SDK. Right now, the parameters we're looking to allow to be overridden are:

• app_id
• app_key
• base_url
• automatic_signing

To make this happen, we would use a dictionary named "client_overrides" as an optional item in the list of arguments in the method definition.

For example, using the definition for chains.get at https://github.com/FactomProject/factom-harmony-connect-python-sdk/blob/master/factom_sdk/client/chains_client.py#L20, we would be adding our overriding properties as the arguments within the **kwargs at the end of the list of arguments:

def get(self, chain_id: str, signature_validation=None, **kwargs):
   ... etc etc etc...

Then, in the RequestHandler class at https://github.com/FactomProject/factom-harmony-connect-python-sdk/blob/master/factom_sdk/request_handler/request_handler.py#L24, we could make the properties that were part of the instantiation fallback values to allow for the possibility of the overrides:

def _generic_request(self, method, endpoint, data: dict = None, params: dict = None):    
    base_url = params.get("base_url", self.base_url) 

    headers = {
        "app_id": params.get("app_id", self.app_id)
        "app_key": params.get("app_key", self.app_key)
   }
 
    ...etc etc etc...

User implementation

factom_client.chains.get("4b9532c79d53ab22b85951b4a5853f81c2682132b3c810a95128c30401cd1e58",
    base_url="YOUR API URL",
    app_id="YOUR APP ID",
    app_key="YOUR APP KEY"
)

To turn automatic_signing off for one call...

create_entry_response = factom_client.chains.entries.create(chain_id=chain["data"]["chain_id"],
                                                            signer_private_key=key_to_sign["private_key"],
                                                            signer_chain_id=identity_chain_id,
                                                            external_ids=["NotarySimulation",
                                                                            "DocumentEntry",
                                                                            "doc987"],
                                                            content=json.dumps({"document_hash": document_hash,
                                                                                "hash_type": "sha256"
                                                                                }),
                                                             automatic_signing=False)

In the constructor for FactomClient, I believe that automatic_signing should be set to False. Under the current implementation, a chain cannot be created unless it is signed.

factom_client.chains.create(content = "Test", external_ids = ['Test', 'One', 'Two'])
results in Exception: signer_private_key is required.

Changing automatic_signing to False would allow unsigned chains to be created using the simple syntax above and would require passing an identity chain and private key to the function call if the chain needs to be signed.

The same thing applies to creating entries as well.

I'd also like to hear opinions on making content default to some text so that it is not a mandatory field while creating a chain.

def create(content: str = "", external_ids: list = [], **options):
    """ """
    signer_chain_id         = options.get('signer_chain_id', None)
    signer_private_key      = options.get('signer_private_key', None)
    callback_url            = options.get('callback_url', None)
    callback_stages         = options.get('callback_stages', [])
    sign                    = self.automatic_signing
    ids_base64              = []

    if signer_chain_id and signer_private_key:
        if KeyCommon.validate_checksum(signer_private_key):
            sign            = True
        else:
            raise Exception("Invalid Private Key passed")

    if sign and (signer_chain_id == None or signer_private_key == None):
        raise("The Chain can't be signed unless BOTH signer identity chain and signer private key are passed..")

    if not hasattr(external_ids, '__iter__'):
        raise Exception("External IDs must be iterable")

    if not hasattr(callback_stages, '__iter__'):
        raise Exception("Callback Stages must be iterable")

    if callback_url and not validators.url(callback_url):
        raise Exception("callback_url is an invalid url format.")

    if len(external_ids) < 1:
        raise Exception("Not enough External IDs provided")

    # Should content be a mandatory field? It seems to me that as long as External IDs are passed, a default first entry can be made. 
    if content == "":
        content             = "This is the first entry in the chain with External IDs: " + ", ".join(str(_id) for _id in external_ids)
    
    if sign:
        time_stamp          = datetime.datetime.utcnow().isoformat()
        message             = signer_chain_id + content + time_stamp
        signature           = KeyCommon.sign_content(signer_private_key, message)
        signer_public_key   = KeyCommon.get_public_key_from_private_key(signer_private_key)
        items               =\
        [
            encode_string("SignedChain"),
            encode_string(bytes([0x01])),
            encode_string(signer_chain_id),
            encode_string(signer_public_key),
            signature,
            encode_string(time_stamp)
        ]
        ids_base64          = items + [encode_string(val) for val in external_ids]

    data                    = \
    {
        "external_ids"  : ids_base64,
        "content"       : encode_string(content)
    }

    if callback_url:
        data["callback_url"]        = callback_url
    if callback_stages:
        data["callback_stages"]     = callback_stages

    return self.request_handler.post(factom_sdk.utils.consts.CHAINS_URL, data)

I'm creating a Django application that utilizes Factom for creating identities and posting entries. I've implemented identities successfully but I've been running into this error when I try to create a chain using the private key I was given. At first I thought I must have stored the key in the wrong format, but I've checked and they look identical to the examples in the docs. You can see one of the keys I'm using (sandbox, so no worries about security): idsecv344jw02xFeZHFozU73ixGCKBWeotb9OkMuYkaM9sLFNmDSvCL

The error is coming from the base58 package when the SDK tries to use the b58decode method on the key I pass in and throws this: ValueError: subsection not found. Looking at the code, I'm not entirely sure why this is being thrown.

Even if my input is formatted wrong, there should a patch to catch this error in the SDK itself. I'm more than happy to contribute to that patch, especially if I can get some advice on whether this error is coming from my side or not.

Full log below:

identity/factom/factom.py:39: in post_chain
    response = factom_client.chains.create(
/usr/local/lib/python3.8/site-packages/factom_sdk/client/chains_client.py:80: in create
    if not KeyCommon.validate_checksum(signer_private_key):
/usr/local/lib/python3.8/site-packages/factom_sdk/utils/key_common.py:34: in validate_checksum
    signer_key_bytes = base58.b58decode(signer_key)
/usr/local/lib/python3.8/site-packages/base58.py:95: in b58decode
    acc = b58decode_int(v)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

v = b'idsecv344jw02xFeZHFozU73ixGCKBWeotb9OkMuYkaM9sLFNmDSvCL'

    def b58decode_int(v):
        '''Decode a Base58 encoded string as an integer'''
        v = v.rstrip()
        v = scrub_input(v)
    
        decimal = 0
        for char in v:
>           decimal = decimal * 58 + alphabet.index(char)
E           ValueError: subsection not found

/usr/local/lib/python3.8/site-packages/base58.py:82: ValueError