-
Notifications
You must be signed in to change notification settings - Fork 5
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
RequestCoreService #9
Conversation
servicesCore/requestCore_service.py
Outdated
from artifacts import * | ||
from config import config | ||
from servicesContracts.requestEthereum_service import RequestEthereumService | ||
import servicesExtensions |
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.
Is there a reason you need to import all of servicesExtensions
? Same for servicesContracts
below.
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.
nope, should be an explicit import
servicesCore/requestCore_service.py
Outdated
pass | ||
self._web3Single = Web3Single.getInstance() | ||
self._ipfs = Ipfs.getInstance() | ||
# TODO: load requestCoreArtifact using JSONLoader |
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.
Is this still a TODO? It should be loaded automatically by the artifacts
module.
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.
no longer at TODO
servicesCore/requestCore_service.py
Outdated
self._abiRequestCore = requestCoreArtifact['abi'] | ||
if not requestCoreArtifact['networks'][self._web3Single.networkName]: | ||
raise ValueError('RequestCore Artifact does not have configuration for network: "' + self._web3Single.networkName + '"') | ||
self._addressRequestCore = requestCoreArtifact['networks'][self._web3Single.networkName].address |
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.
Does the .address
here work?
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.
Nope, should be ['address']
servicesCore/requestCore_service.py
Outdated
|
||
def getRequest(self, requestId: str): | ||
pass | ||
if extension and extension != '' and not self._web3Single.isAddressNoChecksum(extension): |
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.
An empty string already evaluates to False
so no need for extension != ''
.
raise ValueError('request not found') | ||
|
||
# excluding BN | ||
dataResult = { |
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.
Is data here a data structure or a dictionary?
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.
Unclear. The web3
documentation isn't super explicit (I think v4 changed the Contract api).
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.
Then we'll assume that it's fine and if it errors out later it's an easy fix.
servicesCore/requestCore_service.py
Outdated
|
||
ccyContract = transaction.to | ||
|
||
ccyContractservice = await servicesContracts.getServiceFromAddress(ccyContract) |
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.
ccyContractService
instead of service
servicesCore/requestCore_service.py
Outdated
try: | ||
test = await self._web3Single.callMethod(methodGenerated, options) | ||
except Exception as e: | ||
warnings.append('transaction may failed: ' + e) |
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.
may have failed
servicesCore/requestCore_service.py
Outdated
if transaction.gasPrice < config.ethereum.gasPriceMinimumCriticalInWei: | ||
warnings.append('transaction gasPrice is low') | ||
|
||
errors = None if not errors else errors |
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.
You can do errors = errors or None
and if errors == []
it will be None
. Same for warnings below.
servicesCore/requestCore_service.py
Outdated
|
||
errors = None if not errors else errors | ||
warnings = None if not warnings else warnings | ||
return (request, transaction, errors, warnings) |
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.
Not sure if we have to do any of the resolve
stuff here.
servicesCore/requestCore_service.py
Outdated
}) | ||
|
||
# clean the data and get timestamp for request as payee | ||
# TODO: implement this, I believe it is just adding |
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.
If this isn't done can you have this raise a NotImplemented
error so that people don't call it accidentally. Also maybe open an issue for it so that people don't scroll by and see that it's not pass
and assume that it's done.
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.
will do
raise ValueError('request not found') | ||
|
||
# excluding BN | ||
dataResult = { |
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.
Then we'll assume that it's fine and if it errors out later it's an easy fix.
Not 100% complete, just thought I'd submit a PR before heading to bed
Ported most of
RequestCoreService
Left some data cleaning in
getRequestsByAddress
, mainly because the typescript was doing some weird promise + map thing and flu-Justice doesn't like thinkingOh, and docstrings, I should probably do those