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

Bugfix get list of payments does not search right range #1067

Merged
merged 2 commits into from
Jan 16, 2019

Conversation

dybi
Copy link
Contributor

@dybi dybi commented Jan 16, 2019

resolves: #960

Copy link
Contributor

@kbeker kbeker left a comment

Choose a reason for hiding this comment

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

download 1

Please answer me to mine question. Generally ok :)

concent_api/core/payments/backends/sci_backend.py Outdated Show resolved Hide resolved
)

return payments_list


def get_first_block_from_timestamp(sci: SmartContractsInterface, timestamp: int) -> Block:
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is almost the same as in BlocksHelper.get_first_block_after, only marks greater or equal in 73 and 78 lines are different. Is it necessary?

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, these marks are everything ;) In the long run we could make a PR in SCI repo to move it there and extract some common code, but to fix problem in concent, what's done here is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

issue: #1068

Copy link

Choose a reason for hiding this comment

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

Since timestamps are integers these two invocations are equivalent:
get_first_block_from_timestamp(ts) == get_first_block_after(ts - 1)
so this function is not necessary at all (nor any changes to SCI)

first_block_after_payment_number = get_first_block_from_timestamp(payment_interface, min_block_timestamp).number
latest_block_number = payment_interface.get_block_number()
if latest_block_number - first_block_after_payment_number < payment_interface.REQUIRED_CONFS:
return []
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it won't be better that if this would be a variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will this ever change? What could actually cause this situation? Maybe exception is better then? And maybe logging?

Copy link
Contributor Author

@dybi dybi Jan 16, 2019

Choose a reason for hiding this comment

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

#960 (comment)

in short: an empty list should be returned

Copy link
Contributor

@rwrzesien rwrzesien left a comment

Choose a reason for hiding this comment

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

Some clean up is needed.

Copy link
Contributor

@kbeker kbeker left a comment

Choose a reason for hiding this comment

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

good_job_well_done_face_sticker-r50c0f1d960e148f4b79b6dc9b184f237_v9wf3_8byvr_540

Approve:)

@dybi dybi force-pushed the bugfix-get-list-of-payments-does-not-search-right-range branch from 59787c1 to 7eacfa1 Compare January 16, 2019 13:04
Piotr Dybowski added 2 commits January 16, 2019 16:14
… for it

Now the function returns only confirmed blocks, so if there are no suitable blocks, an empty list is returned.
@dybi dybi force-pushed the bugfix-get-list-of-payments-does-not-search-right-range branch from 36f6547 to 3aee642 Compare January 16, 2019 15:32
@dybi dybi merged commit 89e3c96 into master Jan 16, 2019
@dybi dybi deleted the bugfix-get-list-of-payments-does-not-search-right-range branch January 16, 2019 15:33
@kbeker kbeker added this to the next milestone Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get_list_of_payments() does not search through the right range of blocks
4 participants