-
Notifications
You must be signed in to change notification settings - Fork 4
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
Bridge v3 #7
base: bridge-v2
Are you sure you want to change the base?
Conversation
@@ -57,6 +57,11 @@ const config: HardhatUserConfig = { | |||
url: 'https://rpc.sepolia.org', | |||
accounts: [sepoliaPrivateKey], | |||
}, | |||
sepolia2: { |
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.
why? they are the same (sepolia and sepolia2)
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.
To have deployment data for test and stage environment
hardhat.config.ts
Outdated
apiKey: etherscanKey, | ||
customChains: [ | ||
{ | ||
network: "sepolia", |
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.
2 exactly the same chains
@@ -245,7 +245,8 @@ contract Bridge is EthTokenReciever { | |||
require( | |||
checkSignatures( | |||
keccak256( | |||
abi.encodePacked( |
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.
Transition from encode to encodePacked is not that necessary but might be taken as it may only affect the order of the params for the statement we sign but not the signature itself
Just make sure that it works fine when passing params to recoverAddress, now it looks okay, but it will not be superfluous to double check it
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.
It's needed to prevent signature collision
Bridge/Bridge.sol
Outdated
"Not enough tokens transferred" | ||
); | ||
if (tokenAddress == address(_addressUSDT)) { | ||
uint256 balanceBefore = _addressUSDT.balanceOf(address(this)); |
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.
balanceOf in usdt and other ERC20 are the same, so i guess there is no need to split using if
Bridge/Bridge.sol
Outdated
IERC20 coin = IERC20(tokenAddress); | ||
// untrusted call, relies on provided cryptographic proof | ||
coin.safeTransfer(to, amount); | ||
if (tokenAddress == address(_addressUSDT)) { |
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.
have you tested safeERC20 with USDT token? it's supposed to work correctly
PR just for review