Skip to content
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

Feature database transaction storage #617

Merged
merged 10 commits into from
Jul 16, 2018

Conversation

rwrzesien
Copy link
Contributor

@rwrzesien rwrzesien commented Jul 9, 2018

Resolves #584

Adds GlobalTransactionState and PendingEthereumTransaction models.
Adds DatabaseTransactionsStorage and its usage in ConcentRPC.
Adds get_transaction_count to payments.service.

Marked [WIP] because:

  • is waiting for answer from related issue about soft delete.
  • there is some code to uncomment after udpate to SCI 1.4.1 is resolved (link to issue),
  • it need to be tested on cluster.

Also:
Resolves #605
Resolves #607

value = CharField(max_length=32)

to = BinaryField(max_length=20)
data = BinaryField()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After testing new SCI I have discovered that data is not BinaryField it is really long string -> probably as a data we will need TextField()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is string most probably from the same reason as in #617 (comment)

I have decoded the data from sample which you have provided, so I have retrieved the original value from transaction, used it in tests, and it worked fine.

startgas = CharField(max_length=32)
value = CharField(max_length=32)

to = BinaryField(max_length=20)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field to in transaction.json is stored as a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's most probably because JSON doesn't support binary data, so it has to be converted to string. I think our point is to be able to store and recreate Transaction to be exactly the same, not storing the data in the same format as JsonTransactionsStorage, but if I am wrong then please correct me. @cameel ?

Btw. 'I think our point is to be able to store and recreate Transaction' is a good scenario for test, I will add it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. We want to store the original transaction, not a transaction mangled by being converted to JSON.


nonce = DecimalField(max_digits=32, decimal_places=0, unique=True)

gasprice = CharField(max_length=32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gasprice in new SCI is stored as integer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets make it Decimal then with max_digits set to 32.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decimal is fine. But 32 digits is too little. A 32-byte integer has 256 binary digits and the number of decimal digits is about 1/3 of that:

>>> len(str(2 ** 256 - 1))
78

nonce = DecimalField(max_digits=32, decimal_places=0, unique=True)

gasprice = CharField(max_length=32)
startgas = CharField(max_length=32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

startgas in new SCI is stored as integer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets make it Decimal then with max_digits set to 32.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment above: #617 (comment)

data = BinaryField()

v = IntegerField()
r = CharField(max_length=32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field r in transaction.json is stored as a integer. In my tests i had r field which equaled 105276041803796697890139158600495981346175539693000174052040367753737207356915

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have checked that even BigIntegerField can store numbers in range from -9223372036854775808 to 9223372036854775807 so it is too small for our needs. I think the best would be to convert this number to string and then store as TextField()

Copy link
Contributor Author

@rwrzesien rwrzesien Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that what I was already pointing out on daily few days ago, BigIntegerField is not enough for this, but Decimal could do in this case, as I now know how sample this data will look exactly in Transaction.

@kbeker I see you have liked TextField very much but believe me it is not a cure for everything ;)

In my tests i had r field which equaled 105276041803796697890139158600495981346175539693000174052040367753737207356915

And that's 78 digits for Decimal field (which requires max_digits parameter) so I would set it safely to 128. Would that be ok @cameel ? Or can we somehow confirm maximum value of it ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maximum value for a 32-byte unsigned int is just 2**(32*8)-1. 78 decimal digits is enough.


v = IntegerField()
r = CharField(max_length=32)
s = CharField(max_length=32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With s field same story as with r field

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I will change it to Decimal too.

)
except GlobalTransactionState.DoesNotExist:
logger.error(f'Trying to get GlobalTransactionState but it does not exist.')
raise
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will need also method which will get oldest transaction which hasn't been proceed yet. @cameel what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nonce does that. By checking its value you can figure out which transaction was added last.

Copy link
Contributor Author

@rwrzesien rwrzesien Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r = int(ethereum_transaction.r),
s = int(ethereum_transaction.s),
to = encode_hex(ethereum_transaction.to.tobytes()),
data = ethereum_transaction.data.tobytes(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style:)

Copy link
Contributor

@cameel cameel Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's technically against our rules but in this case it clearly improves readability. Maybe we should allow for some exceptions?

@dybi What do you think?

When reviewing this is a lot easier to parse visually

nonce    = int(ethereum_transaction.nonce),
gasprice = int(ethereum_transaction.gasprice),
startgas = int(ethereum_transaction.startgas),
value    = int(ethereum_transaction.value),
v        = int(ethereum_transaction.v),
r        = int(ethereum_transaction.r),
s        = int(ethereum_transaction.s),
to       = encode_hex(ethereum_transaction.to.tobytes()),
data     = ethereum_transaction.data.tobytes(),

than this

nonce = int(ethereum_transaction.nonce),
gasprice = int(ethereum_transaction.gasprice),
startgas = int(ethereum_transaction.startgas),
value = int(ethereum_transaction.value),
v = int(ethereum_transaction.v),
r = int(ethereum_transaction.r),
s = int(ethereum_transaction.s),
to = encode_hex(ethereum_transaction.to.tobytes()),
data = ethereum_transaction.data.tobytes(),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly, my point here was not to follow old rules but to occasionally break new rules for better readability. But if anyone will insist, I will change it.

@cameel It would be even less readable that in the second part of your example, because there should be no spaces around =.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwrzesien. @cameel quite frankly I have really mixed feelings about this . What I mean is - we have pretty strict code style rules and we been struggling for a while to find consensus that all of us would eventually agree upon. I am afraid that having "one exception" here could lead to start the whole discussion again.
My point of view is - if we want to allow some exceptions or loosen up a bit our strict style rules, the best way to do this would be through discussion on retro.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer for it to be allowed but you have a point. @rwrzesien please raise this issue on the retro.

r = tx.r,
s = tx.s,
data = tx.data,
to = tx.to,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style:)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, done for readability, but if you or anyone insist I will change it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me is okay I didn't want to change our style and for me it is more readable :) I have just pointed this because it's not matched with our current style

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reformat (style)


gasprice = CharField(max_length=32)
startgas = CharField(max_length=32)
value = CharField(max_length=32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't PostgreSQL support big integers? If not it would be better to at least use Decimal. We only want integers in this field, not arbitrary strings.

Copy link
Contributor Author

@rwrzesien rwrzesien Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It supports, but big integer is not enough for the data from sample provided by @kbeker . I will switch to Decimal wherever possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Decimal is fine.



def get_transaction_count() -> int:
return 10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A null backend should not return arbitrary numbers like this. It should always be zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -83,6 +83,10 @@ def is_account_status_positive(
return client_acc_balance > pending_value


def get_transaction_count() -> int:
return ConcentRPC().get_transaction_count()
Copy link
Contributor

@cameel cameel Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not have a chance to ask earlier: why is it called ConcentRPC? It's strictly related to SCI and Geth and its name should reflect that. RPC = Remote Procedure Call. It's too generic. I would never realize that it's the payment backend/API if I did not know that already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually have no idea, but if we want to change it lets do this in separate issue.

@kbeker Maybe you can explain ?

@cameel Isn't it RemoTe Procedure Call ?

Copy link
Contributor

@cameel cameel Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cameel Isn't it RemoTe Procedure Call ?

Yeah. Fixed my comment.

I actually have no idea, but if we want to change it lets do this in separate issue.

So let's create an issue. Or just a separate pull request. It's a simple thing but it's been bugging me for a long time :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created #640

if not GlobalTransactionState.objects.filter(pk=0).exists():
global_transaction_state = GlobalTransactionState(
pk=0,
nonce=payments_service.get_transaction_count(), # pylint: disable=no-value-for-parameter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you come up with a more general solution for this warning? We don't want to disable it altogether but it should be possible to do this for a specific class.

Copy link
Contributor Author

@rwrzesien rwrzesien Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible for a class, however this is a module. I can add backend = None in all function signatures in core.payments.service, and that will do the job. Is that ok ? backend is added automatically by decorator, however function will crash without backend.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we don't want unnecessary defaults if we can avoid it.

What about the status of this in pylint? Are there any chances that they'll fix it soon?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think not soon according to pylint-dev/pylint#259 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. So let's ignore it.

BTW, I think I have already responded to this. Where did my comment go?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here :) #617 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right :) That's where it went. Sorry :)

raise Exception(
f'Transaction nonce does not match. '
f'Current={global_transaction_state.nonce}, '
f'Transaction={tx.nonce}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a custom exception class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have followed up the fact that JsonTransactionsStorage is raising pure Exception, and assumed that it might matter, but if you think it doesn't then off course, lets make it a custom exception class.

Copy link
Contributor

@cameel cameel Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's define a custom exception then. Even if it was checked in any way, a custom exception would inherit from Exception anyway.

We actually should add it to our coding style: Code-Poets/coding-style#10

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, OK. Let's keep ignoring it for now then.

try:
return GlobalTransactionState.objects.select_for_update().get(
pk=0,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're never releasing the lock explicitly. From the docs I see that the lock lasts "until the end of the transaction block". The thing is that we will likely have more than one transaction block and they're nested: one explicitly started with @transaction.atomic above and one that's by default on the view (due to ATOMIC_REQUESTS). Could you verify that it's the former that releases the lock? We definitely don't want to keep the row locked until the end of the view.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have checked it positively however I don't really know how to test it negatively (to cause crash because of lock). Please consider the following code:

with transaction.atomic(using='control'):
    with transaction.atomic(using='control'):
        s = Subtask.objects.select_for_update().get(pk=7)
    s = Subtask.objects.get(pk=7)

It works just fine. Do you think its enough ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this?

with transaction.atomic(using='control'):
    with transaction.atomic(using='control'):
        s = Subtask.objects.select_for_update().get(pk=7)
    s = Subtask.objects.select_for_update().get(pk=7)

This should deadlock if you run it and unless the first atomic() releases the lock.

Copy link
Contributor Author

@rwrzesien rwrzesien Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code works without errors, so the inner lock is released with the end of inner context manager.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Great.

try:
pending_ethereum_transaction = PendingEthereumTransaction.objects.get(
nonce=global_transaction_state.nonce - 1
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your're not decreasing the nonce. This means that you can't for example call revert_last_tx() twice to revert two last transactions.

Is this intentional (maybe it's how SCI works)? Or is it a bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake, I thought nonce can never ever be used twice, but I have now checked that JsonTransactionsStorage is doing it.

"""
global_transaction_state = self._get_locked_global_transaction_state()

if global_transaction_state.nonce != tx.nonce:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be tx.nonce - 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ? As I understand, the transaction nonce should match 'current' nonce. It is done the same way in https://github.com/golemfactory/golem-smart-contracts-interface/blob/v1.4.2/golem_sci/transactionsstorage.py#L98

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But isn't tx the new transaction that already has a nonce set to one higher?

I was assuming that nonce is increased only after the new transaction gets added.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was assuming that nonce is increased only after the new transaction gets added.

Exactly, so the current transaction should have current nonce, right ? At least it looks like this in JsonTransactionsStorage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if the JSON version does this like this then it's fine.

pending_ethereum_transaction = PendingEthereumTransaction(
nonce=self.global_transaction_state.nonce,
gasprice=10 ** 9,
startgas=21000,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make at least one test that checks the full range of integers. We want to be sure that it does not break with very large numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but as you see in the discussion in other comments I am still not sure what are maximum values here.

Copy link
Contributor

@cameel cameel Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sizes stated in the issue imply maximum values. For example for a 32-byte unsigned integer the maximum value is 2**(32*8)-1

v = IntegerField()
r = CharField(max_length=32)
s = CharField(max_length=32)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cameel @rwrzesien
Don't we need also a field which will tell SCI which transactions was already done and which are awaiting to proceed?

Copy link
Contributor

@cameel cameel Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need the stuff that's in transactions.json. If there's such a field, please make a comment in the issue and I'll update the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there is no such field there.

@cameel
Copy link
Contributor

cameel commented Jul 9, 2018

Refactors core.payments file structure.

I think it would be better to extract this into a separate pull request so that it does not have to wait until this pull request is merged. I did not see any problems with the refactoring itself so it should go through easily and it's a distraction from the important changes.

@rwrzesien rwrzesien force-pushed the feature-database-transaction-storage branch from d4e5702 to f693332 Compare July 9, 2018 23:05
@rwrzesien
Copy link
Contributor Author

@cameel

I think it would be better to extract this into a separate pull request so that it does not have to wait until this pull request is merged. I did not see any problems with the refactoring itself so it should go through easily and it's a distraction from the important changes.

Done -> #620

git cherry-pick is a kind of magic.

@rwrzesien rwrzesien force-pushed the feature-database-transaction-storage branch 3 times, most recently from b6f6a25 to b23e536 Compare July 10, 2018 01:26
@kbeker kbeker force-pushed the feature-database-transaction-storage branch 2 times, most recently from 82ce692 to d9afaa3 Compare July 12, 2018 07:32
@kbeker kbeker changed the title [WIP] Feature database transaction storage Feature database transaction storage Jul 12, 2018
Copy link
Contributor

@dybi dybi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests seems pretty exhaustive ;) Just two minor style issues.

"""
return [
Transaction(
nonce = int(ethereum_transaction.nonce),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reformat (style)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

r = tx.r,
s = tx.s,
data = tx.data,
to = tx.to,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reformat (style)

from golem_sci.factory import new_sci_rpc
from golem_sci.client import Client
from golem_sci.contracts.provider import ContractDataProvider

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove blank line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

def concent_sci(
storage,
web3,
address,
Copy link
Contributor Author

@rwrzesien rwrzesien Jul 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix indentation. If possible add typings. A comment would be nice too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

address,
tx_sign,
):
if geth_poa_middleware not in web3.middleware_stack:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add assert that storage is subclass of TransactionStorage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Returns the nonce for the next transaction.
"""
global_transaction_state = self._get_locked_global_transaction_state()
return int(global_transaction_state.nonce)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add method to GlobalTransactionState called get_nonce which will change it to int and reuse it in all places.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -23,3 +23,7 @@ class GolemMessageValidationError(ConcentBaseException):

class FrameNumberValidationError(ConcentBaseException):
pass


class TransactionNonceNotMatchError(ConcentBaseException):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it has to inherit from ConcentBaseException and has error code. It won't be handled by decorator and returned to client. It is internally handled by SCI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also fix the name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


v = IntegerField()
r = DecimalField(max_digits=128, decimal_places=0)
s = DecimalField(max_digits=128, decimal_places=0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many digits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

nonce = DecimalField(max_digits=32, decimal_places=0)

gasprice = DecimalField(max_digits=32, decimal_places=0)
startgas = DecimalField(max_digits=32, decimal_places=0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not enough digits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -1,6 +1,8 @@
from enum import Enum

from golem_sci.blockshelper import BlocksHelper
from web3 import Web3

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're doing this, we should add web3 to our dependencies. And remove it later when we stop using it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to requirements.txt

@@ -83,4 +90,5 @@ toolz==0.9.0
typed-ast==1.1.0
urllib3==1.23
vine==1.1.4
web3==3.16.4
web3==4.2.1
websockets==5.0.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does SCI depend on websockets?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

websockets depends on web3 and is installing during it's installation. Look:
Collecting websockets>=4.0.1 (from web3==4.2.1)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird but OK.

eth-keys==0.1.0b4
eth-tester==0.1.0b15
eth-utils==0.7.4
django-constance==2.2.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the dependency on django-constance being changed here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we talked recently on slack and you decided that if something is in pip freeze it should be also in requirements.lock This is how django-constance[database] is installed by pip:

django-constance==2.2.0
django-picklefield==1.0.0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's completely unrelated to this pull request. You should make this change separately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted from this PR

@@ -60,6 +60,7 @@ class ErrorCode(enum.Enum):
QUEUE_WRONG_STATE = 'queue.wrong_state'
REQUEST_BODY_NOT_EMPTY = 'request_body.not_empty'
SUBTASK_DUPLICATE_REQUEST = 'subtask.duplicate_request'
TRANSACTION_NONCE_NOT_MATCH = 'transaction.nonce.not_match'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TRANSACTION_NONCE_DOES_NOT_MATCH or TRANSACTION_NONCE_MISMATCH would be a better name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@kbeker kbeker force-pushed the feature-database-transaction-storage branch 2 times, most recently from 5fef383 to 2824d6d Compare July 13, 2018 06:48
@kbeker kbeker force-pushed the feature-database-transaction-storage branch from 2824d6d to f6890f9 Compare July 13, 2018 07:15
web3: Web3,
address: Web3,
tx_sign: Callable[[Transaction], None],
):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can add -> SCIImplementation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

f'Transaction nonce does not match. '
f'Current={global_transaction_state.nonce}, '
f'Transaction={tx.nonce}',
ErrorCode.TRANSACTION_NONCE_MISMATCH,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't need error code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but won't it be better to have such a thing? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it won't be visible anywhere, it will be just passed to Exception __init__ and disappear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, so I delete it :)

@kbeker kbeker force-pushed the feature-database-transaction-storage branch from d3a7357 to 8d4ce44 Compare July 13, 2018 11:36
nonce initialized with a value obtained from SCI (Client.get_transaction_count()).
"""

nonce = DecimalField(max_digits=32, decimal_places=0, default=0, unique=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't nonce have max_digits=78 too ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


from core.models import GlobalTransactionState
from core.models import PendingEthereumTransaction

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove blank line and order alphabetically.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@kbeker kbeker force-pushed the feature-database-transaction-storage branch from 8d4ce44 to fade04a Compare July 13, 2018 13:43
Copy link
Contributor

@cameel cameel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some more remarks but all the really important issues have been addressed already.


# Temporary SCI factory for Concent needs.
# Gets storage as a parameter
def concent_sci(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a good idea to say in a comment where the original code came from.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


nonce = DecimalField(max_digits=32, decimal_places=0, default=0, unique=True)

def save(self, *args, **kwargs): # pylint: disable=arguments-differ
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this warning need to be disabled here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PILINT gives warning if there is no disable:
Parameters differ from overridden 'save' method (arguments-differ)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So please define it with the same parameters.


@property
def get_nonce(self):
return int(self.nonce)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like a function name. Properties should be named like variables (i.e. using nouns, not verbs). E.g. nonce_as_int.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleted, just as you said in next comment


@property
def get_nonce(self):
return int(self.nonce)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that these properties take more code than they actually save. It would be better to just call int() whenever you access a decimal field.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

requestor_address = requestor_eth_address,
provider_address = provider_eth_address,
requestor_address = Web3.toChecksumAddress(requestor_eth_address),
provider_address = Web3.toChecksumAddress(provider_eth_address),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to let get_forced_payments() do the address conversion internally? It looks like an implementation detail to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, we need to do this conversion before we give addresses as parameters. If not some exception is raised.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But get_force_payments() is our function. If it raises an exception then maybe it shouldn't?

global_transaction_state = self._get_locked_global_transaction_state()

if global_transaction_state.get_nonce != tx.nonce:
raise TransactionNonceNotMatchError(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe TransactionNonceMismatch would be a better name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@kbeker kbeker force-pushed the feature-database-transaction-storage branch from fade04a to c714b8e Compare July 16, 2018 08:17
@kbeker kbeker force-pushed the feature-database-transaction-storage branch from c714b8e to d5f4a97 Compare July 16, 2018 08:44
@kbeker kbeker force-pushed the feature-database-transaction-storage branch from d5f4a97 to 704d43e Compare July 16, 2018 09:07
@kbeker kbeker merged commit 704d43e into master Jul 16, 2018
@kbeker kbeker deleted the feature-database-transaction-storage branch July 16, 2018 09:22
@cameel cameel added this to the 0.8.0 milestone Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants