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

Security review fixes #116

Closed
wants to merge 16 commits into from
Closed

Security review fixes #116

wants to merge 16 commits into from

Conversation

ArseniiPetrovich
Copy link

This one is similar to #113, but also takes into account all review remarks.

@ghost ghost added the review label Nov 13, 2018
env_file: ./.env
environment:
- NODE_ENV=production
- VALIDATOR_ADDRESS=${VALIDATOR_ADDRESS:-0x8fd379246834eac74B8419FfdA202CF8051F7A03}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better not to use any default values here.
Applies to all occurences of VALIDATOR_ADDRESS and VALIDATOR_ADDRESS_PRIVATE_KEY below

@phahulin phahulin self-requested a review November 16, 2018 14:05
Copy link

@phahulin phahulin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Major audit findings were fixed

@patitonar
Copy link

Should this PR target develop instead of master branch @akolotov ?

Copy link
Collaborator

@akolotov akolotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do the changes in the README to reflect how to run the token bridge by using the docker now (https://github.com/poanetwork/token-bridge#docker).
Should some updates be placed to the section to reset the last processed blocks (https://github.com/poanetwork/token-bridge#rollback-the-last-processed-block-in-redis) and to the section where commands to test the bridge are described (https://github.com/poanetwork/token-bridge#native-to-erc20-mode-testing).

env_file: ./.env
environment:
- NODE_ENV=production
- VALIDATOR_ADDRESS=${VALIDATOR_ADDRESS}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need VALIDATOR_ADDRESS here since the address will be generated automatically based on the private key provided in the next line.
@patitonar please confirm.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct. VALIDATOR_ADDRESS is generated automatically using the private key.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I might be mistaking, but it won't work if not specifying the VALIDATOR_ADDRESS. However, let me check it first.

env_file: ./.env
environment:
- NODE_ENV=production
- VALIDATOR_ADDRESS=${VALIDATOR_ADDRESS}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need VALIDATOR_ADDRESS here since the address will be generated automatically based on the private key provided in the next line.

environment:
- NODE_ENV=production
- VALIDATOR_ADDRESS=${VALIDATOR_ADDRESS}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need VALIDATOR_ADDRESS here since the address will be generated automatically based on the private key provided in the next line.

@akolotov
Copy link
Collaborator

Should this PR target develop instead of master branch @akolotov ?

Yes, it is correct. We need to use develop branch. As soon as the PR is merged @patitonar, I think, we need to run stress testing to see if the CPU and memory limits declared by the docker-compose configuration file are enough.

@ghost ghost removed the review label Dec 5, 2018
@ArseniiPetrovich
Copy link
Author

Should be targeted to different branch.

@akolotov
Copy link
Collaborator

@ArseniiPetrovich are you going to open another PR for the develop branch?

@ArseniiPetrovich ArseniiPetrovich mentioned this pull request Dec 11, 2018
5 tasks
@ArseniiPetrovich
Copy link
Author

@ArseniiPetrovich are you going to open another PR for the develop branch?

Yes, @akolotov. Check #119, please

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.

5 participants