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

Fix/owner cleanup delegation #2630

Merged
merged 7 commits into from Dec 30, 2020
Merged

Conversation

sasurobert
Copy link
Contributor

@sasurobert sasurobert commented Dec 30, 2020

  • fixed the wrong owner key hex encoding
  • fixed the getUnStakedTokensList view function

@raduchis raduchis self-requested a review December 30, 2020 09:59
epochStart/metachain/stakingDataProvider.go Show resolved Hide resolved
vm/systemSmartContracts/delegation.go Outdated Show resolved Hide resolved
vm/systemSmartContracts/delegation.go Show resolved Hide resolved
@@ -1447,7 +1447,7 @@ func (s *stakingSC) getOwner(args *vmcommon.ContractCallInput) vmcommon.ReturnCo
return vmcommon.UserError
}

s.eei.Finish([]byte(hex.EncodeToString(stakedData.OwnerAddress)))
s.eei.Finish(stakedData.OwnerAddress)
Copy link
Contributor

Choose a reason for hiding this comment

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

There still are some s.eei.Finish() with hexEncode on the RewardAddress: lines 1273 and 1384. Should not all be not hexEncoded?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have implemented this and indeed I have used the other examples in the system SC. What we have figure out today is that this hex encoding is useless as we need a complementary hex.Decode call when processing internal data. The code is now slightly optimized and one bug was fixed. If we need an API function for the owner field we will create a new function that will return the data in hex format.

epochStart/metachain/stakingDataProvider.go Show resolved Hide resolved
@@ -70,27 +70,32 @@ func TestDelegationSystemNodesOperations(t *testing.T) {
checkNodesStatus(t, tpn, vm.StakingSCAddress, blsKeys[:numNodesToStake], "staked")

//unStake 3 nodes
Copy link
Contributor

Choose a reason for hiding this comment

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

is this comment still relevant? You basically unstake all 5 nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not more relevant

vm/systemSmartContracts/delegation.go Show resolved Hide resolved
vm/systemSmartContracts/delegation.go Outdated Show resolved Hide resolved
@@ -1447,7 +1447,7 @@ func (s *stakingSC) getOwner(args *vmcommon.ContractCallInput) vmcommon.ReturnCo
return vmcommon.UserError
}

s.eei.Finish([]byte(hex.EncodeToString(stakedData.OwnerAddress)))
s.eei.Finish(stakedData.OwnerAddress)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have implemented this and indeed I have used the other examples in the system SC. What we have figure out today is that this hex encoding is useless as we need a complementary hex.Decode call when processing internal data. The code is now slightly optimized and one bug was fixed. If we need an API function for the owner field we will create a new function that will return the data in hex format.

Copy link
Contributor Author

@sasurobert sasurobert left a comment

Choose a reason for hiding this comment

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

Green from me

@LucianMincu LucianMincu merged commit 229fa2d into development Dec 30, 2020
@LucianMincu LucianMincu deleted the fix/owner-cleanup-delegation branch December 30, 2020 15:38
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.

None yet

5 participants