Currency primary key #910
Comments
kilrau
changed the title
ghost
mentioned this issue
|
Related: #893, let's discuss it after milestone 15 |
|
If you are convinced by our code, that swapping the wrong token address can never happen, I suggest to move this to post-1.0.0 @erkarl |
|
As per call @sangaman , define details today since these are breaking changes which we want to have in |
|
Mistakenly had this in |
sangaman
added a commit
that referenced
this issue
Jun 24, 2019
At a high level, this adds a concept of a token identifier for each currency supported by xud that is consistent across the entire network. "BTC" can mean mainnet tokens or testnet tokens (or simnet, etc...) and can even be misconfigured to point to the LTC lightning network. Meanwhile, Raiden ERC20 tokens refer to a particular token contract, and there's no guarantee that peers won't be using different (and therefore incompatible) contracts for the same currency such as WETH. Here, we use an identifier to ensure peers are referring to the same token as we are. For currencies that use lnd, the token identifier is determined dynamically using the `chain` and `network` returned by lnd on a `GetInfo` call. For example, BTC mainnet would be "bitcoin-mainnet" and LTC testnet would be "litecoin-testnet". For currencies that use Raiden, we use the token contract address as the identifier. These token identifiers are shared with peers during the handshake and on any subsequent node state updates. The p2p messaging format is changed to accommodate this. This also drops static node values like the `nodePubKey` and `version` from the node state p2p message, as these values can not change mid-session for a peer. A future improvement may be to detect and support discrepancies in the currency symbol used by a peer for the same token. For example, a peer that advertises "XBT" with a token identifier of "bitcoin-mainnet" should be compatible with our "BTC" that also is identified as "bitcoin-mainnet". This is not included in this commit as it would require further refactoring and complexity. Another follow-up may be creating a test suite for the `Peer` class or refactoring it to move logic elsewhere (along with tests for that logic). There is some logic around handling node state updates where information regarding the peer's supported currencies may change that is currently not tested. Closes #910. BREAKING CHANGE: Changed p2p messaging structure for `SessionInit` and `NodeStateUpdate` packets.
sangaman
added a commit
that referenced
this issue
Jun 24, 2019
At a high level, this adds a concept of a token identifier for each currency supported by xud that is consistent across the entire network. "BTC" can mean mainnet tokens or testnet tokens (or simnet, etc...) and can even be misconfigured to point to the LTC lightning network. Meanwhile, Raiden ERC20 tokens refer to a particular token contract, and there's no guarantee that peers won't be using different (and therefore incompatible) contracts for the same currency such as WETH. Here, we use an identifier to ensure peers are referring to the same token as we are. For currencies that use lnd, the token identifier is determined dynamically using the `chain` and `network` returned by lnd on a `GetInfo` call. For example, BTC mainnet would be "bitcoin-mainnet" and LTC testnet would be "litecoin-testnet". For currencies that use Raiden, we use the token contract address as the identifier. These token identifiers are shared with peers during the handshake and on any subsequent node state updates. The p2p messaging format is changed to accommodate this. This also drops static node values like the `nodePubKey` and `version` from the node state p2p message, as these values can not change mid-session for a peer. A future improvement may be to detect and support discrepancies in the currency symbol used by a peer for the same token. For example, a peer that advertises "XBT" with a token identifier of "bitcoin-mainnet" should be compatible with our "BTC" that also is identified as "bitcoin-mainnet". This is not included in this commit as it would require further refactoring and complexity. Another follow-up may be creating a test suite for the `Peer` class or refactoring it to move logic elsewhere (along with tests for that logic). There is some logic around handling node state updates where information regarding the peer's supported currencies may change that is currently not tested. Closes #910. BREAKING CHANGE: Changed p2p messaging structure for `SessionInit` and `NodeStateUpdate` packets.
sangaman
added a commit
that referenced
this issue
Jun 24, 2019
At a high level, this adds a concept of a token identifier for each currency supported by xud that is consistent across the entire network. "BTC" can mean mainnet tokens or testnet tokens (or simnet, etc...) and can even be misconfigured to point to the LTC lightning network. Meanwhile, Raiden ERC20 tokens refer to a particular token contract, and there's no guarantee that peers won't be using different (and therefore incompatible) contracts for the same currency such as WETH. Here, we use an identifier to ensure peers are referring to the same token as we are. For currencies that use lnd, the token identifier is determined dynamically using the `chain` and `network` returned by lnd on a `GetInfo` call. For example, BTC mainnet would be "bitcoin-mainnet" and LTC testnet would be "litecoin-testnet". For currencies that use Raiden, we use the token contract address as the identifier. These token identifiers are shared with peers during the handshake and on any subsequent node state updates. The p2p messaging format is changed to accommodate this. This also drops static node values like the `nodePubKey` and `version` from the node state p2p message, as these values can not change mid-session for a peer. A future improvement may be to detect and support discrepancies in the currency symbol used by a peer for the same token. For example, a peer that advertises "XBT" with a token identifier of "bitcoin-mainnet" should be compatible with our "BTC" that also is identified as "bitcoin-mainnet". This is not included in this commit as it would require further refactoring and complexity. Another follow-up may be creating a test suite for the `Peer` class or refactoring it to move logic elsewhere (along with tests for that logic). There is some logic around handling node state updates where information regarding the peer's supported currencies may change that is currently not tested. Closes #910. BREAKING CHANGE: Changed p2p messaging structure for `SessionInit` and `NodeStateUpdate` packets.
sangaman
added a commit
that referenced
this issue
Jun 25, 2019
At a high level, this adds a concept of a token identifier for each currency supported by xud that is consistent across the entire network. "BTC" can mean mainnet tokens or testnet tokens (or simnet, etc...) and can even be misconfigured to point to the LTC lightning network. Meanwhile, Raiden ERC20 tokens refer to a particular token contract, and there's no guarantee that peers won't be using different (and therefore incompatible) contracts for the same currency such as WETH. Here, we use an identifier to ensure peers are referring to the same token as we are. For currencies that use lnd, the token identifier is determined dynamically using the `chain` and `network` returned by lnd on a `GetInfo` call. For example, BTC mainnet would be "bitcoin-mainnet" and LTC testnet would be "litecoin-testnet". For currencies that use Raiden, we use the token contract address as the identifier. These token identifiers are shared with peers during the handshake and on any subsequent node state updates. The p2p messaging format is changed to accommodate this. This also drops static node values like the `nodePubKey` and `version` from the node state p2p message, as these values can not change mid-session for a peer. A future improvement may be to detect and support discrepancies in the currency symbol used by a peer for the same token. For example, a peer that advertises "XBT" with a token identifier of "bitcoin-mainnet" should be compatible with our "BTC" that also is identified as "bitcoin-mainnet". This is not included in this commit as it would require further refactoring and complexity. Another follow-up may be creating a test suite for the `Peer` class or refactoring it to move logic elsewhere (along with tests for that logic). There is some logic around handling node state updates where information regarding the peer's supported currencies may change that is currently not tested. Closes #910. BREAKING CHANGE: Changed p2p messaging structure for `SessionInit` and `NodeStateUpdate` packets.
sangaman
added a commit
that referenced
this issue
Jun 28, 2019
At a high level, this adds a concept of a token identifier for each currency supported by xud that is consistent across the entire network. "BTC" can mean mainnet tokens or testnet tokens (or simnet, etc...) and can even be misconfigured to point to the LTC lightning network. Meanwhile, Raiden ERC20 tokens refer to a particular token contract, and there's no guarantee that peers won't be using different (and therefore incompatible) contracts for the same currency such as WETH. Here, we use an identifier to ensure peers are referring to the same token as we are. For currencies that use lnd, the token identifier is determined dynamically using the `chain` and `network` returned by lnd on a `GetInfo` call. For example, BTC mainnet would be "bitcoin-mainnet" and LTC testnet would be "litecoin-testnet". For currencies that use Raiden, we use the token contract address as the identifier. These token identifiers are shared with peers during the handshake and on any subsequent node state updates. The p2p messaging format is changed to accommodate this. This also drops static node values like the `nodePubKey` and `version` from the node state p2p message, as these values can not change mid-session for a peer. A future improvement may be to detect and support discrepancies in the currency symbol used by a peer for the same token. For example, a peer that advertises "XBT" with a token identifier of "bitcoin-mainnet" should be compatible with our "BTC" that also is identified as "bitcoin-mainnet". This is not included in this commit as it would require further refactoring and complexity. Another follow-up may be creating a test suite for the `Peer` class or refactoring it to move logic elsewhere (along with tests for that logic). There is some logic around handling node state updates where information regarding the peer's supported currencies may change that is currently not tested. Closes #910. BREAKING CHANGE: Changed p2p messaging structure for `SessionInit` and `NodeStateUpdate` packets.
sangaman
added a commit
that referenced
this issue
Jun 28, 2019
At a high level, this adds a concept of a token identifier for each currency supported by xud that is consistent across the entire network. "BTC" can mean mainnet tokens or testnet tokens (or simnet, etc...) and can even be misconfigured to point to the LTC lightning network. Meanwhile, Raiden ERC20 tokens refer to a particular token contract, and there's no guarantee that peers won't be using different (and therefore incompatible) contracts for the same currency such as WETH. Here, we use an identifier to ensure peers are referring to the same token as we are. For currencies that use lnd, the token identifier is determined dynamically using the `chain` and `network` returned by lnd on a `GetInfo` call. For example, BTC mainnet would be "bitcoin-mainnet" and LTC testnet would be "litecoin-testnet". For currencies that use Raiden, we use the token contract address as the identifier. These token identifiers are shared with peers during the handshake and on any subsequent node state updates. The p2p messaging format is changed to accommodate this. This also drops static node values like the `nodePubKey` and `version` from the node state p2p message, as these values can not change mid-session for a peer. A future improvement may be to detect and support discrepancies in the currency symbol used by a peer for the same token. For example, a peer that advertises "XBT" with a token identifier of "bitcoin-mainnet" should be compatible with our "BTC" that also is identified as "bitcoin-mainnet". This is not included in this commit as it would require further refactoring and complexity. Another follow-up may be creating a test suite for the `Peer` class or refactoring it to move logic elsewhere (along with tests for that logic). There is some logic around handling node state updates where information regarding the peer's supported currencies may change that is currently not tested. Closes #910. BREAKING CHANGE: Changed p2p messaging structure for `SessionInit` and `NodeStateUpdate` packets.
sangaman
added a commit
that referenced
this issue
Jun 28, 2019
At a high level, this adds a concept of a token identifier for each currency supported by xud that is consistent across the entire network. "BTC" can mean mainnet tokens or testnet tokens (or simnet, etc...) and can even be misconfigured to point to the LTC lightning network. Meanwhile, Raiden ERC20 tokens refer to a particular token contract, and there's no guarantee that peers won't be using different (and therefore incompatible) contracts for the same currency such as WETH. Here, we use an identifier to ensure peers are referring to the same token as we are. For currencies that use lnd, the token identifier is determined dynamically using the `chain` and `network` returned by lnd on a `GetInfo` call. For example, BTC mainnet would be "bitcoin-mainnet" and LTC testnet would be "litecoin-testnet". For currencies that use Raiden, we use the token contract address as the identifier. These token identifiers are shared with peers during the handshake and on any subsequent node state updates. The p2p messaging format is changed to accommodate this. This also drops static node values like the `nodePubKey` and `version` from the node state p2p message, as these values can not change mid-session for a peer. A future improvement may be to detect and support discrepancies in the currency symbol used by a peer for the same token. For example, a peer that advertises "XBT" with a token identifier of "bitcoin-mainnet" should be compatible with our "BTC" that also is identified as "bitcoin-mainnet". This is not included in this commit as it would require further refactoring and complexity. Another follow-up may be creating a test suite for the `Peer` class or refactoring it to move logic elsewhere (along with tests for that logic). There is some logic around handling node state updates where information regarding the peer's supported currencies may change that is currently not tested. Closes #910. BREAKING CHANGE: Changed p2p messaging structure for `SessionInit` and `NodeStateUpdate` packets.
sangaman
added a commit
that referenced
this issue
Jun 28, 2019
At a high level, this adds a concept of a token identifier for each currency supported by xud that is consistent across the entire network. "BTC" can mean mainnet tokens or testnet tokens (or simnet, etc...) and can even be misconfigured to point to the LTC lightning network. Meanwhile, Raiden ERC20 tokens refer to a particular token contract, and there's no guarantee that peers won't be using different (and therefore incompatible) contracts for the same currency such as WETH. Here, we use an identifier to ensure peers are referring to the same token as we are. For currencies that use lnd, the token identifier is determined dynamically using the `chain` and `network` returned by lnd on a `GetInfo` call. For example, BTC mainnet would be "bitcoin-mainnet" and LTC testnet would be "litecoin-testnet". For currencies that use Raiden, we use the token contract address as the identifier. These token identifiers are shared with peers during the handshake and on any subsequent node state updates. The p2p messaging format is changed to accommodate this. This also drops static node values like the `nodePubKey` and `version` from the node state p2p message, as these values can not change mid-session for a peer. A future improvement may be to detect and support discrepancies in the currency symbol used by a peer for the same token. For example, a peer that advertises "XBT" with a token identifier of "bitcoin-mainnet" should be compatible with our "BTC" that also is identified as "bitcoin-mainnet". This is not included in this commit as it would require further refactoring and complexity. Another follow-up may be creating a test suite for the `Peer` class or refactoring it to move logic elsewhere (along with tests for that logic). There is some logic around handling node state updates where information regarding the peer's supported currencies may change that is currently not tested. Closes #910. BREAKING CHANGE: Changed p2p messaging structure for `SessionInit` and `NodeStateUpdate` packets.
sangaman
added a commit
that referenced
this issue
Jul 1, 2019
At a high level, this adds a concept of a token identifier for each currency supported by xud that is consistent across the entire network. "BTC" can mean mainnet tokens or testnet tokens (or simnet, etc...) and can even be misconfigured to point to the LTC lightning network. Meanwhile, Raiden ERC20 tokens refer to a particular token contract, and there's no guarantee that peers won't be using different (and therefore incompatible) contracts for the same currency such as WETH. Here, we use an identifier to ensure peers are referring to the same token as we are. For currencies that use lnd, the token identifier is determined dynamically using the `chain` and `network` returned by lnd on a `GetInfo` call. For example, BTC mainnet would be "bitcoin-mainnet" and LTC testnet would be "litecoin-testnet". For currencies that use Raiden, we use the token contract address as the identifier. These token identifiers are shared with peers during the handshake and on any subsequent node state updates. The p2p messaging format is changed to accommodate this. This also drops static node values like the `nodePubKey` and `version` from the node state p2p message, as these values can not change mid-session for a peer. A future improvement may be to detect and support discrepancies in the currency symbol used by a peer for the same token. For example, a peer that advertises "XBT" with a token identifier of "bitcoin-mainnet" should be compatible with our "BTC" that also is identified as "bitcoin-mainnet". This is not included in this commit as it would require further refactoring and complexity. Another follow-up may be creating a test suite for the `Peer` class or refactoring it to move logic elsewhere (along with tests for that logic). There is some logic around handling node state updates where information regarding the peer's supported currencies may change that is currently not tested. Closes #910. BREAKING CHANGE: Changed p2p messaging structure for `SessionInit` and `NodeStateUpdate` packets.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
How to generically set a unique primary key for coins & ERC20 tokens to prevent intentionally or unintentionally tricking a trading partner into thinking my contract is WETH on xud p2p message layer, even though it's not. Swaps will fail in the end, but that would be annoying.
Short-term:
Use token contract address as unique identifier for ERC20.
Long-term:
A way might be to call getinfo from lnd/raiden and use this as chain id. Token chains like ethereum additionally need the token address (#897) to be part of the primary key.
The text was updated successfully, but these errors were encountered: