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

Governance Proposal View Decoding should indicate scientific notation of values #5343

Closed
nambrot opened this issue Oct 15, 2020 · 3 comments · Fixed by #5584
Closed

Governance Proposal View Decoding should indicate scientific notation of values #5343

nambrot opened this issue Oct 15, 2020 · 3 comments · Fixed by #5584
Assignees

Comments

@nambrot
Copy link
Contributor

nambrot commented Oct 15, 2020

Expected Behavior

The values should should display scientific notation when above 1 Million

Current Behavior

The values are displayed in full

@tromer
Copy link
Contributor

tromer commented Oct 21, 2020

On related note (which may merit a dedicated issue):

Some parameters have magical correction factors between the natural representation and the one used by contracts (and thus showing in proposals). This makes it error-prone to verify proposals.

For example, consider the way fixed-point fractions are represented by FixidityLib:

$ celocli governance:view --proposalID 1
...
  1: 
    contract: EpochRewards
    function: setCarbonOffsettingFund
...
    params: 
      partner: 0x0ba9f5B3CdD349aB65a8DacDA9A38Bc525C2e6D6
      value: 1000000000000000000000
...

To understand what reward fraction this really sets, one would have to open up the code of FixidityLib and figure out how it represents fixed-point fractions as integers.

Similarly with the reserve fraction of CGP 0007 and CGP 0008. The CGP mentions the correction factor (Data: 1000000000000000000000 (1/1000 * 10^24 = 1e21)), so you can sort of know what to expect... But imagine if we had a proposal that claims to set some number to 17 and indeed puts 17 in the actual parameter, yet (unbeknownst to the reviewer) the contract uses FixidityLib for that parameter so 17 actually means 17e-24.

Same problem with the 10^-18 factor for CELO amounts as in CGP 0006. Now you may say, people are already used to dividing by 10^18 whenever they see a huge number representing CELO amounts. But that's not good enough, and in fact is dangerous: if the number does look natural, people may forget that it should be divided by 10^18 anyway. And also, conversely: if for whatever reason some contract accepts CELO amounts as integers instead of fractions, and a proposal sends it 10^19 in a parameter, reviewers will think that means 10 CELO.

I'm not sure how to resolve all of these, and they also affect the usability of celocli in many other use cases.

@nambrot
Copy link
Contributor Author

nambrot commented Oct 21, 2020

That is absolutely a great point! I'm not sure how easy it will be to automatically parse that a value is indeed a fixidity value, but whoever works on this should consider looking into it!

@yorhodes yorhodes self-assigned this Oct 21, 2020
@yorhodes
Copy link
Contributor

related ethers-io/ethers.js#228

@nambrot nambrot removed their assignment Oct 23, 2020
@mergify mergify bot closed this as completed in #5584 Nov 5, 2020
mergify bot pushed a commit that referenced this issue Nov 5, 2020
…and CLI (#5584)

### Description

- Modifies cli utility `printValueMapRecursive` to display scientific notation when significant digits > 3 up to 3 decimal points
- Makes `network:parameters` use `kit.getHumanReadableNetworkConfig` by default and `kit.getNetworkConfig` with `--raw` flag provided

### Other changes

- Adds `blocksToDurationString`, `secondsToDurationString`, `unixSecondsTimestampToDateString` utilities to contractkit's `BaseWrapper` for use in `getHumanReadableConfig` of various wrappers

### Tested

`celocli network:parameters --node https://baklava-forno.celo-testnet.org`
![Screen Shot 2020-10-26 at 1 45 04 PM](https://user-images.githubusercontent.com/3020995/97226609-75dc4a00-1791-11eb-839a-c61ced912df7.png)

### Related issues

- Fixes #5343

### Backwards compatibility

Machine consumers of `network:parameters` must now pass `--raw`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants