Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

Confirm outgoing GNT #4458

Merged
merged 4 commits into from
Jul 17, 2019
Merged

Confirm outgoing GNT #4458

merged 4 commits into from
Jul 17, 2019

Conversation

jiivan
Copy link
Contributor

@jiivan jiivan commented Jul 11, 2019

@igor discovered that GNT withdraw confirmation didn't work as expected.

@jiivan jiivan force-pushed the gnt_withdraw_confirmation branch from e86de45 to 4e4da37 Compare July 13, 2019 11:41
@jiivan jiivan changed the title [WIP] Confirm outgoing GNT Confirm outgoing GNT Jul 13, 2019
@jiivan jiivan requested review from Krigpl and kmazurek July 13, 2019 11:42
@jiivan jiivan force-pushed the gnt_withdraw_confirmation branch from 4e4da37 to 4439e9f Compare July 13, 2019 11:46
@jiivan jiivan marked this pull request as ready for review July 13, 2019 12:30
golem/ethereum/transactionsystem.py Outdated Show resolved Hide resolved
golem/ethereum/transactionsystem.py Outdated Show resolved Hide resolved
golem/ethereum/transactionsystem.py Outdated Show resolved Hide resolved
golem/ethereum/transactionsystem.py Outdated Show resolved Hide resolved
golem/ethereum/paymentskeeper.py Show resolved Hide resolved
tests/golem/ethereum/test_transactionsystem.py Outdated Show resolved Hide resolved
tests/golem/ethereum/test_transactionsystem.py Outdated Show resolved Hide resolved
tests/golem/ethereum/test_transactionsystem.py Outdated Show resolved Hide resolved
@@ -27,6 +32,21 @@
PASSWORD = 'derp'


def get_transaction_receipt(tx_hash, status=1):
return golem_sci.structs.TransactionReceipt(
raw_receipt={
Copy link
Contributor

Choose a reason for hiding this comment

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

Imo there's no reason for this function to be implemented this way, you could instead use Mock(spec=TransactionReceipt) and set fields you are interested in.
Especially that blockHash is still buggy just like transactionHash has been (raw bytes).

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 a simple struct. Why should I mock it?

Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid the kind of bugs that you already introduced (incorrect types in the raw_receipt).

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 got your point. But reason I introduced that function was exactly that. I wasn't sure what was the correct type. Once this function is bug-free it will be a good factory for all tests here. Better than mock IMO.

@jiivan jiivan force-pushed the gnt_withdraw_confirmation branch from 22ff4bb to 7964176 Compare July 15, 2019 13:29
@codecov
Copy link

codecov bot commented Jul 16, 2019

Codecov Report

Merging #4458 into develop will decrease coverage by 0.22%.
The diff coverage is 76%.

@@            Coverage Diff             @@
##           develop   #4458      +/-   ##
==========================================
- Coverage    90.13%   89.9%   -0.23%     
==========================================
  Files          222     222              
  Lines        19523   19545      +22     
==========================================
- Hits         17597   17572      -25     
- Misses        1926    1973      +47

@jiivan jiivan merged commit 736402f into develop Jul 17, 2019
@jiivan jiivan deleted the gnt_withdraw_confirmation branch July 17, 2019 09:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants