Open tomtau opened 5 years ago
I believe this issue may be relevant in tendermint to change the int64 types to uint64 types in proto files where needed?
I agree with you @tomtau. Also, @marbar3778 is right that the proto files (and hence the generated code) should match the tendermint repo. Actually discussing signed vs unsinged integers has a long tradition: https://github.com/tendermint/tendermint/issues/2684 https://blog.cosmos.network/choosing-a-type-for-blockchain-height-beware-of-unsigned-integers-714804dddf1d
IIRC, we wanted to switch to unsigned where values could never go negative but focused on other things.
Ad that block height discussion: Rust does under/overflow assertion checks in debug releases by default; one could enable them in production builds in a Cargo profile configuration. Anyway, I guess except for "logic" (https://github.com/tendermint/rust-abci/issues/49), there could be some value sanity checks/assertions as well
Besides block height (and perhaps time), there are a few other examples where the protobuf type doesn't indicate to what the specs say -- for example, for Tags: https://tendermint.com/docs/spec/abci/abci.html#tags it mentions keys/values must be UTF-8 strings, but they are Vec
Another thing is "Query": https://tendermint.com/docs/spec/abci/abci.html#query
Apps MUST interpret '/store' as a query by key on the underlying store.
Currently, the trait doesn't force implementors to do so -- on the other hand, I didn't observe any consequences if one doesn't do so?
Ad that block height discussion: Rust does under/overflow assertion checks in debug releases by default; one could enable them in production builds in a Cargo profile configuration. Anyway, I guess except for "logic" (#49), there could be some value sanity checks/assertions as well
Any reason not having
[profile.release]
overflow-checks = true
in Cargo.toml?
@martinholovsky https://doc.rust-lang.org/cargo/reference/manifest.html#the-profile-sections
Any manifest may declare a profile, but only the top level package’s profiles are actually read. All dependencies’ profiles will be overridden.
rust-abci is a library, i.e. won't be top-level. My guess (from that description) is that regardless what's declared here, profiles of application crates override it (so it should be declared in Cargo.toml of applications rather than here).
Some of the message types generated by protobuf have a bit imprecise types -- for example, the block height is returned as
i64
even though it should never be negative