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

TrustingPeriodFraction should be a fraction #408

Closed
2 tasks done
shaspitz opened this issue Oct 20, 2022 · 4 comments · Fixed by #593
Closed
2 tasks done

TrustingPeriodFraction should be a fraction #408

shaspitz opened this issue Oct 20, 2022 · 4 comments · Fixed by #593
Assignees
Labels
type: feature-request New feature or request improvement type: refactoring Code refactoring

Comments

@shaspitz
Copy link
Contributor

shaspitz commented Oct 20, 2022

Problem

#393 made this parameter an sdk "param". But it has always existed as an integer, acting as a denominator to calculate trusting periods. This param makes more sense as a fraction, but will require a way to encode a numerator and denominator into json on genesis.

We could try using ibctmtypes.Fraction or two separate params for numerator and denominator

Related to this, the default for the new fraction should be 2/3 per https://github.com/cosmos/ibc-go/blob/main/docs/architecture/adr-026-ibc-client-recovery-mechanisms.md, i.e.,

we recommend that the trusting period for the 07-tendermint client be set to 2/3 of the UnbondingPeriod.

Closing criteria

Make this param a fraction, be able to encode that fraction in json, set new recommended default

TODOs

  • Labels have been added for issue
  • Issue has been added to the ICS project
@shaspitz shaspitz added type: feature-request New feature or request improvement type: refactoring Code refactoring labels Oct 20, 2022
@shaspitz shaspitz changed the title TrustingPeriodFraction parameter is not a fraction, it should be a fraction TrustingPeriodFraction should be a fraction Oct 20, 2022
@mpoke
Copy link
Contributor

mpoke commented Oct 20, 2022

Alternatively, we can use the same approach as for ConsumerRedistributionFrac.

@shaspitz
Copy link
Contributor Author

Alternatively, we can use the same approach as for ConsumerRedistributionFrac.

Ah yes, this seems best 👍

@danwt
Copy link
Contributor

danwt commented Nov 30, 2022

I would suggest a 'scalar' or 'multiplier' in [0,1] or whatever appropriate interval.

@jtremback
Copy link
Contributor

Yea, ConsumerRedistributionFrac is a stringified decimal eg "0.75"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature-request New feature or request improvement type: refactoring Code refactoring
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants