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.

Sign up for GitHub

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

Jump to bottom

Extend ERC677BridgeToken contract for xDai-POA bridge #129

Merged
merged 7 commits into from
Jan 4, 2019
Merged

Extend ERC677BridgeToken contract for xDai-POA bridge #129

merged 7 commits into from
Jan 4, 2019

Conversation

varasev
Copy link
Contributor

@varasev varasev commented Dec 26, 2018

These changes add mintReward, stake and withdraw functions which are used by DPoS contracts (BlockReward and ValidatorSet).

@ghost ghost assigned varasev Dec 26, 2018
@varasev varasev requested a review from akolotov December 26, 2018 11:41
akolotov
akolotov requested changes Dec 26, 2018
View reviewed changes
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.

Could you keep ERC677BridgeToken unchanged? In this case all these changes should be introduced to another contract like RewardableBridgeToken as so it will inherit functionality of ERC677BridgeToken?

@ghost ghost added the in progress label Dec 26, 2018
@varasev
Copy link
Contributor Author

varasev commented Dec 26, 2018

@akolotov Ok, done: 60644ad

@akolotov
Copy link
Collaborator

Thanks for quick response.
I think it makes sense to modify deployment script for the native-to-erc mode to support a new parameter DEPLOY_REWARDABLE_TOKEN that could have value true or false. If it is true, ERC677BridgeTokenRewardable will be deployed, if it equals false (default value), the script must deal with ERC677BridgeToken.

Another moment - should we extend tests to cover new functionality?

@ghost ghost removed the in progress label Dec 27, 2018
@varasev
Copy link
Contributor Author

varasev commented Dec 27, 2018

I think it makes sense to modify deployment script for the native-to-erc mode to support a new parameter DEPLOY_REWARDABLE_TOKEN ...

Done: bc289bd

Another moment - should we extend tests to cover new functionality?

Yes, it makes sense. We need to extend test/poa20_test.js, am I right?

@akolotov akolotov requested a review from patitonar December 27, 2018 08:20
@akolotov
Copy link
Collaborator

Yes, it makes sense. We need to extend test/poa20_test.js, am I right?

Yes, it is point to start. Then we need to look at scenarios where the new functionality is being used,

akolotov
akolotov reviewed Dec 27, 2018
View reviewed changes
deploy/README.md Outdated
DEPLOY_REWARDABLE_TOKEN=false
# The address of ValidatorSet contract used by ERC677BridgeTokenRewardable contract.
# Makes sense only when DEPLOY_REWARDABLE_TOKEN=true
VALIDATOR_SET_ADDRESS=0x
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm... wait a minute: is this bridge validators or network validators? If they are just for network let's rename the parameter to DPOS_VALIDATOR_SET_ADDRESS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the address of ValidatorSet contract which contains network validators. Ok, I've renamed the param: 47f766b

deploy/src/native_to_erc/foreign.js Show resolved Hide resolved
deploy/src/native_to_erc/foreign.js Show resolved Hide resolved
@ghost ghost added the in progress label Dec 27, 2018
@varasev
Copy link
Contributor Author

varasev commented Dec 27, 2018

Yes, it is point to start. Then we need to look at scenarios where the new functionality is being used,

OK, I'll add the tests a bit later.

@ghost ghost removed the in progress label Dec 28, 2018
@varasev
Copy link
Contributor Author

varasev commented Dec 28, 2018

The poa20 tests have been extended: 45a094f

@akolotov
Merge pull request omni#131 from poanetwork/develop
4615162 
Merge the changes related to security updates to the master branch
akolotov
akolotov requested changes Jan 3, 2019
View reviewed changes
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 resolve merge conflicts. As soon as they are resolved we will merge this PR.

@ghost ghost added the in progress label Jan 4, 2019
@varasev
Copy link
Contributor Author

varasev commented Jan 4, 2019

The conflicts have been resolved.

@akolotov
Copy link
Collaborator

akolotov commented Jan 4, 2019

@varasev sorry. just noticed. please change the target branch to develop.

@varasev varasev changed the base branch from master to develop January 4, 2019 07:32
@varasev
Copy link
Contributor Author

varasev commented Jan 4, 2019

please change the target branch to develop.

Done.

@akolotov akolotov merged commit 8a7cf85 into omni:develop Jan 4, 2019
@ghost ghost removed the in progress label Jan 4, 2019
@akolotov akolotov removed the review label Jan 4, 2019
@varasev varasev deleted the update-ERC677BridgeToken branch February 11, 2019 07:00
@akolotov akolotov mentioned this pull request May 22, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akolotov akolotov akolotov requested changes

@patitonar patitonar patitonar approved these changes

Assignees

@varasev varasev

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@varasev @akolotov @patitonar @fernandomg