-
Notifications
You must be signed in to change notification settings - Fork 14
Conversation
go.mod
Dismissed
@@ -1,4 +1,4 @@ | |||
module github.com/ava-labs/teleporter-token-bridge | |||
module github.com/ava-labs/avalanche-interchain-token-transfer |
Check failure
Code scanning / Snyk Open Source
High severity - Dual license: GPL-3.0, LGPL-3.0 vulnerability in github.com/ava-labs/subnet-evm/accounts Error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These all point to subnet-evm 0.6.1. I'm not sure what the vulnerability is, but given the large number of Go packages names, my guess it's a recent Go CVE that has since been patched. Since we're upgrading to a newere subnet-evm shortly (and considering none of the critical code is directly dependent), this should be safe to ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A handful of minor nits, but overall looks great. Thanks for making all the renaming changes in one pass - that made it very straightforward to search the entire repo while reviewing.
@@ -67,7 +67,7 @@ struct SendAndCallInput { | |||
uint256 secondaryFee; | |||
} | |||
|
|||
enum BridgeMessageType { | |||
enum TokenTransferType { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we leave MessageType
in the name? REGISTER_REMOTE
is not a type of Token transfer, for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to TransfererMessage
and TransfererMessageType
.
go.mod
Dismissed
@@ -1,4 +1,4 @@ | |||
module github.com/ava-labs/teleporter-token-bridge | |||
module github.com/ava-labs/avalanche-interchain-token-transfer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These all point to subnet-evm 0.6.1. I'm not sure what the vulnerability is, but given the large number of Go packages names, my guess it's a recent Go CVE that has since been patched. Since we're upgrading to a newere subnet-evm shortly (and considering none of the critical code is directly dependent), this should be safe to ignore.
README.md
Outdated
|
||
Each bridge instance consists of one "home" contract and at least one but possibly many "remote" contracts. Each home contract instance manages one asset to be bridged out to `TokenRemote` instances. The home contract lives on the Subnet where the asset to be bridged exists and locks that asset as collateral to be bridged to other Subnets. The remote contracts, each of which has a single specified home contract, live on other Subnets that want to import the asset bridged by their specified home. The token bridges are designed to be permissionless: anyone can register compatible `TokenRemote` instances to allow for bridging tokens from the `TokenHome` instance to that new `TokenRemote` instance. The home contract keeps track of token balances bridged to each `TokenRemote` instance, and handles returning the original tokens back to the user when assets are bridged back to the `TokenHome` instance. `TokenRemote` instances are registered with their home contract via a Teleporter message upon creation. | ||
Each token transferer instance consists of one "home" contract and at least one but possibly many "remote" contracts. Each home contract instance manages one asset to be transferred out to `TokenRemote` instances. The home contract lives on the Subnet where the asset to be transferred exists and locks that asset as collateral to be transferred to other Subnets. The remote contracts, each of which has a single specified home contract, live on other Subnets that want to import the asset transferred by their specified home. The token transferers are designed to be permissionless: anyone can register compatible `TokenRemote` instances to allow for transferring tokens from the `TokenHome` instance to that new `TokenRemote` instance. The home contract keeps track of token balances transferred to each `TokenRemote` instance, and handles returning the original tokens back to the user when assets are transferred back to the `TokenHome` instance. `TokenRemote` instances are registered with their home contract via a Teleporter message upon creation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we're trying to get rid of "bridge" in favor of "transfer" most places but there are several places it feels incorrect to me:
- "collateral to be transferred"
- the collateral itself never gets transferred anywhere; it stays on the home
- "to allow for transferring tokens from the
TokenHome
instance to that newTokenRemote
instance"- the wrapped tokens remain locked in the home, not actually transferred to the remote
- "transferred back to the
TokenHome
instance" - "the asset transferred by their specified home."
- I think "bridged to" was better.
- if we do keep "transferred", it should be "transferred from their specified home", not "transferred by".
in one part it says that token balances are transferred to the remote, and I can make sense of that, but to word things as if the actual tokens themselves are transferred to the remote, that feels potentially confusing to me.
is the adoption of the "transfer" concept essential to the idea of this rename? are we trying to abstract the collateral/bridge concept away from the user? I took the user to be a developer building on a platform, who would understand collateral/bridge just fine, and abstracting that away under the guise of "transfer" feels potentially confusing.
if we do want to stick with the "transfer abstraction," maybe there should be a paragraph somewhere in the readme explaining that we're really doing lock-and-mint, but we just call it "transfer."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can first define what "transfer" means in this context.
Instead of: "The home contract lives on the Subnet where the asset to be transferred exists and locks that asset as collateral to be transferred to other Subnets."
We can define transfer as: "The home contract lives on the Subnet where the asset to be transferred exists. A transfer consists of locking the asset as collateral on the home Subnet and minting a representation of the asset on the remote Subnet."
We can further position our description away from bridging terminology by removing "collateral" altogether and simply say "A transfer consists of exporting the asset by locking it in the home contract and importing the asset on the remote Subnet"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea to define transfer and replaced with Bernard's line. Don't think we need to go further and remove "collateral" though.
Continuing Cam's message, between |
Co-authored-by: cam-schultz <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>
Co-authored-by: cam-schultz <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>
Co-authored-by: cam-schultz <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>
Rename
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Rename
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Rename
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good. I added a comment responding to Gene's comment about transferring collateral.
README.md
Outdated
|
||
Each bridge instance consists of one "home" contract and at least one but possibly many "remote" contracts. Each home contract instance manages one asset to be bridged out to `TokenRemote` instances. The home contract lives on the Subnet where the asset to be bridged exists and locks that asset as collateral to be bridged to other Subnets. The remote contracts, each of which has a single specified home contract, live on other Subnets that want to import the asset bridged by their specified home. The token bridges are designed to be permissionless: anyone can register compatible `TokenRemote` instances to allow for bridging tokens from the `TokenHome` instance to that new `TokenRemote` instance. The home contract keeps track of token balances bridged to each `TokenRemote` instance, and handles returning the original tokens back to the user when assets are bridged back to the `TokenHome` instance. `TokenRemote` instances are registered with their home contract via a Teleporter message upon creation. | ||
Each token transferer instance consists of one "home" contract and at least one but possibly many "remote" contracts. Each home contract instance manages one asset to be transferred out to `TokenRemote` instances. The home contract lives on the Subnet where the asset to be transferred exists and locks that asset as collateral to be transferred to other Subnets. The remote contracts, each of which has a single specified home contract, live on other Subnets that want to import the asset transferred by their specified home. The token transferers are designed to be permissionless: anyone can register compatible `TokenRemote` instances to allow for transferring tokens from the `TokenHome` instance to that new `TokenRemote` instance. The home contract keeps track of token balances transferred to each `TokenRemote` instance, and handles returning the original tokens back to the user when assets are transferred back to the `TokenHome` instance. `TokenRemote` instances are registered with their home contract via a Teleporter message upon creation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can first define what "transfer" means in this context.
Instead of: "The home contract lives on the Subnet where the asset to be transferred exists and locks that asset as collateral to be transferred to other Subnets."
We can define transfer as: "The home contract lives on the Subnet where the asset to be transferred exists. A transfer consists of locking the asset as collateral on the home Subnet and minting a representation of the asset on the remote Subnet."
We can further position our description away from bridging terminology by removing "collateral" altogether and simply say "A transfer consists of exporting the asset by locking it in the home contract and importing the asset on the remote Subnet"
README.md
Outdated
@@ -8,7 +8,7 @@ The avalanche-interchain-token-transfer contracts are non-upgradeable and cannot | |||
|
|||
Avalanche interchain token transfer is an application that allows users to transfer tokens between Subnets. The token transferer is a set of smart contracts that are deployed across multiple Subnets, and leverages [Teleporter](https://github.com/ava-labs/teleporter) for cross-chain communication. | |||
|
|||
Each token transferer instance consists of one "home" contract and at least one but possibly many "remote" contracts. Each home contract instance manages one asset to be transferred out to `TokenRemote` instances. The home contract lives on the Subnet where the asset to be transferred exists and locks that asset as collateral to be transferred to other Subnets. The remote contracts, each of which has a single specified home contract, live on other Subnets that want to import the asset transferred by their specified home. The token transferers are designed to be permissionless: anyone can register compatible `TokenRemote` instances to allow for transferring tokens from the `TokenHome` instance to that new `TokenRemote` instance. The home contract keeps track of token balances transferred to each `TokenRemote` instance, and handles returning the original tokens back to the user when assets are transferred back to the `TokenHome` instance. `TokenRemote` instances are registered with their home contract via a Teleporter message upon creation. | |||
Each token transferer instance consists of one "home" contract and at least one but possibly many "remote" contracts. Each home contract instance manages one asset to be transferred out to `TokenRemote` instances. The home contract lives on the Subnet where the asset to be transferred exists and locks that asset as collateral for futuer transfers to other Subnets. The remote contracts, each of which has a single specified home contract, live on other Subnets that want to import the asset transferred by their specified home. The token transferers are designed to be permissionless: anyone can register compatible `TokenRemote` instances to allow for transferring tokens from the `TokenHome` instance to that new `TokenRemote` instance. The home contract keeps track of token balances transferred to each `TokenRemote` instance, and handles returning the original tokens back to the user when assets are transferred back to the `TokenHome` instance. `TokenRemote` instances are registered with their home contract via a Teleporter message upon creation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each token transferer instance consists of one "home" contract and at least one but possibly many "remote" contracts. Each home contract instance manages one asset to be transferred out to `TokenRemote` instances. The home contract lives on the Subnet where the asset to be transferred exists and locks that asset as collateral for futuer transfers to other Subnets. The remote contracts, each of which has a single specified home contract, live on other Subnets that want to import the asset transferred by their specified home. The token transferers are designed to be permissionless: anyone can register compatible `TokenRemote` instances to allow for transferring tokens from the `TokenHome` instance to that new `TokenRemote` instance. The home contract keeps track of token balances transferred to each `TokenRemote` instance, and handles returning the original tokens back to the user when assets are transferred back to the `TokenHome` instance. `TokenRemote` instances are registered with their home contract via a Teleporter message upon creation. | |
Each token transferer instance consists of one "home" contract and at least one but possibly many "remote" contracts. Each home contract instance manages one asset to be transferred out to `TokenRemote` instances. The home contract lives on the Subnet where the asset to be transferred exists and locks that asset as collateral for future transfers to other Subnets. The remote contracts, each of which has a single specified home contract, live on other Subnets that want to import the asset transferred by their specified home. The token transferers are designed to be permissionless: anyone can register compatible `TokenRemote` instances to allow for transferring tokens from the `TokenHome` instance to that new `TokenRemote` instance. The home contract keeps track of token balances transferred to each `TokenRemote` instance, and handles returning the original tokens back to the user when assets are transferred back to the `TokenHome` instance. `TokenRemote` instances are registered with their home contract via a Teleporter message upon creation. |
.golangci.yml
Outdated
@@ -30,4 +30,4 @@ linters-settings: | |||
simplify: true | |||
misspell: | |||
ignore-words: | |||
- transferer | |||
- transferrer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, does the linter still consider this a misspelling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope all good now, removed
README.md
Outdated
@@ -8,7 +8,7 @@ The avalanche-interchain-token-transfer contracts are non-upgradeable and cannot | |||
|
|||
Avalanche interchain token transfer is an application that allows users to transfer tokens between Subnets. The token transferer is a set of smart contracts that are deployed across multiple Subnets, and leverages [Teleporter](https://github.com/ava-labs/teleporter) for cross-chain communication. | |||
|
|||
Each token transferer instance consists of one "home" contract and at least one but possibly many "remote" contracts. Each home contract instance manages one asset to be transferred out to `TokenRemote` instances. The home contract lives on the Subnet where the asset to be transferred exists and locks that asset as collateral to be transferred to other Subnets. The remote contracts, each of which has a single specified home contract, live on other Subnets that want to import the asset transferred by their specified home. The token transferers are designed to be permissionless: anyone can register compatible `TokenRemote` instances to allow for transferring tokens from the `TokenHome` instance to that new `TokenRemote` instance. The home contract keeps track of token balances transferred to each `TokenRemote` instance, and handles returning the original tokens back to the user when assets are transferred back to the `TokenHome` instance. `TokenRemote` instances are registered with their home contract via a Teleporter message upon creation. | |||
Each token transferer instance consists of one "home" contract and at least one but possibly many "remote" contracts. Each home contract instance manages one asset to be transferred out to `TokenRemote` instances. The home contract lives on the Subnet where the asset to be transferred exists. A transfer consists of locking the asset as collateral on the home Subnet and minting a representation of the asset on the remote Subnet.. The remote contracts, each of which has a single specified home contract, live on other Subnets that want to import the asset transferred by their specified home. The token transferers are designed to be permissionless: anyone can register compatible `TokenRemote` instances to allow for transferring tokens from the `TokenHome` instance to that new `TokenRemote` instance. The home contract keeps track of token balances transferred to each `TokenRemote` instance, and handles returning the original tokens back to the user when assets are transferred back to the `TokenHome` instance. `TokenRemote` instances are registered with their home contract via a Teleporter message upon creation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each token transferer instance consists of one "home" contract and at least one but possibly many "remote" contracts. Each home contract instance manages one asset to be transferred out to `TokenRemote` instances. The home contract lives on the Subnet where the asset to be transferred exists. A transfer consists of locking the asset as collateral on the home Subnet and minting a representation of the asset on the remote Subnet.. The remote contracts, each of which has a single specified home contract, live on other Subnets that want to import the asset transferred by their specified home. The token transferers are designed to be permissionless: anyone can register compatible `TokenRemote` instances to allow for transferring tokens from the `TokenHome` instance to that new `TokenRemote` instance. The home contract keeps track of token balances transferred to each `TokenRemote` instance, and handles returning the original tokens back to the user when assets are transferred back to the `TokenHome` instance. `TokenRemote` instances are registered with their home contract via a Teleporter message upon creation. | |
Each token transferer instance consists of one "home" contract and at least one but possibly many "remote" contracts. Each home contract instance manages one asset to be transferred out to `TokenRemote` instances. The home contract lives on the Subnet where the asset to be transferred exists. A transfer consists of locking the asset as collateral on the home Subnet and minting a representation of the asset on the remote Subnet. The remote contracts, each of which has a single specified home contract, live on other Subnets that want to import the asset transferred by their specified home. The token transferers are designed to be permissionless: anyone can register compatible `TokenRemote` instances to allow for transferring tokens from the `TokenHome` instance to that new `TokenRemote` instance. The home contract keeps track of token balances transferred to each `TokenRemote` instance, and handles returning the original tokens back to the user when assets are transferred back to the `TokenHome` instance. `TokenRemote` instances are registered with their home contract via a Teleporter message upon creation. |
Co-authored-by: cam-schultz <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>
Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>
Why this should be merged
Rename to follow new
avalanche-interchain-token-transfer
name