Resend txn & Handle dropped txns #70 #72

bshevchenko · 2019-08-28T15:35:23Z

Description

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code is formatted by rubocop.
If the external API is modified such the REST endpoints or GraphQL
schema, do update the documentation via apipie or the schema itself.
I have added tests to cover my code and critical edge cases.
I have run all tests and passes via bin/rake test with high
code coverage(at least 90%).
I have squashed my commit with a meaningful message before merging.
I have updated the documentation if needed and accordingly.
I have signed and submitted the Contributors License Agreement.
QA has tested and verified the PR.

bshevchenko · 2019-08-28T15:36:27Z

#70

bshevchenko · 2019-09-02T15:28:33Z

rubocop displays 1265 offenses from all over the project, so not sure what to do about it

FrancisMurilloDigix · 2019-09-04T02:41:46Z

@bshevchenko Don't worry about the warnings. We are using rubocop as the formatter, not a linter.

FrancisMurilloDigix · 2019-09-04T13:08:22Z

Can you run rubocop -a and only commit the files for this PR? In my editor, I usually have the files I touched autoformat with rubocop.

bshevchenko · 2019-09-04T13:28:43Z

@FrancisMurilloDigix done! Draft code for cron job is within the watching_transaction module (resend). Need your feedback on it. Job itself should be configured depending on the server environment, which I am not aware of.

Btw, as far as I am concerned, @roynalnaruto is supposed to be the reviewer for this PR, no?

FrancisMurilloDigix · 2019-09-04T13:42:21Z

@bshevchenko I am the main developer for the Rails application so I am also the reviewer. For the cron job, checkout scheduler.rb.

bshevchenko · 2019-09-04T13:46:11Z

@FrancisMurilloDigix all right, thank you! So shall I just put my code there under the "schedule.every '5m' do"?
Also what do you think on web3 initialization in the watching_transaction.rb? And on test coverage for resend method. I didn't test it yet, neither manually or automatically.

FrancisMurilloDigix · 2019-09-04T13:55:31Z

@bshevchenko 5m is fine.

Was going to comment whether we can remove the web3 dependency and simply put it in ethereum_api.rb.

FrancisMurilloDigix

Initial set of comments.

app/models/watching_transaction.rb

Gemfile

app/graphql/mutations/resend_transaction_mutation.rb

app/models/watching_transaction.rb

test/graphql/resend_transaction_mutation_test.rb

bshevchenko · 2019-09-04T21:47:52Z

@FrancisMurilloDigix ran out of your comments. Please check new commits

FrancisMurilloDigix

More comprehensive set of comments

app/graphql/mutations/resend_transaction_mutation.rb

app/graphql/mutations/watch_transaction_mutation.rb

app/models/watching_transaction.rb

app/graphql/resolvers/watched_transaction_resolver.rb

app/graphql/mutations/watch_transaction_mutation.rb

app/models/watching_transaction.rb

test/graphql/resend_transaction_mutation_test.rb

app/models/watching_transaction.rb

FrancisMurilloDigix

Still reviewing the tests.

The group_id for rewatch might be incorrect or misunderstood so watch out for that. Gonna try to stay open for tomorrow if you have more questions.

app/graphql/resolvers/watched_transaction_resolver.rb

app/models/watching_transaction.rb

test/graphql/resend_transaction_mutation_test.rb

test/factories/watching_transactions.rb

test/graphql/resend_transaction_mutation_test.rb

test/graphql/watch_transaction_mutation_test.rb

FrancisMurilloDigix

Almost done.

app/graphql/mutations/resend_transaction_mutation.rb

app/models/watching_transaction.rb

test/graphql/resend_transaction_mutation_test.rb

bshevchenko · 2019-09-09T16:42:44Z

So, as of now, I see 6 unresolved conversations that are related to:

Tests for WatchingTransaction model (watch, resend, resend_transactions)
Having transactionHash outside of the transactionObject argument

On 1 I'll soon provide new commits, but for 2 I need more feedback

FrancisMurilloDigix

For 2, you are right about the receipt and txhash so we have to take the extra txhash field for watchTransaction.

bshevchenko · 2019-09-10T19:39:53Z

@FrancisMurilloDigix check out my latest commit please

bshevchenko · 2019-09-10T19:43:09Z

@FrancisMurilloDigix or even 3 latest commits. They include what was lacking. I hope that's all

FrancisMurilloDigix

Almost there.

test/models/watching_transaction_test.rb

app/models/watching_transaction.rb

test/models/watching_transaction_test.rb

bshevchenko · 2019-09-11T10:54:37Z

@FrancisMurilloDigix everything seems to be resolved to me

FrancisMurilloDigix

Pretty pleased with this.

Just some enhancements on the test and one forgotten test on the WatchedTransaction

test/models/watching_transaction_test.rb

app/graphql/resolvers/watched_transaction_resolver.rb

bshevchenko · 2019-09-11T13:53:36Z

@FrancisMurilloDigix check latests commits please

FrancisMurilloDigix

Nicely done. When you're done addressing this last touch up.

Do rubocop -a and commit the files again.
Do squash the commits

FrancisMurilloDigix · 2019-09-12T01:52:54Z

test/models/watching_transaction_test.rb

+
+    WatchingTransaction.resend_transactions
+
+    assert_requested(get_failed_stub, times: 1)


Can you use size instead of 1 here?

It's supposed to be 1, because if API is unavailable then no more attempts will be made during current job

Actually it is called for each transaction so it is n times instead of 1. You should try setting it to n and then n+1 to see the effect.

No, it breaks whole cycle. Tests are passing

You missed few new lines in watching_transaction.rb:
if ok_tx == :error
Rails.logger.info 'Failed to get transaction by hash. Killing job..'
break
end

@FrancisMurilloDigix I'm not getting what you trying to say

It'll anyway break on the next iteration

if API would be still unavailable

Even if the next iteration fails and the reason is that the API is down or a network issue, then returning or breaking early is fine. Since error handling is not truly addressed, at least the consistency of behavior is the same when either request is down.

Yeah, so adding a break or return on a failed send_raw_transaction is good enough for now.

test/models/watching_transaction_test.rb

test/graphql/watched_transaction_query_test.rb

bshevchenko · 2019-09-12T07:13:35Z

Nicely done. When you're done addressing this last touch up.

Do rubocop -a and commit the files again.

Do squash the commits

All right, but I'm curious why you don't just use "Squash and merge" feature?

bshevchenko · 2019-09-12T07:55:40Z

@FrancisMurilloDigix done

FrancisMurilloDigix · 2019-09-12T21:21:51Z

Thanks for your hard work. Will merge after some advice from @tymat

Will add the remaining break and rubocop -a everything so the formatter just works hopefully.

bshevchenko marked this pull request as ready for review September 2, 2019 15:26

mandres-digix requested a review from FrancisMurilloDigix September 4, 2019 01:28

gitcoinbot mentioned this pull request Sep 4, 2019

[feature] Resend txn & Handle dropped txns #70

Open

FrancisMurilloDigix reviewed Sep 4, 2019

View reviewed changes

FrancisMurilloDigix reviewed Sep 5, 2019

View reviewed changes

FrancisMurilloDigix reviewed Sep 8, 2019

View reviewed changes

FrancisMurilloDigix reviewed Sep 9, 2019

View reviewed changes

FrancisMurilloDigix reviewed Sep 10, 2019

View reviewed changes

FrancisMurilloDigix requested changes Sep 11, 2019

View reviewed changes

FrancisMurilloDigix requested changes Sep 12, 2019

View reviewed changes

DigixGlobal#70 watching transactions

0b61e36

bshevchenko force-pushed the develop branch from 2510186 to 0b61e36 Compare September 12, 2019 07:53

FrancisMurilloDigix approved these changes Sep 12, 2019

View reviewed changes


		WatchingTransaction.resend_transactions

		assert_requested(get_failed_stub, times: 1)

Resend txn & Handle dropped txns #70 #72

Are you sure you want to change the base?

Resend txn & Handle dropped txns #70 #72

Conversation

bshevchenko commented Aug 28, 2019 • edited

Description

Types of changes

Checklist:

bshevchenko commented Aug 28, 2019

bshevchenko commented Sep 2, 2019

FrancisMurilloDigix commented Sep 4, 2019

FrancisMurilloDigix commented Sep 4, 2019

bshevchenko commented Sep 4, 2019

FrancisMurilloDigix commented Sep 4, 2019 • edited

bshevchenko commented Sep 4, 2019

FrancisMurilloDigix commented Sep 4, 2019

FrancisMurilloDigix left a comment

Choose a reason for hiding this comment

bshevchenko commented Sep 4, 2019

FrancisMurilloDigix left a comment

Choose a reason for hiding this comment

FrancisMurilloDigix left a comment

Choose a reason for hiding this comment

FrancisMurilloDigix left a comment

Choose a reason for hiding this comment

bshevchenko commented Sep 9, 2019 • edited

FrancisMurilloDigix left a comment • edited

Choose a reason for hiding this comment

bshevchenko commented Sep 10, 2019

bshevchenko commented Sep 10, 2019

FrancisMurilloDigix left a comment

Choose a reason for hiding this comment

bshevchenko commented Sep 11, 2019

FrancisMurilloDigix left a comment

Choose a reason for hiding this comment

bshevchenko commented Sep 11, 2019

FrancisMurilloDigix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bshevchenko Sep 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bshevchenko commented Sep 12, 2019

bshevchenko commented Sep 12, 2019

FrancisMurilloDigix commented Sep 12, 2019

bshevchenko commented Aug 28, 2019 •

edited

FrancisMurilloDigix commented Sep 4, 2019 •

edited

bshevchenko commented Sep 9, 2019 •

edited

FrancisMurilloDigix left a comment •

edited

bshevchenko Sep 12, 2019 •

edited