Async kin + Typing by Ronserruya · Pull Request #66 · kinecosystem/kin-sdk-python

Ronserruya · 2019-03-24T10:48:45Z

Update the kin-sdk to work with asyncio:

Changes:

Most of the methods are now async
Minimum supported python version changed to 3.6
User-agent on http requests changed from "kin-core-python" to "kin-sdk-python"
Added type hints to all methods
Builder and Horizon modules were removed since we use kin-base instead of inheriting them
KinAccount doesn't throw an error when initialized with account that doesn't exist on the blockchain
Monitors were changed to async generators and not generators in a different threads
New parameter to set timeout between events in payment monitors
MultiMonitor was changed to remove the "add/remove_address" methods, the passed set can just be changed at will.
Timeout parameter removed from get_channel method, as async queue doesn't provide timeout
Added context manager to the KinClient object to close the connection to the blockchain
Removed "verify_kin_payment" method
Transactions are now retried with the same tx, instead of building a new one each time

codecov · 2019-03-24T10:53:39Z

Codecov Report

Merging #66 into v2-master will increase coverage by 6.41%.
The diff coverage is 71%.

@@              Coverage Diff              @@
##           v2-master      #66      +/-   ##
=============================================
+ Coverage      78.33%   84.75%   +6.41%     
=============================================
  Files             17       15       -2     
  Lines           1071      774     -297     
  Branches         115       83      -32     
=============================================
- Hits             839      656     -183     
+ Misses           200       93     -107     
+ Partials          32       25       -7

Impacted Files	Coverage Δ
kin/blockchain/errors.py	`100% <ø> (+0.88%)`	⬆️
kin/blockchain/environment.py	`100% <100%> (ø)`	⬆️
kin/transactions.py	`85.07% <100%> (-0.22%)`	⬇️
kin/blockchain/keypair.py	`93.54% <100%> (+0.95%)`	⬆️
kin/config.py	`100% <100%> (ø)`	⬆️
kin/__init__.py	`100% <100%> (ø)`	⬆️
kin/version.py	`100% <100%> (ø)`	⬆️
kin/monitors.py	`22.85% <16.66%> (+9.06%)`	⬆️
kin/utils.py	`41.66% <21.73%> (+3.43%)`	⬆️
kin/account.py	`84.54% <86.04%> (-0.42%)`	⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9db0ca...779c6a2. Read the comment docs.

mrsegev

|Please see comments.

mrsegev · 2019-03-27T09:00:11Z

images/docker-compose.yml

 services:
  stellar-core-1:
-    image: kinecosystem/stellar-core:kinecosystem-v2.0.0-stellar-v9.2.0
+    image: kinecosystem/stellar-core:latest


It's better to hardcode the version. This is the only way to be 100% sure the build is stable. Future updates might break the code.

mrsegev · 2019-03-27T09:00:45Z

images/docker-compose.yml


  stellar-core-2:
-    image: kinecosystem/stellar-core:kinecosystem-v2.0.0-stellar-v9.2.0
+    image: kinecosystem/stellar-core:latest


Same. Let's hardcode the versions in this file.

blockchain team removes their docker images a lot so the tests fail. latest will always be there

mrsegev · 2019-03-27T09:07:25Z

requirements-dev.txt

-py==1.5.2
-pytest-cov==2.5.1
-pytest==3.4.0
+codecov


Let's lock the version here.

yosriz

Looking good, few small comments.

yosriz · 2019-03-27T19:38:51Z

kin/client.py

        return raw_tx

-    def get_account_tx_history(self, address, amount=10, descending=True, cursor=None, simple=True):
+    async def get_account_tx_history(self, address: str, amount: Optional[int] = 10, descending: Optional[bool] = True,


from a talking with @mrsegev we decided we should't expose this utill we solve history issue on the blockchain side, as it's not reliable now it can cause just troubles,

@mrsegev This has been exposed for months now, should I remove it then?

yosriz · 2019-03-27T19:43:17Z

kin/client.py

-        response = requests.get(self.environment.friendbot_url, params={'addr': address})
-        if response.ok:
-            return response.json()['hash']
+        response = await self.horizon._session.get(self.environment.friendbot_url, params={'addr': address})


in node-sdk, I've added also the fund option, in case of an existing account, you might want to add it as well (not necessarily now)

Good idea, I will create a ticket

yosriz · 2019-03-28T06:40:50Z

kin/blockchain/channel_manager.py

+class ChannelPool(queue):
    """
-    A thread-safe queue that sets a member's status instead of pulling it in/out of the queue.
+    An async queue that sets a member's status instead of pulling it in/out of the queue.


out of curiosity, why queue is needed? seems simple list/set will provide the same functionality? I mean there's no real use of push/pop here.
(mainly trying to understand that I'm not missing something with my node impl for that)

There is really no use of any push/pop here. I used the queue class before since it helped me with thread-safety.

Now I'm inheriting it since it has asnyc methods to get/push back to the "queue". Otherwise, I would need to write my own implementation to select an item from the set asyncrounsly

ok, so I understood it correctly, I would suggest to call it pool instead of queue (just in case it is a public api)

yosriz · 2019-03-28T06:46:53Z

kin/monitors.py

-        :param threading.Event stop_event: an event that can be used to stop this method
-        """
-        import json
+async def single_monitor(kin_client: 'KinClient', address: str,


why does the KinClinet is wrapped with ' ?

Kin client imports "Single Monitor"

If I wanted to write KinClient without '' I would need to import KinClient in SingleMonitor, and then I would have a circular loop of importing. So instead of importing the type you can just write it as a string. That's called 'forward referencing'

https://stackoverflow.com/questions/39740632/python-type-hinting-without-cyclic-imports

python is fun!

yosriz · 2019-03-28T06:47:55Z

kin/monitors.py

+    :raises: asyncio.TimeoutError: If too much time has passed between events (only if "timeout" is set)
+    """
+    if not is_valid_address(address):
+        raise StellarAddressInvalidError('invalid address: {}'.format(address))


can we remove Stellar from this error name?

yosriz · 2019-03-28T06:48:40Z

kin/monitors.py

-        import json
+            tx_data = SimplifiedTransaction(RawTransaction(tx))
+        except CantSimplifyError:
+            logger.debug("SSE transaction couldn't be simplified: ", tx)


what's exactly this logger? is it controllable by the developer?

Just a logger like any other. And yes, you can set it's level

See https://github.com/kinecosystem/migration-server/blob/master/scripts/src/init.py#L42

yosriz · 2019-03-28T06:50:45Z

kin/monitors.py

+            logger.debug("Non-payment SSE transaction skipped: ", tx_data)
+            continue
+
+        # Will yield twice if both of these are correct. (someone sent to himself) - which it fine


why not check both and send an only single event? (not crucial as it's an edge case, but don't see a reason to fire twice)

This is just how it worked so far so I didnt wanted to change it

yosriz · 2019-03-28T06:56:31Z

test/test_monitor.py

-    sleep(10)
-    assert not monitor.thread.is_alive()
-    assert hash4 not in txs_found2
+    pass


what exactly is broken? does it happen also on testnet?

I tested my sse implementation manually and it works, but yes we get these 404s from time to time so I didn't want to get my test to fail because of a BC issue

ronserruya added 13 commits March 13, 2019 13:53

Add async to kin-sdk

d152bbb

Allign errors with new kin-base HorizonError

1f283af

Add async and type hints

82b8ee3

Add type hints to transactions

0b27b1a

Add tests to async functionality

ef83c6c

Update requirments

daa45bd

Update intergration tests images

3d7c832

Fix typo in images

aa3826e

Update requirments

5c99b1c

Update req-dev

0e92157

Fix module name

b2e2cb9

Fix module name

baeb8a7

Update network passphrase

c0b67ed

Ronserruya requested review from doodyparizada and yosriz March 24, 2019 10:48

Remove todo

4e3ee36

ronserruya added 2 commits March 24, 2019 14:38

Add close method to KinClient

eaa0feb

Update readme

d46d74d

Ronserruya requested a review from mrsegev March 24, 2019 16:08

ronserruya added 5 commits March 25, 2019 10:20

Disable codecov patch warning

75941a1

Update setup.py

91824d1

Use context manager when creating channels

a3d8c5d

Remove codecov.yml

f6744ae

Adjust comments to improve docs generation

ca88477

mrsegev reviewed Mar 27, 2019

View reviewed changes

lock req-dev versions

779c6a2

yosriz approved these changes Mar 28, 2019

View reviewed changes

Ronserruya merged commit 38db287 into v2-master Apr 7, 2019

Ronserruya deleted the async_kin branch April 7, 2019 11:06

Conversation

Ronserruya commented Mar 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mrsegev 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!

yosriz 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!

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ronserruya commented Mar 24, 2019 •

edited

Loading

codecov bot commented Mar 24, 2019 •

edited

Loading