client/asset: clarify eth and token versioning #2094
Conversation
buck54321
force-pushed
the
token-supported-vers
branch
from
8d7cd6c to
5d2dd39
Compare
client/asset/eth/eth.go
Outdated
| }, | ||
| } | ||
|
|
||
| SupportedTokenVersions = []uint32{0} // Server versions that we can handle. |
There was a problem hiding this comment.
Won't each token have it's own list of versions?
There was a problem hiding this comment.
Server protocol version != contract version. We've been sloppy with this concept. The client knows how to handle a range of server versions. Those server versions certainly correspond to a range of contract versions, but not necessarily one-to-one.
There was a problem hiding this comment.
Ah right. Say there's a new v1 swap contract version for USDC, server protocol version 1 (up from 0) could support contract versions 0 and 1. Server advertises whichever single protocol version it supports, while client can manage multiple. Right, ok. I think this works out but is definitely a bit hard to keep straight.
There was a problem hiding this comment.
We've never really pinned down the versioning scheme. Lemme come up with a comprehensive plan and document it. This PR may not be the best way, now that I think about it.
client/asset/eth/eth.go
Outdated
| Type: "token", | ||
| Description: desc, | ||
| }, nets...) | ||
| }, SupportedTokenVersions, nets...) |
There was a problem hiding this comment.
I may not be seeing the problem clearly, but above we do token, found := dexeth.Tokens[tokenID], and it looks like here in registerToken is the right place to get the supported versions out of NetTokens.
Also doesn't this mean that each net will have a different list of versions?
There was a problem hiding this comment.
Same answer here, really. From NetTokens, we would get the versioned contracts that have been deployed for a specific token. What we're interested in is which server protocol versions we can communicate with.
|
Keeping open for discussion. I think we're a little uncertain at present if the parent asset version is good for it's tokens or we need multiple sets. |
buck54321
force-pushed
the
token-supported-vers
branch
from
5d2dd39 to
ce6cce5
Compare
buck54321
changed the title
martonp
left a comment
martonp
left a comment
There was a problem hiding this comment.
I think it makes sense share the version for ETH and all tokens. I don't think a server would want to not support the latest of all contracts.
3 participants
This is needed for #2054.The tokens supported versions were not exposed because tokens without wallets don't exposeWalletInfo(noDriver).Changing tacts on this a bit. Instead, we'll just have the tokens share the parent asset's versioning, and the server will maintain with a single protocol version.
Just a reminder that server protocol version != contract version. A server's version does map to a contract version, but it's not one-to-one. i.e. you cannot add a new contract version without incrementing the server version, but a server version could be incremented for other reasons too. I've fixed versioning ambiguity in a couple places in this effort, but there is much more work done in #2038. I could potentially pull that out into a separate PR.