-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Monorepo #254
Monorepo #254
Conversation
|
|
https://github.com/singnet/snet-code-examples/pull/3/files Matches the JS SDK example |
I would propose to not make separate directory for snet_cli and basically do not touch old directory structure for now and just add sdk directory. I afraid this move would make more harm then good (for example CI is brorken because of this now... ) |
@vforvalerio87 Could you make your PR green before we review it? If you add some tests for SDK itself then it would be even greater... |
I suggest that we fix the CI for the folder structure ASAP. Infact I would propose that we merge in the new folder structure first to ensure that the client functionality is all fine. We can continue to work on the SDK in parallel. @vforvalerio87 lets get the CI for snet-cli fixed right away |
@raamb I do insist that we shoudn't touch directory structure of snet-cli. We could add sdk as separate folder, or, at first directly in snet-cli folder.... |
So, CircleCI config and scripts have been adapted to work in the monorepo scenario, build tests pass. I tried building and using both the CLI and SDK locally and they worked correctly, feel free to try the same. I changed the name of the "stub" object to "service" as we discussed. |
@vforvalerio87 will you add tests for SDK in this PR (again feel free to reuse test infrastructure from #246)? |
sure, I'll get to it as soon as I'm finished with updating the README |
snet_sdk/snet_sdk/__init__.py
Outdated
from snet_sdk.account import Account | ||
from snet_sdk.mpe_contract import MPEContract | ||
from snet_sdk.payment_channel_management_strategies.default import PaymentChannelManagementStrategy | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets use a similar approach to version as we did for the snet-cli i.e. use a version.py file which is changed via CD script. Makes it easier to control versioning in the code and Pypi. Can be taken up as its own PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I had it like that because it used to be in init when I worked on the first iteration of the SDK... didn't notice it had changed places since then. I'll move it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the suggestions were already pointed by others:
-
Re-use blockchain/
-
Re-use scripts/
-
test/ should go to the main project folder
-
Maybe use just one .gitignore?
-
setup.py:
- python_requires='>=3.6',
- url='https://github.com/singnet/snet-sdk-python', -> "monorepo"
-
MANIFEST.in:
- snet_sdk/resources/proto/* doesn't exist.
- Re-use resources/ ?
-
VERSION (sdk gets it in a different way from cli).
-
Unused imports (init.py):
- import operator
- from functools import reduce
- import sys
- import os
- import importlib
- import base64
- from pathlib import PurePath, Path
- import hashlib
- import collections
- from eth_account.messages import defunct_hash_message
- import snet_sdk.generic_client_interceptor as generic_client_interceptor -
init.py#L58
create_service_client(..., options={}) -> Mutable argument.
create_service_client(..., options=None)
if options is None:
options = dict() -
Suggestion (mpe_contract.py):
MaybeEVENT_ABI
could be set like it is insnet_cli/mpe_channel_command.py#L423
-
Unused arguments (service_client.py#L81):
request_streaming
andresponse_streaming
-
Remove compiled
state_service.proto
codes.
It gives the following error:
I suppose it is not working with web3==4.8.3 which snet-cli requires. So I was not able to run your example... |
@vforvalerio87 Could you provide two examples (better add them directly in functional tests):
Comment 1:
You are not doing it in Comment 2: |
snet_sdk/snet_sdk/__init__.py
Outdated
group = next(filter(lambda group: group["group_name"] == group_name, service_metadata.groups)) | ||
except StopIteration: | ||
raise ValueError("Group[name: {}] not found for orgId: {} and serviceId: {}".format(group_name, org_id, service_id)) | ||
service_client = ServiceClient(self, service_metadata, group, service_stub, payment_channel_management_strategy(self), options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should accept an instance rather than instantiate it here. That way we don't have to worry about freezing the constructor api for the PaymentChannelManagmentStrategy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Hey, I was working on the CLI command to compile the client libraries for JS. I reviewed all the comments:
|
@vforvalerio87 have the commits been pushed? |
5c79127
to
3470ab7
Compare
Reorganized sdk and cli to be two different packages in the same repository Moved sdk to cli repo Rewrote SDK to match JS SDK implementation CircleCI fix
…el with interceptor in service_client instance
optional parameter Added call allowance to payment channel strategy as an optional parameter Split .gitignore to have a top level one and separate ones for each package Same for LICENSE Updated CLI README, added top-level README (sdk-specific is still in progress)
this monorepo is organized as a PEP 420 compliant namespace (https://www.python.org/dev/peps/pep-0420/). The sdk package has been restructured to conform to the PEP 420 namespace package specification and it's under the "snet" namespace. The cli package is both a regular package and a namespace package. setup.py for the monorepo itself installs the dependencies for all packages in the repository. Each package and the overarching monorepo also have "requirements.txt" files for local installations and CI enviroments: by installing via "requirements.txt", the dependencies listed in "setup.py" are only pulled and installed from PyPI if a local version is not already available. This allows to get dependencies within the monorepo to be installed from the local filesystem. The whole package is called "snet" and gives the name to the namespace. The subpackages are "snet.sdk", "snet.snet_cli" and snet_cli (since the CLI package is both within the namespace and a regular package). Going forward, all of the code in the "snet_cli" package can be migrated to the namespace package: both were kept momentarily for compatibility. In this scenario, everything inside the "snet.sdk" package has visibility on the contents of the "snet.snet_cli" package and vice-versa. Everything inside the "snet_cli" regular package has visibility on the contents of the "snet.snet_cli" package due to the regular Python import mechanics, but it has no visibility on the contents of the "snet.sdk" package (and vice-versa). So, the shared code between the SDK and the CLI goes under the "snet.snet_cli" package which is at "packages/snet_cli/snet/snet_cli". Code and assets deduplication: thanks to the new structure, everything under "resources" has been moved to the "snet.snet_cli" package and is now shared between the SDK and CLI. Specifically, this includes the json artifacts from the compilation and deployment of the smart contracts, the raw ".proto" files to interact with the daemon and the compiled python client libraries. Changes: since the CLI is both a namespace package and a regular package, changes to the code have been kept to a minimum. The only change of note is that state service does not compile its own ".proto" files at runtime inside the user's home directory anymore but they are imported from the "resources" directory under the "snet.snet_cli" namespace package, where they are compiled during setup.
Set up monorepo: this monorepo is organized as a PEP 420 compliant namespace The sdk package has been restructured to conform to the PEP 420 The cli package is both a regular package and a namespace package. setup.py for the monorepo itself installs the dependencies for all Each package and the overarching monorepo also have "requirements.txt" The whole package is called "snet" and gives the name to the namespace. In this scenario, everything inside the "snet.sdk" package has Code and assets deduplication: thanks to the new structure, everything under "resources" has been moved Changes: since the CLI is both a namespace package and a regular package, changes |
Done
I'd probably have each package have its own tests but we can discuss this
This is easier if stuff gets moved around since everything is relative to the package itself. More compartmentalized
Done
This is dead now. All resources are in the cli package
Done
Fixed
Fixed
Done
Done! |
getting abi for OpenChannel event from the contract object using web3 function passing instance of payment channel management strategy to service client
Obviously a lot more code can be shared and reused now, but I wanted to get the new repository structure understood and merged in first to serve as a basis for refactoring, especially to reuse the CLI code for dynamic compilation. Additional refactoring can come in later PRs |
… package, moved version identifier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Please add at least a simple test for SDK, we have dummy daemon and all necessary infrastructure in snet-cli for it...
-
The problem with this approach (in comparison to New sdk witch comply to new sdk-API (temporaly solution) #246 !!! ) that a lot of staff now is implemented differently in snet-cli and sdk (not talking about much bigger development time):
- gas_price_strategies
- functions to get channel state from the service
- functions to get channel for given service
- dynamic protobuf compilations (which present in snet-cli but absent in SDK)
- etc..
And it will cause problems. For example Full stateless logic #241 will fix_get_channel_state_statelessly
for snet-cli but not for your sdk...
I understand that the goal is to eliminate such duplication, but it is not clear how long it will take to do it...
packages/sdk/snet/sdk/__init__.py
Outdated
eth_rpc_endpoint = self._config.get("eth_rpc_endpoint", "https://mainnet.infura.io") | ||
provider = web3.HTTPProvider(eth_rpc_endpoint) | ||
self.web3 = web3.Web3(provider) | ||
self.web3.eth.setGasPriceStrategy(rpc_gas_price_strategy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this rpc_gas_price_strategy? How high gas price returned by this method in comparison to time based gas price strategies (fast, medium, slow). The problem is that after this PR snet-cli and sdk will use different gas price strategies...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't tested it compared to other strategies, I'll change it to default to medium or selectable by config parameter
packages/sdk/snet/sdk/__init__.py
Outdated
if options is None: | ||
options = dict() | ||
|
||
service_metadata = self.service_metadata(org_id, service_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I would rename function to get_service_metadata, otherwise there is a name "clash".
- In snet-cli we have special class for metadata (mpe_service_metadata.py). I would propose to use it here. It already has functions which we need here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Done
- Done
packages/sdk/snet/sdk/__init__.py
Outdated
|
||
service_metadata = self.service_metadata(org_id, service_id) | ||
try: | ||
group = next(filter(lambda group: group["group_name"] == group_name, service_metadata.groups)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MPEServiceMetadata already has function get_group. We could use it, instead of re-implementing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
packages/sdk/snet/sdk/__init__.py
Outdated
|
||
def service_metadata(self, org_id, service_id): | ||
(found, registration_id, metadata_uri, tags) = self.registry_contract.functions.getServiceRegistrationById(bytes(org_id, "utf-8"), bytes(service_id, "utf-8")).call() | ||
metadata = AttributeDict(json.loads(self.ipfs_client.cat(metadata_uri.rstrip(b"\0").decode('ascii')[7:]))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use snet_cli.utils_ips.get_from_ipfs_and_checkhash function here (we should checksum).
Please see MPEServiceCommand._get_service_metadata_from_registry
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
packages/sdk/snet/sdk/account.py
Outdated
def _send_signed_transaction(self, contract_fn, *args): | ||
transaction = contract_fn(*args).buildTransaction({ | ||
"chainId": int(self.web3.version.network), | ||
"gas": DEFAULT_GAS, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that we should specify "gas" parameter here. In snet-cli we don't do it and it is estimated automatically (see contract.py in snet-cli). By the way we could reuse contract.py
from snet-cli.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried removing it... it works locally but randomly fails for me when working with Kovan or Ropsten through Infura (ValueError: {'code': -32000, 'message': 'gas required exceeds allowance or always failing transaction'}
). Works every time if a high enough gas parameter is manually specified. The eth_estimateGas RPC seems to be unreliable on Infura.
I get the same error with the "client call" command of the CLI by the way.
|
||
from snet.snet_cli.utils import DefaultAttributeObject, get_web3, serializable, type_converter, get_contract_def, get_cli_version, bytes32_to_str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain again new directory structure. I do not understand some details.
Why utils.py are moved packages/snet_cli/snet/snet_cli
and the rest of snet-cli is in packages/snet_cli/snet_cli
. Why? Is utils.py still a part of snet-cli package? or is it in a separate package now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, now we have:
snet_cli (the regular, old package)
Then there is snet
which is a PEP 420 "namespace package" which has two subpackages:
snet.snet_cli
snet.sdk
PEP 420 namespace packages have the following directory structure:
<package_a> -> <namespace_package> -> <package_a>
If you have another package in your file system with a directory structure like <package_a> -> <namespace_package> -> <package_a>
, then package_a
and package_b
can share code between them. It can be imported as namespace_package.package_a
and namespace_package.package_b
from any of the two.
So, in the CLI package everything that is under snet_cli/snet_cli
is not shared; everything that is under snet_cli/snet/snet_cli
can be used in the sdk package by importing from snet.snet_cli
.
The sdk only needs what is under snet_cli/snet/snet_cli
to work (it will be published as a separate package), not the whole snet_cli
package.
self.block_offset = block_offset | ||
self.call_allowance = call_allowance | ||
|
||
def select_channel(self, service_client): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be consistent with snet-cli and dApp one should get channel with smallest channel id. See also singnet/snet-sdk-js#4 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
self.deployment_block = get_contract_deployment_block(self.web3, "MultiPartyEscrow.json") | ||
|
||
|
||
def get_past_open_channels(self, account, service, starting_block_number=0, to_block_number=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this function is so complicated? Why it is required to read logs in batches (while loop)??? Please take a look at MPEChannelCommand._get_all_filtered_channels
in mpe_channel_command.py
. It works perfectly without while loop. We should simply combine _get_all_channels_filter_sender_recipient_group
and _get_all_filtered_channels
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting all the logs from the deployment block to the current block is randomly giving me errors from Infura on both Ropsten and Kovan.
Also, _get_all_channels_filter_sender_recipient_group
uses _get_all_filtered_channels
which requires functions that rely on the CLI configuration and identity which work quite differently from the SDK.
I would open an issue for this, look into it and change it in a subsequent PR, after I'm finished with the dynamic compilation.
def _get_grpc_channel(self): | ||
endpoint = self.options.get("endpoint", None) | ||
if endpoint is None: | ||
endpoint = next(filter(lambda endpoint: endpoint["group_name"] == self.group["group_name"], self.metadata["endpoints"]))["endpoint"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would propose to use get_endpoints_for_group from MPEServiceMetadata here (and use MPEServiceMetadata class in general)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return self.web3.toHex(self.web3.eth.sendRawTransaction(signed_txn.rawTransaction)) | ||
|
||
|
||
def send_transaction(self, contract_fn, *args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why function for sending transaction is in Account class? In Account class two entities are mixed:
- class for ETH/MPE account (analog of MPEAccountCommand from snet-cli)
- class which sends on-chain transactions (analog of ContractCommand from snet-cli)
I would propose to reuse contract.py from snet-cli and probably write "better" version of ContractCommand, but not mixing two different entities here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contract.py
and ContractCommand are different. contract.py
does not do much.
Right now in the CLI sending transactions is all done by the Identity class as the process is different for all identity types, while only calls and building transactions is done by contract.py as that's the same for all identity types.
I can reuse contract.py for calls and building transactions in the SDK too, I can move the logic for sending transactions in another class (an analog to the CLI identity class but for the SDK), but I would not reuse the identity class or the ContractCommand class in the SDK as they are in the CLI as they rely heavily on CLI-specific configuration and identity
Hey there- I'm a bot, here to let you know that some code in this PR might not Please see that Pull Request's description for more details. |
…d package Refactored SDK package to use shared code
@arturgontijo @astroseger request your feedback on this. Thanks in advance |
Set up monorepo:
this monorepo is organized as a PEP 420 compliant namespace
(https://www.python.org/dev/peps/pep-0420/).
The sdk package has been restructured to conform to the PEP 420
namespace package specification and it's under the "snet" namespace.
The cli package is both a regular package and a namespace package.
setup.py for the monorepo itself installs the dependencies for all
packages in the repository.
Each package and the overarching monorepo also have "requirements.txt"
files for local installations and CI enviroments: by installing via
"requirements.txt", the dependencies listed in "setup.py" are only
pulled and installed from PyPI if a local version is not already
available. This allows to get dependencies within the monorepo to be
installed from the local filesystem.
The whole package is called "snet" and gives the name to the namespace.
The subpackages are "snet.sdk", "snet.snet_cli" and snet_cli (since the
CLI package is both within the namespace and a regular package).
Going forward, all of the code in the "snet_cli" package can be migrated
to the namespace package: both were kept momentarily for compatibility.
In this scenario, everything inside the "snet.sdk" package has
visibility on the contents of the "snet.snet_cli" package and
vice-versa.
Everything inside the "snet_cli" regular package has visibility on the
contents of the "snet.snet_cli" package due to the regular Python import
mechanics, but it has no visibility on the contents of the "snet.sdk"
package (and vice-versa).
So, the shared code between the SDK and the CLI goes under the
"snet.snet_cli" package which is at "packages/snet_cli/snet/snet_cli".
Code and assets deduplication:
thanks to the new structure, everything under "resources" has been moved
to the "snet.snet_cli" package and is now shared between the SDK and
CLI. Specifically, this includes the json artifacts from the compilation
and deployment of the smart contracts, the raw ".proto" files to
interact with the daemon and the compiled python client libraries.
Changes:
since the CLI is both a namespace package and a regular package, changes
to the code have been kept to a minimum.
The only change of note is that state service does not compile its own
".proto" files at runtime inside the user's home directory anymore but
they are imported from the "resources" directory under the
"snet.snet_cli" namespace package, where they are compiled during setup.