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

Rename TimestampNanos to Timestamp and improve IbcTimeout docs #902

Closed
wants to merge 1 commit into from

Conversation

webmaster128
Copy link
Member

Simply the case name by avoiding type information in the variable name

@webmaster128 webmaster128 force-pushed the timeout-renamings-and-docs branch from 9ac8ce2 to ad3e86f Compare April 27, 2021 13:37
@ethanfrey
Copy link
Member

I changed the name from Timeout to TimeoutNanos as I twice put seconds in there (eg. env.block.time + 1000), which only failed when running it in relayer tests (a few levels up).

This is the only nanosecond time we have in cosmwasm (the block time is broken into unix seconds, plus the nanoseconds fraction). We need to make this clear. If you don't like this name, I have two other proposals.

  1. Split it into Seconds/Nanoseconds and force the Go side to recreate. (like env.block.time, env.block.nanos)
  2. Use a custom type type struct Nanos(u64) with a few helpers and use that here. It is JSON-compatible, but forces the Rust side to realize they cannot just pass in seconds.

(Note we accept one field with timeout in IbcPacket, which should probably be updated with whatever approach we choose)

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't just rename. I proposed two other approaches.

I feel renaming to timestamp: Nanos with a different u64 new-type will keep a clean JSON interface that you like and keep contract developers from making simple mistakes that are hard to debug (and only show up in real relayers)

@webmaster128
Copy link
Member Author

I see, makes sense. I was also thinging about

  1. a simple serde rename that allows us to differentiate between external JSON API for Go land and contract developer types.

I'd avoid 1. as there is already enough complexity in the type and it is nice that we can use u64 nanos in IBC, which serves us well for 600 years as long as no float parser is reading that JSON. 2^53 nanoseconds is less than 15 weeks.

Version 2. seems solid to me. There the unit goes where it belongs: from the variable to the value.

I'll think about it a little more.

@ethanfrey
Copy link
Member

  1. a simple serde rename that allows us to differentiate between external JSON API for Go land and contract developer types.

Sure, TimeoutNanos for rust, timeout for JSON, Timeout for Go. It also works.

We do have a helper to take care of most usage, so I think just the serde rename may be plenty.

impl From<Timestamp> for IbcTimeout {
    fn from(time: Timestamp) -> IbcTimeout {
        IbcTimeout::TimestampNanos(time.seconds * 1_000_000_000 + time.nanos)
    }
}

@webmaster128
Copy link
Member Author

as long as no float parser is reading that JSON. 2^53 nanoseconds is less than 15 weeks.

Oh, I just realize this makes the timestamp IbcTimeout pretty much unusable for messages and queries, at least when working with float clients like JavaScript or jq. This is already used in the ibc-reflect-send API:

pub enum ExecuteMsg {
    /// Changes the admin
    UpdateAdmin {
        admin: String,
    },
    SendMsgs {
        channel_id: String,
        // Note: we don't handle custom messages on remote chains
        msgs: Vec<CosmosMsg<Empty>>, // <---- IbcTimeout used here
    },

@ethanfrey
Copy link
Member

This is already used in the ibc-reflect-send API:

I was going to say it was just internal, but yeah, CosmosMsg exposes this (also in normal reflect). If we think we will eventually want this in CosmJS (and I guess we will), we will probably need a stringified version to make JS happy and just suck it up in Go and Rust? Better ideas welcome.

@webmaster128
Copy link
Member Author

webmaster128 commented Apr 27, 2021

We can solve both by having struct Nanos(Uint128) which we can decode nicely unti uint64 by using this decorator: https://stackoverflow.com/a/21152548/2013738

@webmaster128 webmaster128 mentioned this pull request Apr 28, 2021
@webmaster128 webmaster128 deleted the timeout-renamings-and-docs branch July 21, 2021 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants