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

bugFix/change reward address #1749

Merged
merged 6 commits into from
May 16, 2020
Merged

Conversation

sasurobert
Copy link
Contributor

Change reward address fix - auction smart contract was calling in the wrong order the staking smart contract.

@sasurobert sasurobert changed the base branch from master to development May 15, 2020 13:58
@iulianpascalau iulianpascalau self-requested a review May 15, 2020 14:03
@iulianpascalau iulianpascalau added the type:bug Something isn't working label May 15, 2020
integrationTests/testProcessorNodeWithMultisigner.go Outdated Show resolved Hide resolved
@@ -183,13 +183,15 @@ func (s *stakingAuctionSC) changeRewardAddress(args *vmcommon.ContractCallInput)
return vmcommon.UserError
}

txData := "changeRewardAddress@" + hex.EncodeToString(registrationData.RewardAddress)
Copy link
Contributor

Choose a reason for hiding this comment

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

for this, I have used something like this in genesis package

arguments := make([]string, 0, len(registrationData.BlsPubKeys) + 2)
arguments = append(arguments, changeRewardAddressFunction)
arguments = append(arguments, hex.EncodeToString(registrationData.RewardAddress))
for _, blsKey := range registrationData.BlsPubKeys {
    arguments = append(arguments, hex.EncodeToString(blsKey))
}
txData := strings.Join(arguments, "@")

As can be seen, "changeRewardAddress" might be a const for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i would leave it like this. it is the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know, just it was clearer what you add to the arguments list.

vmOutput, err := s.executeOnStakingSC([]byte(txData))
isError := err != nil || vmOutput.ReturnCode != vmcommon.Ok
if isError {
log.LogIfError(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe log at debug level the original error from vmOutput

Copy link
Contributor Author

Choose a reason for hiding this comment

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

vmOutput cannot is only userError.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, could have used vmOutput.ReturnMessage but I'm guessing the system SC does not write that.

@@ -276,8 +276,7 @@ func (r *stakingSC) changeRewardAddress(args *vmcommon.ContractCallInput) vmcomm
return vmcommon.UserError
}

for i := 1; i < len(args.Arguments); i++ {
blsKey := args.Arguments[i]
for _, blsKey := range args.Arguments[1:] {
Copy link
Contributor

Choose a reason for hiding this comment

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

🥇

Copy link
Contributor

@LucianMincu LucianMincu left a comment

Choose a reason for hiding this comment

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

System tests passed.

@LucianMincu LucianMincu merged commit 252a29b into development May 16, 2020
@LucianMincu LucianMincu deleted the bugFix/change-reward-address branch May 16, 2020 14:00
@iulianpascalau iulianpascalau restored the bugFix/change-reward-address branch May 18, 2020 19:16
@miiu96 miiu96 deleted the bugFix/change-reward-address branch May 21, 2020 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants