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

Bug-fix outport data provider #5043

Merged
merged 4 commits into from
Mar 3, 2023

Conversation

miiu96
Copy link
Contributor

@miiu96 miiu96 commented Mar 1, 2023

Reasoning behind the pull request

  • Bug-fix the case when transactionsFeeProcessor component found a smart contract result with a refund but the original transaction is not found in storage/

Proposed changes

  • For the above case the smart contract result with a refund will be ignored.

Testing procedure

  • Verify that the message shardProcessor.indexBlockIfNeeded cannot prepare argSaveBlock will no appears in the log files.

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

@miiu96 miiu96 self-assigned this Mar 1, 2023
@iulianpascalau iulianpascalau self-requested a review March 1, 2023 10:30
@@ -160,7 +164,8 @@ func (tep *transactionsFeeProcessor) prepareScrsNoTx(transactionsAndScrs *transa

txFromStorage, err := tep.txGetter.GetTxByHash(scr.OriginalTxHash)
if err != nil {
return err
tep.log.Trace("transactionsFeeProcessor.prepareScrsNoTx: cannot find transaction in storage", "hash", scr.OriginalTxHash, "error", err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

the only clean way to test that the log.Trace function was called 👍

@@ -37,6 +40,7 @@ func NewTransactionsFeeProcessor(arg ArgTransactionsFeeProcessor) (*transactions
txFeeCalculator: arg.TxFeeCalculator,
shardCoordinator: arg.ShardCoordinator,
txGetter: newTxGetter(arg.TransactionsStorer, arg.Marshaller),
log: logger.GetOrCreate("outport/process/transactionsfee"),
Copy link
Contributor

Choose a reason for hiding this comment

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

we can either have the constant string defined as a constant or the log instance declared as a var to comply with the rest of the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a const for the logger name

iulianpascalau
iulianpascalau previously approved these changes Mar 1, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2023

Codecov Report

❗ No coverage uploaded for pull request base (feat/polaris-fixes@3c57b72). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 365bf91 differs from pull request most recent head 3657017. Consider uploading reports for the commit 3657017 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@                  Coverage Diff                  @@
##             feat/polaris-fixes    #5043   +/-   ##
=====================================================
  Coverage                      ?   70.79%           
=====================================================
  Files                         ?      649           
  Lines                         ?    85376           
  Branches                      ?        0           
=====================================================
  Hits                          ?    60442           
  Misses                        ?    20395           
  Partials                      ?     4539           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ssd04 ssd04 self-requested a review March 1, 2023 10:40
ssd04
ssd04 previously approved these changes Mar 1, 2023
@miiu96 miiu96 changed the title Bug-fix ourport data provider Bug-fix outport data provider Mar 1, 2023
bogdan-rosianu
bogdan-rosianu previously approved these changes Mar 1, 2023
@miiu96 miiu96 changed the base branch from master to feat/polaris-fixes March 2, 2023 07:47
@miiu96 miiu96 dismissed stale reviews from bogdan-rosianu, ssd04, and iulianpascalau March 2, 2023 07:47

The base branch was changed.

miiu96 added 2 commits March 2, 2023 09:50
# Conflicts:
#	outport/process/transactionsfee/transactionsFeeProcessor.go
#	outport/process/transactionsfee/transactionsFeeProcessor_test.go
@miiu96 miiu96 merged commit 88a5873 into feat/polaris-fixes Mar 3, 2023
@miiu96 miiu96 deleted the fix-outport-data-provider-bug branch March 3, 2023 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants