ABCI tighter type constraints

[tomtau]

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

[tac0turtle]

I believe this issue may be relevant in tendermint to change the int64 types to uint64 types in proto files where needed?

[liamsi]

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:
tendermint/tendermint#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.

[tomtau]

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

[tomtau]

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.
On the other hand, I've been abusing this particular part and it doesn't seem to have any effect on Tendermint (RPC happily returns them base64-encoded). I guess that only affects indexing / querying or the ABCI specs documented some no longer valid assumption?

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?

[martinholovsky]

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?

[tomtau]

@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).