feat(contracts): add new forwarder contracts #108
Conversation
contracts/NewForwarder.sol
Outdated
| // Address to which any funds sent to this contract will be forwarded | ||
| address public parentAddress; | ||
| address public feeAddress; | ||
| mapping(address => bool) public whiteListedAddresses; |
There was a problem hiding this comment.
If we are not adding a function to update whiteListedAddresses with additional addresses, Do we need to add this mapping?, Just need to have a comparison in the modifier to check the address is parentAddress or feeAddress, But if using mapping uses lesser gas, Its okay
There was a problem hiding this comment.
Modified this and removed mapping
contracts/NewForwarder.sol
Outdated
| parentAddress = _parentAddress; | ||
| feeAddress = _feeAddress; |
There was a problem hiding this comment.
Should we add null address checks here in constructor
gianchandania
force-pushed
the
WIN-506-update-forwarder-contracts
branch
from
937e1d6
to
a486408
Compare
gianchandania
force-pushed
the
WIN-506-update-forwarder-contracts
branch
from
8da20d3
to
26c90ef
Compare
gianchandania
force-pushed
the
WIN-506-update-forwarder-contracts
branch
from
26c90ef
to
0923ba1
Compare
gianchandania
force-pushed
the
WIN-506-update-forwarder-contracts
branch
from
93dd8fa
to
320df67
Compare
@barathcj Can you please share your thoughts on moving these updated contracts to a new eth-multisig-v6 repo |
gianchandania
force-pushed
the
WIN-506-update-forwarder-contracts
branch
from
34660ac
to
7d0ce22
Compare
gianchandania
force-pushed
the
WIN-506-update-forwarder-contracts
branch
from
7d0391c
to
1c5a815
Compare
| onlyAllowedAddress | ||
| { | ||
| address forwarderAddress = address(this); | ||
| uint256 length = tokenContractAddresses.length; |
There was a problem hiding this comment.
I think we should have a max length check here
There was a problem hiding this comment.
I think its okay not to have that, if it exceeds beyond a single transaction could handle, we should be getting a gaslimit error, Also the implementation of the token also matters when it comes to number of transactions we can do too.
| address forwarderAddress = address(this); | ||
| uint256 length = tokenContractAddresses.length; | ||
| for (uint256 i; i < length; i++) { | ||
| ERC20Interface instance = ERC20Interface(tokenContractAddresses[i]); |
There was a problem hiding this comment.
What happens if tokenContractAddresses[i] is not an erc20 contract address
also incase we pass the same erc20 contract address twice, I think forwarderBalance check will go through for both cases
In this case, do we see partial failures or the whole tx fails?
There was a problem hiding this comment.
If we pass the same token address multiple times, then it doesn't seem to be causing any failures or something, it just seem to be performing the tx once. https://sepolia.arbiscan.io/tx/0x82a819eed6b4ee0b87d40893bd92faf99db2f731074d1f65c548966e1a62face
Also if the token address passed is not an erc20 contract address, then it seems to be erroring during the gas estimate itself, so shouldn't be an issue of partial or whole tx failure
| /** | ||
| * @inheritdoc IForwarderV4 | ||
| */ | ||
| function batchFlushERC20Tokens(address[] calldata tokenContractAddresses) |
There was a problem hiding this comment.
Can you set some context on how are we doing to use this function from application side
There was a problem hiding this comment.
We should be able to batch token flushes together, should help in better gas savings.
contracts/ForwarderV4.sol
Outdated
| address target, | ||
| uint256 value, | ||
| bytes calldata data | ||
| ) external onlyAllowedAddress returns (bytes memory) { |
There was a problem hiding this comment.
wondering do we have to allow feeAddress here as well ?
cause using feeAddr we can move funds to any address
There was a problem hiding this comment.
I think that's a valid point. Bitgo on its own shouldn't be able to move any funds. This function should only be used in exceptional cases any ways
There was a problem hiding this comment.
updated this, now only parent address will be able to call this method
| ) external onlyUninitialized { | ||
| require(_parentAddress != address(0x0), 'Invalid parent address'); | ||
| parentAddress = _parentAddress; | ||
| require(_feeAddress != address(0x0), 'Invalid fee address'); |
There was a problem hiding this comment.
should we add feeAddr != parentAddr?
maybe we would be over doing it
There was a problem hiding this comment.
I think probably no need for this
gianchandania
force-pushed
the
WIN-506-update-forwarder-contracts
branch
from
e0bbd8c
to
31edb46
Compare
gianchandania
force-pushed
the
WIN-506-update-forwarder-contracts
branch
from
fdd3ff4
to
b8abd6d
Compare
| /// @notice Address which is allowed to call methods on this contract alongwith the parentAddress | ||
| address public feeAddress; | ||
| bool public autoFlush721 = true; | ||
| bool public autoFlush1155 = true; |
There was a problem hiding this comment.
ok thanks for the comparison. It is weird that arbitrum gas is twice than of optimism. Anyway, not a blocker but something to keep in mind in the future
|
For this comment, #108 (comment) actually arbitrum gas vary drastically probably because there is this L1 part of the gas which depend on the base fees of the Eth network |
This PR adds 3 contracts ForwarderV4.sol, ForwarderFactoryV4.sol and IForwarderV4.sol
Change in contracts (as compared to Forwarder.sol and ForwarderFactory.sol) is that we have added feeAddress as an address that will be one of the allowed addresses to call the methods of the contract along with parentAddress. This means the methods to flush the tokens, etc can be called using the gas tank address. Apart from this batchFlushERC20Tokens method to batch flush erc20 tokens has been added to the ForwarderV4 contract
WIN-506