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

[L02] Inconsistent quorum calculation #2796

Merged
merged 14 commits into from Feb 21, 2020

Conversation

mrsmkl
Copy link
Contributor

@mrsmkl mrsmkl commented Feb 17, 2020

Description

Tested

Other changes

Related issues

Backwards compatibility

@codecov
Copy link

codecov bot commented Feb 17, 2020

Codecov Report

Merging #2796 into master will decrease coverage by <.01%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2796      +/-   ##
==========================================
- Coverage   73.58%   73.58%   -0.01%     
==========================================
  Files         567      567              
  Lines       14160    14162       +2     
  Branches     1464     1703     +239     
==========================================
+ Hits        10420    10421       +1     
- Misses       3455     3458       +3     
+ Partials      285      283       -2
Flag Coverage Δ
#mobile 74.12% <100%> (ø) ⬆️
#web 72.85% <0%> (-0.02%) ⬇️
Impacted Files Coverage Δ
packages/web/src/dev/Engage.tsx 94.44% <ø> (ø) ⬆️
packages/web/src/dev/ValidatorsListApp.tsx 0% <ø> (ø) ⬆️
packages/web/src/shared/Button.3.tsx 89.52% <ø> (ø) ⬆️
packages/web/src/dev/ValidatorsList.tsx 0% <0%> (ø) ⬆️
packages/mobile/src/identity/contactMapping.ts 89.34% <100%> (+0.08%) ⬆️
packages/web/src/utils/utils.ts 71.69% <0%> (ø) ⬆️
packages/web/src/utils/abortableFetch.ts 71.42% <0%> (ø) ⬆️

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 d43ffbe...3a5821b. Read the comment docs.

@@ -83,7 +83,8 @@ const INTRINSIC_TX_GAS_COST = 21000
// Additional intrinsic gas for a transaction with fee currency specified
const ADDITIONAL_INTRINSIC_TX_GAS_COST = 50000

const stableTokenTransferGasCost = 20653
// const stableTokenTransferGasCost = 20653
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sometimes changes, haven't found the reason yet...

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this. We need to make sure we know why gas costs change.

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 was because a public method was added to UsingPrecompiles, so the routing code changed.

packages/protocol/contracts/common/UsingPrecompiles.sol Outdated Show resolved Hide resolved
@asaj asaj assigned mrsmkl and unassigned asaj Feb 18, 2020
packages/protocol/contracts/common/UsingPrecompiles.sol Outdated Show resolved Hide resolved
@@ -83,7 +83,8 @@ const INTRINSIC_TX_GAS_COST = 21000
// Additional intrinsic gas for a transaction with fee currency specified
const ADDITIONAL_INTRINSIC_TX_GAS_COST = 50000

const stableTokenTransferGasCost = 20653
// const stableTokenTransferGasCost = 20653
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this. We need to make sure we know why gas costs change.

@nategraf nategraf removed their assignment Feb 19, 2020
@mrsmkl mrsmkl assigned asaj and nategraf and unassigned mrsmkl Feb 20, 2020
@asaj asaj removed their assignment Feb 20, 2020
@mrsmkl mrsmkl added the automerge Have PR merge automatically when checks pass label Feb 21, 2020
@celo-ci-bot-user celo-ci-bot-user merged commit 438278e into master Feb 21, 2020
@celo-ci-bot-user celo-ci-bot-user deleted the mrsmkl/min-quorum-size-l02 branch February 21, 2020 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-fix automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants