Skip to content
This repository has been archived by the owner on Jul 27, 2022. It is now read-only.

Problem (Fix #1089): the upper bound of reward parameters are too restrictive #1090

Merged
merged 1 commit into from
Feb 18, 2020
Merged

Problem (Fix #1089): the upper bound of reward parameters are too restrictive #1090

merged 1 commit into from
Feb 18, 2020

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Feb 18, 2020 •
edited
Loading

Solution:

  • Add more reasoning into the range of parameters.

The reasoning behind the choice:
#1089 (comment)

@codecov
Copy link

codecov bot commented Feb 18, 2020 •
edited
Loading

Codecov Report

Merging #1090 into master will increase coverage by <.01%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master    #1090      +/-   ##
==========================================
+ Coverage   66.24%   66.24%   +<.01%     
==========================================
  Files         145      145              
  Lines       18840    18843       +3     
==========================================
+ Hits        12481    12483       +2     
- Misses       6359     6360       +1
Impacted Files Coverage Δ
chain-abci/tests/validator_changes.rs 100% <100%> (ø) ⬆️
chain-core/src/common/fixed.rs 100% <100%> (ø) ⬆️
chain-core/src/init/params.rs 77.77% <50%> (ø) ⬆️
chain-core/src/common/merkle_tree.rs 98.55% <0%> (-0.25%) ⬇️

tomtau
tomtau reviewed Feb 18, 2020
View reviewed changes
Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

ok to merge, but need to understand more context.
current docs have this upper bound mentioned there: https://crypto-com.github.io/getting-started/staking.html#rewards
my assumption was that most of these bounds were there for "safe" (no panic or strange results) operations with fixed points?

@yihuang
Copy link
Collaborator Author

yihuang commented Feb 18, 2020 •
edited
Loading

ok to merge, but need to understand more context.
current docs have this upper bound mentioned there: https://crypto-com.github.io/getting-started/staking.html#rewards
my assumption was that most of these bounds were there for "safe" (no panic or strange results) operations with fixed points?

Yes, there is no logical constraint to the upper bound of both r0 and tau, I'll check the safety of fixed point computation again, and sent pr to update docs first before we merge this one.

@yihuang yihuang force-pushed the master branch 2 times, most recently from 7c9552e to 3c43935 Compare February 18, 2020 08:23
@yihuang yihuang changed the title Problem (Fix #1089): No need to constraint upper bond of monetary_expansion_tau Problem (Fix #1089): the upper bound of reward parameters are too restrictive Feb 18, 2020
@tomtau
Copy link
Contributor

tomtau commented Feb 18, 2020

bors r+

bors bot added a commit that referenced this pull request Feb 18, 2020
@bors @yihuang
Merge #1090
078bc84 
1090: Problem (Fix #1089): the upper bound of reward parameters are too restrictive r=tomtau a=yihuang

Solution:
  - Add more reasoning into the range of parameters.

The reasoning behind the choice:
#1089 (comment)

Co-authored-by: yihuang <huang@crypto.com>
@bors
Copy link
Contributor

bors bot commented Feb 18, 2020

Build failed

@yihuang
Problem (Fix crypto-com#1089): the upper bound of reward parameters a…
164cd3a 
…re too restrictive

Solution:
- Add more reasoning into the range of parameters.
@tomtau
Copy link
Contributor

tomtau commented Feb 18, 2020

bors retry

bors bot added a commit that referenced this pull request Feb 18, 2020
@bors @yihuang
Merge #1090
8819082 
1090: Problem (Fix #1089): the upper bound of reward parameters are too restrictive r=tomtau a=yihuang

Solution:
  - Add more reasoning into the range of parameters.

The reasoning behind the choice:
#1089 (comment)

Co-authored-by: yihuang <huang@crypto.com>
@bors
Copy link
Contributor

bors bot commented Feb 18, 2020

Build succeeded

@bors bors bot merged commit 164cd3a into crypto-com:master Feb 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tomtau tomtau tomtau left review comments

@lezzokafka lezzokafka lezzokafka approved these changes

@calvinaco calvinaco Awaiting requested review from calvinaco calvinaco is a code owner

@devashishdxt devashishdxt Awaiting requested review from devashishdxt devashishdxt is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yihuang @tomtau @lezzokafka