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

View funcs delegation #2637

Merged
merged 33 commits into from Jan 6, 2021
Merged

View funcs delegation #2637

merged 33 commits into from Jan 6, 2021

Conversation

sasurobert
Copy link
Contributor

@sasurobert sasurobert commented Jan 4, 2021

added new view functions to delegation sc
added protection of unStaking / unBonding all tokens when there are active nodes
take out warm instance from config
added integration tests for different scenarios

@iulianpascalau iulianpascalau self-requested a review January 4, 2021 15:24
@@ -908,7 +909,7 @@ func (sc *scProcessor) isSCExecutionAfterBuiltInFunc(
CallValue: big.NewInt(0),
CallType: callType,
GasPrice: vmInput.GasPrice,
GasProvided: vmOutput.GasRemaining - vmInput.GasLocked,
GasProvided: scExecuteOutTransfer.GasLimit,
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 backwards compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is. This feature is under built in functions

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@@ -1966,6 +1980,85 @@ func (d *delegation) getClaimableRewards(args *vmcommon.ContractCallInput) vmcom
return vmcommon.Ok
}

func (d *delegation) isDelegator(args *vmcommon.ContractCallInput) vmcommon.ReturnCode {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add these functions in the Jira task list so we can have unit tests for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bogdan already did it and opened PR

Copy link
Contributor

Choose a reason for hiding this comment

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

We can easily forget about them if the PRs overlap

Copy link
Contributor

Choose a reason for hiding this comment

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

the PR with unit tests is directed to this branch. We are safe

Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot to test a function

…-unit-tests

EN-8537: unit tests for new view functions
@@ -313,7 +313,7 @@ func (d *delegationManager) getAllContractAddresses(args *vmcommon.ContractCallI
return vmcommon.UserError
}

for _, address := range contractList.Addresses {
for _, address := range contractList.Addresses[1:] {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add a check that len(contractList.Addresses)>0, otherwise this will panic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

at initialization it will be at least one. I can add for extra security.

Copy link
Contributor

@iulianpascalau iulianpascalau left a comment

Choose a reason for hiding this comment

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

Function getUserUnDelegatedList not tested @bogdan-rosianu. Task EN-8525 is not complete

iulianpascalau
iulianpascalau previously approved these changes Jan 4, 2021
@@ -1575,6 +1587,11 @@ func (v *validatorSC) unBondTokens(args *vmcommon.ContractCallInput) vmcommon.Re
return vmcommon.Ok
}

if registrationData.NumRegistered > 0 && registrationData.TotalStakeValue.Cmp(v.minDeposit) < 0 {
v.eei.AddReturnMessage("cannot unStake tokens, the validator would remain without min deposit, nodes are still active")
Copy link
Contributor

Choose a reason for hiding this comment

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

cannot unBond

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


callFunctionAndCheckResult(t, "unBondTokens", sc, caller, nil, zero, vmcommon.UserError)
vmOutput := eei.CreateVMOutput()
assert.True(t, strings.Contains(vmOutput.ReturnMessage, "cannot unStake tokens, the validator would remain without min deposit, nodes are still active"))
Copy link
Contributor

Choose a reason for hiding this comment

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

cannot unBond

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

iulianpascalau
iulianpascalau previously approved these changes Jan 4, 2021
SebastianMarian
SebastianMarian previously approved these changes Jan 4, 2021
return vmcommon.UserError
}
if args.CallValue.Cmp(zero) != 0 {
return vmcommon.UserError
Copy link
Contributor

Choose a reason for hiding this comment

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

No s.eei.AddReturnMessage ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -120,6 +120,7 @@ func (e *esdtTransfer) ProcessBuiltinFunction(
}

if isSCCallAfter {
vmOutput.GasRemaining = vmInput.GasProvided - e.funcGasCost
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be negative in some edge cases? safeSub ?

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 do not think so. but added safeSub

Copy link
Contributor

@stefanandy stefanandy left a comment

Choose a reason for hiding this comment

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

System Test Passed

@iulianpascalau iulianpascalau merged commit 75ed405 into development Jan 6, 2021
@iulianpascalau iulianpascalau deleted the view-funcs-delegation branch January 6, 2021 11:01
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

8 participants