-
Notifications
You must be signed in to change notification settings - Fork 105
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.
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
Asset from polkadot #1155
Asset from polkadot #1155
Changes from 44 commits
7501630
e0c3581
f070e16
780c7bd
27353f0
a543f0b
6212597
d28d9df
c23f8bb
8c8afac
0b3344c
2d17fca
047becf
3fc6cab
e53afd0
5e9820b
fddb9cf
e5d9dd8
9f7e99b
724c649
6eb8344
7c29684
edccccd
c170eea
953245b
adc73a3
28f513d
9b7811d
b00538f
3da6be4
e051f4c
79c1ba0
fdf15aa
1e6dac0
3439213
c2adf8f
b0b582d
985115f
ec33548
32f8d86
b2a666a
17f328b
6148c03
4075c11
fcd7539
dc78276
07545cf
03cf01c
e9013cd
a028d5c
3b7e178
63e8322
c887e7e
f892da4
84bb52c
64b9ef4
de3863c
25092dd
001c990
324cf53
fe4cb73
46bc45f
cf596f3
6efc7a1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,29 +2,20 @@ | |
// SPDX-FileCopyrightText: 2023 Snowfork <[email protected]> | ||
pragma solidity 0.8.23; | ||
|
||
import {AgentExecuteCommand, ParaID} from "./Types.sol"; | ||
import {ParaID} from "./Types.sol"; | ||
import {SubstrateTypes} from "./SubstrateTypes.sol"; | ||
|
||
import {IERC20} from "./interfaces/IERC20.sol"; | ||
import {SafeTokenTransfer, SafeNativeTransfer} from "./utils/SafeTransfer.sol"; | ||
import {ERC20} from "./ERC20.sol"; | ||
import {Gateway} from "./Gateway.sol"; | ||
|
||
/// @title Code which will run within an `Agent` using `delegatecall`. | ||
/// @dev This is a singleton contract, meaning that all agents will execute the same code. | ||
contract AgentExecutor { | ||
using SafeTokenTransfer for IERC20; | ||
using SafeNativeTransfer for address payable; | ||
|
||
/// @dev Execute a message which originated from the Polkadot side of the bridge. In other terms, | ||
/// the `data` parameter is constructed by the BridgeHub parachain. | ||
/// | ||
function execute(bytes memory data) external { | ||
(AgentExecuteCommand command, bytes memory params) = abi.decode(data, (AgentExecuteCommand, bytes)); | ||
if (command == AgentExecuteCommand.TransferToken) { | ||
(address token, address recipient, uint128 amount) = abi.decode(params, (address, address, uint128)); | ||
_transferToken(token, recipient, amount); | ||
} | ||
} | ||
|
||
/// @dev Transfer ether to `recipient`. Unlike `_transferToken` This logic is not nested within `execute`, | ||
/// as the gateway needs to control an agent's ether balance directly. | ||
/// | ||
|
@@ -33,7 +24,16 @@ contract AgentExecutor { | |
} | ||
|
||
/// @dev Transfer ERC20 to `recipient`. Only callable via `execute`. | ||
function _transferToken(address token, address recipient, uint128 amount) internal { | ||
function transferToken(address token, address recipient, uint128 amount) external { | ||
IERC20(token).safeTransfer(recipient, amount); | ||
} | ||
|
||
/// @dev Mint ERC20 token to `recipient`. | ||
function mintToken(address token, address recipient, uint256 amount) external { | ||
ERC20(token).mint(recipient, amount); | ||
} | ||
|
||
function burnToken(address token, address sender, uint256 amount) external { | ||
ERC20(token).burn(sender, amount); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,191 @@ | ||
// SPDX-License-Identifier: MIT | ||
// SPDX-FileCopyrightText: 2023 Axelar Network | ||
// SPDX-FileCopyrightText: 2023 Snowfork <[email protected]> | ||
|
||
pragma solidity 0.8.23; | ||
|
||
import {IERC20} from "./interfaces/IERC20.sol"; | ||
import {IERC20Permit} from "./interfaces/IERC20Permit.sol"; | ||
import {ERC20Lib} from "./ERC20Lib.sol"; | ||
|
||
/** | ||
* @dev Implementation of the {IERC20} interface. | ||
* | ||
* This implementation is agnostic to the way tokens are created. This means | ||
* that a supply mechanism has to be added in a derived contract using {_mint}. | ||
* This supply mechanism has been added in {ERC20Permit-mint}. | ||
* | ||
* We have followed general OpenZeppelin guidelines: functions revert instead | ||
* of returning `false` on failure. This behavior is conventional and does | ||
* not conflict with the expectations of ERC20 applications. | ||
* | ||
* Additionally, an {Approval} event is emitted on calls to {transferFrom}. | ||
* This allows applications to reconstruct the allowance for all accounts just | ||
* by listening to these events. Other implementations of the EIP may not emit | ||
* these events, as it isn't required by the specification. | ||
* | ||
* Finally, the non-standard {decreaseAllowance} and {increaseAllowance} | ||
* functions have been added to mitigate the well-known issues around setting | ||
* allowances. See {IERC20-approve}. | ||
*/ | ||
contract ERC20 is IERC20, IERC20Permit { | ||
using ERC20Lib for ERC20Lib.TokenStorage; | ||
|
||
error Unauthorized(); | ||
|
||
ERC20Lib.TokenStorage token; | ||
|
||
address public immutable OWNER; | ||
|
||
uint8 public immutable decimals; | ||
|
||
string public name; | ||
string public symbol; | ||
|
||
/** | ||
* @dev Sets the values for {name}, {symbol}, and {decimals}. | ||
*/ | ||
constructor(address _owner, string memory name_, string memory symbol_, uint8 decimals_) { | ||
OWNER = _owner; | ||
name = name_; | ||
symbol = symbol_; | ||
decimals = decimals_; | ||
token.init(name_); | ||
} | ||
|
||
modifier onlyOwner() { | ||
if (msg.sender != OWNER) { | ||
revert Unauthorized(); | ||
} | ||
_; | ||
} | ||
|
||
/** | ||
* @dev Creates `amount` tokens and assigns them to `account`, increasing | ||
* the total supply. Can only be called by the owner. | ||
* | ||
* Emits a {Transfer} event with `from` set to the zero address. | ||
* | ||
* Requirements: | ||
* | ||
* - `account` cannot be the zero address. | ||
*/ | ||
function mint(address account, uint256 amount) external virtual onlyOwner { | ||
token.mint(account, amount); | ||
} | ||
|
||
/** | ||
* @dev Destroys `amount` tokens from the account. | ||
*/ | ||
function burn(address account, uint256 amount) external virtual onlyOwner { | ||
token.burn(account, amount); | ||
} | ||
|
||
/** | ||
* @dev See {IERC20-transfer}. | ||
* | ||
* Requirements: | ||
* | ||
* - `recipient` cannot be the zero address. | ||
* - the caller must have a balance of at least `amount`. | ||
*/ | ||
function transfer(address recipient, uint256 amount) external virtual override returns (bool) { | ||
return token.transfer(msg.sender, recipient, amount); | ||
} | ||
|
||
/** | ||
* @dev See {IERC20-approve}. | ||
* | ||
* NOTE: Prefer the {increaseAllowance} and {decreaseAllowance} methods, as | ||
* they aren't vulnerable to the frontrunning attack described here: | ||
* https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729 | ||
* See {IERC20-approve}. | ||
* | ||
* NOTE: If `amount` is the maximum `uint256`, the allowance is not updated on | ||
* `transferFrom`. This is semantically equivalent to an infinite approval. | ||
* | ||
* Requirements: | ||
* | ||
* - `spender` cannot be the zero address. | ||
*/ | ||
function approve(address spender, uint256 amount) external virtual override returns (bool) { | ||
return token.approve(msg.sender, spender, amount); | ||
} | ||
|
||
/** | ||
* @dev See {IERC20-transferFrom}. | ||
* | ||
* Emits an {Approval} event indicating the updated allowance. This is not | ||
* required by the EIP. See the note at the beginning of {ERC20}. | ||
* | ||
* Requirements: | ||
* | ||
* - `sender` and `recipient` cannot be the zero address. | ||
* - `sender` must have a balance of at least `amount`. | ||
* - the caller must have allowance for ``sender``'s tokens of at least | ||
* `amount`. | ||
*/ | ||
function transferFrom(address sender, address recipient, uint256 amount) external virtual override returns (bool) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should move most of these external functions, especially the larger ones, to a common library, to save on deployment costs. Especially since this code is going to be deployed hundreds of times. It will save on bridging fees. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we should just be deploying simple proxies for each token rather than a full erc20 contract each time? but it's not a big deal i guess, even hundreds of times of redeploys won't cost too much There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, maybe we can delegate these advanced features(allowance/permit) to something like https://github.com/Uniswap/permit2 Currently, the ERC20.sol is only 4KB so may not be a big issue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking about this yesterday actually. It seems ERC-6909 is the emerging standard for multi-token contracts (replacing ERC-1155). Its being used in Uniswap-v4 And so I was thinking Gateway can maintain a single ERC-6909 contract, and then instantiate thin ERC20 proxies over it. I am ambivalent about it, which is why I didn't suggest it previously. Positives:
Negatives:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we make a decision here? maybe best to keep it simple and stick with existing standards? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I am favor the current approach - just brought the idea up in case anyone had strong opinions. @yrong lets go with the current approach, but with most of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Vincent, so which functions you'd like to move to a library? I would assume the contract size may not be a big issue here and will not make a difference for the bridging fees? If we do want to slim the contract somehow, I'd prefer to remove all token permit/approve functions to make For all EIP-2612, EIP-1271 features we delegate to a separate contract like permit2. so WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per this permit2 explainer, permit2 is really meant to be compatibility layer for older tokens that don't support EIP-2612. Its not meant to be a replacement for it. And the user still has to manually approve the permit2 contract to spend the tokens. So there is an initial setup step that detracts from the UX experience. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it will take long to introduce After that's done, I would suggest moving There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
return token.transferFrom(sender, recipient, amount); | ||
} | ||
|
||
/** | ||
* @dev Atomically increases the allowance granted to `spender` by the caller. | ||
* | ||
* This is an alternative to {approve} that can be used as a mitigation for | ||
* problems described in {IERC20-approve}. | ||
* | ||
* Emits an {Approval} event indicating the updated allowance. | ||
* | ||
* Requirements: | ||
* | ||
* - `spender` cannot be the zero address. | ||
*/ | ||
function increaseAllowance(address spender, uint256 addedValue) external virtual returns (bool) { | ||
return token.increaseAllowance(spender, addedValue); | ||
} | ||
|
||
/** | ||
* @dev Atomically decreases the allowance granted to `spender` by the caller. | ||
* | ||
* This is an alternative to {approve} that can be used as a mitigation for | ||
* problems described in {IERC20-approve}. | ||
* | ||
* Emits an {Approval} event indicating the updated allowance. | ||
* | ||
* Requirements: | ||
* | ||
* - `spender` cannot be the zero address. | ||
* - `spender` must have allowance for the caller of at least | ||
* `subtractedValue`. | ||
*/ | ||
function decreaseAllowance(address spender, uint256 subtractedValue) external virtual returns (bool) { | ||
return token.decreaseAllowance(spender, subtractedValue); | ||
} | ||
|
||
function permit(address issuer, address spender, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) | ||
external | ||
{ | ||
token.permit(issuer, spender, value, deadline, v, r, s); | ||
} | ||
|
||
function balanceOf(address account) external view returns (uint256) { | ||
return token.balancesOf(account); | ||
} | ||
|
||
function nonces(address account) external view returns (uint256) { | ||
return token.noncesOf(account); | ||
} | ||
|
||
function totalSupply() external view returns (uint256) { | ||
return token.totalSupplyOf(); | ||
} | ||
|
||
function allowance(address owner, address spender) external view returns (uint256) { | ||
return token.allowanceOf(owner, spender); | ||
} | ||
|
||
function DOMAIN_SEPARATOR() external view returns (bytes32) { | ||
return token.domainSeparatorOf(); | ||
} | ||
} |
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 a better name for this would be something like
WrappedPolkadotAsset
or something more descriptive. This name feels extremely generic. We can also specialize this contract with thesetOwner
function and events for minting, burning, transfer, etc.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 prefer the name
ERC20
at least for the base contract.However, along the lines you suggest, we could add a child contract
WrappedToken
that inherits fromERC20
, with specialized functions for burning and anything else needed.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.
Actually, I think we should just leave the naming alone for now while we sort out the more important protocol-level issues you raised in other comments.