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.

Sign up for GitHub

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

Change IBCTimeout's Timestamp field to new Uint64 #473

Closed
wants to merge 2 commits into from
Closed

Change IBCTimeout's Timestamp field to new Uint64 #473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
935a63f
Use new Uint64 for IBCTimeout timestamp
chipshort Nov 20, 2023
51d6a71
Fix IBCTimeout tests
chipshort Nov 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion types/ibc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ func (t IBCTimeoutBlock) IsZero() bool {
type IBCTimeout struct {
Block *IBCTimeoutBlock `json:"block"`
// Nanoseconds since UNIX epoch
Timestamp uint64 `json:"timestamp,string,omitempty"`
Timestamp *Uint64 `json:"timestamp,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, not sure if this is the way. But let's see.

IbcTimeout is used in both directions:

  1. Rust -> Go: e.g. IbcMsg::Transfer
  2. Go -> Rust: e.g. IbcPacket

On the Go side, 0 is used to represent an unset Timeout (wasmd, ibc-go). This means wasmd would have to convert 0 -> nil as a special case. Then we have two different nulls here that cannot be differentiated by ibc-go (nil and Uint64(0)), so we either need to make it impossible to every have Uint64(0) or have a step in wasmd that merged the two into 0.

Would it be possible to get Timestamp Uint64 to work instead? Not sure how flexible the combination of custom type and annotations is.

Copy link
Member

Choose a reason for hiding this comment

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

The whole concept of mapping a Go uint64 to a Rust Option is a bet flawed anyways. What you'd actually want is Option<NonNull> to keep a 1:1 mapping.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the mapping is a bit flawed. Timestamp Uint64 would work with timestamp: Timestamp on the rust side. Not very rusty, but at least consistent with the go side.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or if we could make the go-gen emit Timestamp Uint64 with omitempty, then that could also work with timestamp: Option<Timestamp> on the rust side, but not sure how to best approach that.
Ofc this still has the flaw that both 0 and None are ways to represent 0 on the go side.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think this is pretty much what we want. Get back to the current code but just change uint64 -> Uint64.

Then we have for

Go -> Rust:

In Go, only one zero state exists, which is Uint64(0). Does it understand that to be empty and can omit it? If yes, good, then it becomes None in Rust. If no, we get Some(Timestamp::from_nanos(0)) which is not what we want in Rust.

Rust -> Go:

Option<Timestamp> can be null or "0". "0" becomes Uint64(0). But does Go understand Uint64's default? Can it translate null or unset to Uint64(0)?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe we should not do this in general and instead create a IBCTimeout specific solution that deserialized into a helper type first and then turns nil into Uint64(0) for convenient of our Go users.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I thought the same at first, because we probably don't always want null to become 0.
On the other hand, as you already said, that is the default behaviour for Go's uint64, so for Go users it probably makes more sense to always do that conversion.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe not so relevant for Option<Timestamp>, but I can easily come up with ideas for Option<Uint64> where None means something different than Some(0). Let's look for a timeout specific solution, which can either be some sort of custom deserializer or just keep the type we currently have.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we want to make that distinction, then it could still be made with *Uint64 on the Go side: nil then serializes to null (or skipped if omitempty) and null deserializes to nil, while "0" deserializes to &Uint64(0).
But I am also fine with just keep the version we have right now.

Copy link
Member

Choose a reason for hiding this comment

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

I think we want the ability to make the distinction in other places, but not here. So I suggest keeping the current version and close this.

Glad we discussed this though!

}

type IBCAcknowledgement struct {
Expand Down
14 changes: 8 additions & 6 deletions types/ibc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@ import (
)

func TestIbcTimeoutSerialization(t *testing.T) {
ts := Uint64(1578939743_987654321)
// All set
timeout := IBCTimeout{
Block: &IBCTimeoutBlock{
Revision: 17,
Height: 42,
},
Timestamp: 1578939743_987654321,
Timestamp: &ts,
}
bz, err := json.Marshal(timeout)
require.NoError(t, err)
Expand All @@ -24,7 +25,7 @@ func TestIbcTimeoutSerialization(t *testing.T) {
// Null block
timeout = IBCTimeout{
Block: nil,
Timestamp: 1578939743_987654321,
Timestamp: &ts,
}
bz, err = json.Marshal(timeout)
require.NoError(t, err)
Expand All @@ -38,7 +39,7 @@ func TestIbcTimeoutSerialization(t *testing.T) {
Revision: 17,
Height: 42,
},
Timestamp: 0,
Timestamp: nil,
}
bz, err = json.Marshal(timeout)
require.NoError(t, err)
Expand All @@ -47,6 +48,7 @@ func TestIbcTimeoutSerialization(t *testing.T) {

func TestIbcTimeoutDeserialization(t *testing.T) {
var err error
ts := Uint64(1578939743_987654321)

// All set
var timeout1 IBCTimeout
Expand All @@ -57,7 +59,7 @@ func TestIbcTimeoutDeserialization(t *testing.T) {
Revision: 17,
Height: 42,
},
Timestamp: 1578939743_987654321,
Timestamp: &ts,
}, timeout1)

// Null block
Expand All @@ -66,7 +68,7 @@ func TestIbcTimeoutDeserialization(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, IBCTimeout{
Block: nil,
Timestamp: 1578939743_987654321,
Timestamp: &ts,
}, timeout2)

// Null timestamp
Expand All @@ -78,6 +80,6 @@ func TestIbcTimeoutDeserialization(t *testing.T) {
Revision: 17,
Height: 42,
},
Timestamp: 0,
Timestamp: nil,
}, timeout3)
}