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

fix(connector-quorum): transaction with different credentials #1098 #1488

Merged
merged 2 commits into from
Nov 5, 2021

Conversation

AzaharaC
Copy link
Contributor

Resolves #1098

@AzaharaC AzaharaC self-assigned this Oct 28, 2021
@AzaharaC AzaharaC marked this pull request as ready for review October 29, 2021 06:21
@AzaharaC AzaharaC requested a review from takeutak October 29, 2021 06:21
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@AzaharaC LGTM, just have a small nit: Please include a logging statement on the debug level within the if condition that checks for a falsy gas value.
The logging statement should make it clear that gas was not explicitly specified and therefore we'll be using the estimate from web3 and also include what the estimated value came out to be.

…dger-cacti#1098

Signed-off-by: AzaharaC <a.castano.benito@accenture.com>
@AzaharaC
Copy link
Contributor Author

AzaharaC commented Nov 3, 2021

@AzaharaC LGTM, just have a small nit: Please include a logging statement on the debug level within the if condition that checks for a falsy gas value. The logging statement should make it clear that gas was not explicitly specified and therefore we'll be using the estimate from web3 and also include what the estimated value came out to be.

I updated the PR to include the logging statement

@AzaharaC AzaharaC requested a review from petermetz November 3, 2021 17:03
@codecov-commenter
Copy link

codecov-commenter commented Nov 3, 2021

Codecov Report

Merging #1488 (6e5babc) into main (9022064) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

❗ Current head 6e5babc differs from pull request most recent head fa238de. Consider uploading reports for the commit fa238de to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1488      +/-   ##
==========================================
- Coverage   69.80%   69.78%   -0.03%     
==========================================
  Files         336      336              
  Lines       12622    12626       +4     
  Branches     1512     1513       +1     
==========================================
  Hits         8811     8811              
- Misses       2989     2992       +3     
- Partials      822      823       +1     
Impacted Files Coverage Δ
.../main/typescript/plugin-ledger-connector-quorum.ts 74.44% <0.00%> (-1.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9022064...fa238de. Read the comment docs.

Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@AzaharaC Thank you for adding the logging statements! LGTM

@petermetz petermetz merged commit af6c240 into hyperledger-cacti:main Nov 5, 2021
@AzaharaC AzaharaC deleted the feat/issue-1098 branch November 10, 2021 08:30
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.

fix(connector-quorum): runTransaction does not behave the same with different types of credentials
4 participants