Skip to content
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

Draft
wants to merge 8 commits into
base: bridge-v2
Choose a base branch
from
Draft

Bridge v3 #7

wants to merge 8 commits into from

Conversation

vovac12
Copy link

@vovac12 vovac12 commented Oct 16, 2022

PR just for review

@@ -57,6 +57,11 @@ const config: HardhatUserConfig = {
url: 'https://rpc.sepolia.org',
accounts: [sepoliaPrivateKey],
},
sepolia2: {

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)

Copy link
Author

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

apiKey: etherscanKey,
customChains: [
{
network: "sepolia",

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(

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

Copy link
Author

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

"Not enough tokens transferred"
);
if (tokenAddress == address(_addressUSDT)) {
uint256 balanceBefore = _addressUSDT.balanceOf(address(this));

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

IERC20 coin = IERC20(tokenAddress);
// untrusted call, relies on provided cryptographic proof
coin.safeTransfer(to, amount);
if (tokenAddress == address(_addressUSDT)) {

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants