cosmos
ICS2: Potential Changes
Surfaced from Informal Systems IBC Audit of cosmos-sdk hash https://github.com/cosmos/cosmos-sdk/commit/0bd46574f431d8281e71cad2f166973bb558f7c3.
-
[ ] The code has several aspects that are not discussed in the specification.
-
[ ] In ICS 002 the Height is more abstract (just a linear order), while the implementation encodes it as a pair of integers in lexicographical order
-
[ ] the use of cmp in the code seems quite intricate. The pseudo code in ICS 007 seems more clear.
-
[ ] ClientState.Validate:
- ConsensusParams.Version never checked for nil but never used
- MaxClockDrift should be less than TrustingPeriod ?
- TrustingPeriod should be much less than UnbondingPeriod? How much?
-
[ ] consensusState.GetTimestamp has overflow risk (cast UnixNano to uint64). Probably not a problem since negative time is pre 1970 but still inconsistent to use uint64 when the data type is really int64.
Regarding proposals and genesis, see my comment here - https://github.com/cosmos/ics/issues/497#issuecomment-732541353.
Regarding upgrades, I am also not convinced this should be in the ICS specs - it is really a hack implemented to work around Tendermint not supporting certain features (but that will probably be fixed soon anyways and we can simplify the code).
Otherwise I agree.
