multi: Version server API. #1047

JoeGruffins · 2021-04-12T07:52:10Z

Add a version to the server that is communicated in the Config response.
Client checks and saves the version. The burden of conforming to an
API version is placed completely on the client.

depends on #1046
closes #530

JoeGruffins · 2021-04-12T07:53:32Z

Will add some more tests if the idea looks ok.

chappjc · 2021-04-12T14:40:11Z

Thanks, @JoeGruffins.

The plan we decided on was to introduce the API version and have it communicated asap, but keeping it at v0 until dcrdex 1.0 or SPV (probably same release). v0 because it's not stable, but still communicated and checked so when a major bump to v1 happens the client sees that they're going to need to update to a client that knows the v1 API.

Of course we may have to bump 0->1 sooner than SPV if there are changes we can't solve with clever backward compatibility, but I think it's best if we keep it v0 to indicate that there are no promises yet. In my mind the necessity with this PR is having the version checking machinery in place well in advance of such major breaking version bumps... which this PR totally does, but I wanted to talk through the planned jump to a stable v1 proto.

chappjc

Looks like a good start. Having clients looking at the version starting with 0.2 is important.

The other more fine grained versioning we'll need is for each asset. This PR is the version for the comms protocol and maybe some other agreed upon things, but I believe we'll need to version each asset for when things with an asset are changed that don't affect the dex API (e.g. kind of addresses, transaction structure, coin ID encoding, contract address, lock times, encoding of txdata payload, etc.)... things that concern a particular asset backend rather than the higher level core packages or comms.

chappjc · 2021-04-12T14:45:10Z

client/core/core.go

+	// serverAPIVers are the DEX server API versions this client is capable
+	// of communicating with.
+	serverAPIVers = []int{serverdex.InitialAPIVersion}


This makes sense and I'm OK with using a server/dex import for this now, but I suspect we're going to want to break the tight coupling of client -> server packages at some point.

Probably we're just discovering things (like server/account) that would be better in the "common" decred.org/dcrdex/dex packages at some point.

chappjc · 2021-04-12T14:48:24Z

server/dex/dex.go

+	InitialAPIVersion = iota
+
+	// APIVersion is the current API version.
+	APIVersion = InitialAPIVersion


Good to start at v0 as you have now.

JoeGruffins · 2021-04-13T03:42:32Z

The other more fine grained versioning we'll need is for each asset.

Maybe something similar to this but a map of asset id to api versions in the config response?

JoeGruffins · 2021-04-13T06:05:53Z

client/core/core.go

-	err = dc.refreshServerConfig() // handleReconnect must too
+	err = dc.refreshServerConfig(serverAPIVers) // handleReconnect must too
 	if err != nil {
+		if errors.Is(err, outdatedClientErr) {
+			sendOutdatedClientNotification(c, dc)
+		}
 		return dc, err
 	}


Might be less awkward if just do the api check in a separate function, although it makes sense to me that the API version is the first thing checked in the response and a quick fail is best.

chappjc · 2021-04-13T14:31:52Z

client/core/bookie.go

+	if apiVer != cfgAPIVer {
+		// If not initiation, do not allow api changes
+		// between connections.
+		if apiVer != -1 {
+			return fmt.Errorf("server API version unexpectedly changed from %v to %v",


I've been thinking about this, and this seems to be inevitable, so I don't think we have a choice but to go along with the new version if it's recognized. A server update that changes API version will happen and I don't think we should always torpedo connected clients if they recognize the new api version.

I agree. I just think it opens some angles possibly, that are hard to notice. Will remove, and we'll just be careful when updating.

K. A warning would probably be good though.

chappjc · 2021-04-13T14:32:26Z

client/core/bookie.go

-func (dc *dexConnection) refreshServerConfig() error {
+// refreshServerConfig fetches and replaces server configuration data. It also
+// initially checks that a server's API version is one of apiVers.
+func (dc *dexConnection) refreshServerConfig(apiVers []int) error {


Why not use the package-level var serverAPIVers instead of an arg?

For testing...
If we use the package level variable, it makes testing in parallel a bit difficult, although doable. Will see what it looks like, I guess will need to add a lock to the test harness.

Ah, I see. I don't think there will be any concurrency issues though. I think you can just change the slice and call refreshServerConfig for each test case without any risk of concurrent reads. But if for some reason something is a problem with that, I don't think you even need a running tCore. That is, you could manually put together a dexConnection either with testDexConnection or manually with just what's needed to test this method.

Just using the top numbers of a uint16 which are probably not going to ever be actual versions.

chappjc · 2021-04-13T14:35:50Z

The other more fine grained versioning we'll need is for each asset.

Maybe something similar to this but a map of asset id to api versions in the config response?

Yeah, that would be appropriate. I think we could put a version in the dex/msgjson.Asset struct.

Add a version to the server that is communicated in the Config response. Client checks and saves the version. The burden of conforming to an API version is placed completely on the client.

chappjc · 2021-04-16T14:41:26Z

The other more fine grained versioning we'll need is for each asset.

Maybe something similar to this but a map of asset id to api versions in the config response?

Yeah, that would be appropriate. I think we could put a version in the dex/msgjson.Asset struct.

@buck54321 any objections to this?

In short these versions would be for things with an asset are changed that don't affect the dex API (e.g. kind of addresses, transaction structure, coin ID encoding, contract address, lock times, encoding of txdata payload, etc.)... things that concern a particular asset backend rather than the higher level core packages or comms.

Checking the asset version at order placement time for both base and quote seems right to me for that.

buck54321 · 2021-04-16T16:06:22Z

@buck54321 any objections to this?

In short these versions would be for things with an asset are changed that don't affect the dex API (e.g. kind of addresses, transaction structure, coin ID encoding, contract address, lock times, encoding of txdata payload, etc.)... things that concern a particular asset backend rather than the higher level core packages or comms.

Checking the asset version at order placement time for both base and quote seems right to me for that.

Sounds about right, though it's not readily apparent to me how an upgrade would work. Also not quite clear what to do at the UI level when we know that a server's asset version is incompatible with the client backend. I guess these details will get worked out in time. I think the important bit is to convey the info now so we're not trying to coordinate this in a patch release or something dumb like that.

chappjc · 2021-04-16T16:16:06Z

Yeah, I think at a minimum it's a seatbelt that says "to trade with this asset at X you gotta update to version Y", to prevent making swaps or whatever that will be rejected (or worse with an old version that possibly had a security issue).

chappjc · 2021-04-16T20:38:41Z

I'm doing the asset versioning @JoeGruffins... shoulda mentioned.

chappjc reviewed Apr 12, 2021

View reviewed changes

chappjc added this to the 0.2 milestone Apr 12, 2021

JoeGruffins force-pushed the apiconfig branch from c6d642f to 8d6fdad Compare April 13, 2021 06:00

JoeGruffins commented Apr 13, 2021

View reviewed changes

JoeGruffins force-pushed the apiconfig branch 2 times, most recently from 687e723 to d1a8b3c Compare April 13, 2021 06:15

chappjc reviewed Apr 13, 2021

View reviewed changes

JoeGruffins force-pushed the apiconfig branch from cef7888 to b1ef298 Compare April 15, 2021 05:56

JoeGruffins added 5 commits April 15, 2021 14:56

spec: Add apiver to server config response.

0952770

Start at v0.

bd65cde

multi: Version server API.

88f7929

Add a version to the server that is communicated in the Config response. Client checks and saves the version. The burden of conforming to an API version is placed completely on the client.

Rename InitialAPI to PreAPI.

a7ca51b

Allow changing API.

b1ef298

chappjc merged commit c5eeb28 into decred:master Apr 16, 2021

chappjc mentioned this pull request Apr 16, 2021

multi: Enable client/server API version checking. #530

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: Version server API. #1047

multi: Version server API. #1047

JoeGruffins commented Apr 12, 2021

JoeGruffins commented Apr 12, 2021

chappjc commented Apr 12, 2021 •

edited

Loading

chappjc left a comment •

edited

Loading

chappjc Apr 12, 2021

chappjc Apr 12, 2021

JoeGruffins commented Apr 13, 2021

JoeGruffins Apr 13, 2021

chappjc Apr 13, 2021 •

edited

Loading

JoeGruffins Apr 13, 2021

chappjc Apr 14, 2021

JoeGruffins Apr 15, 2021

chappjc Apr 13, 2021

JoeGruffins Apr 13, 2021

chappjc Apr 14, 2021 •

edited

Loading

JoeGruffins Apr 15, 2021

chappjc commented Apr 13, 2021

chappjc commented Apr 16, 2021

buck54321 commented Apr 16, 2021

chappjc commented Apr 16, 2021

chappjc commented Apr 16, 2021

multi: Version server API. #1047

multi: Version server API. #1047

Conversation

JoeGruffins commented Apr 12, 2021

JoeGruffins commented Apr 12, 2021

chappjc commented Apr 12, 2021 • edited Loading

chappjc left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins commented Apr 13, 2021

Choose a reason for hiding this comment

chappjc Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Apr 13, 2021

chappjc commented Apr 16, 2021

buck54321 commented Apr 16, 2021

chappjc commented Apr 16, 2021

chappjc commented Apr 16, 2021

chappjc commented Apr 12, 2021 •

edited

Loading

chappjc left a comment •

edited

Loading

chappjc Apr 13, 2021 •

edited

Loading

chappjc Apr 14, 2021 •

edited

Loading