-
Notifications
You must be signed in to change notification settings - Fork 1k
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 vote #1792
Fix vote #1792
Conversation
@shargon Ready for merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If state_account.VoteTo
keeps the original value, then other changes should also be rolled back, otherwise transaction consistency cannot be guaranteed. Maybe Vote()
shouldn't return bool
, but should throw an exception when something goes wrong.
Retesting. |
Test passed:
|
@erikzhang Could you review it again? |
validator_new = engine.Snapshot.Storages.GetAndChange(CreateStorageKey(Prefix_Candidate).Add(voteTo))?.GetInteroperable<CandidateState>(); | ||
if (validator_new is null) return false; | ||
if (!validator_new.Registered) return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not throw an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning false
can give the contract a chance to recover from the exception. Throwing an exception will directly terminate the virtual machine. we should try not to throw exceptions if the consistency can be guaranteed.
Retest passed.
|
Looks good now. |
@erikzhang Shall we merge? |
@shargon Need your merge. |
* Classes related to Neo.SmartContract types should not be internal (#1785) * Classes related to Neo.SmartContract types should not be internal Fixes #1782 * public callbacks + protected internal service methods * Add AsReadOnly() Co-authored-by: Harry Pierson <[email protected]> Co-authored-by: Shargon <[email protected]> * Add AppEngine.ValidateCallFlags to be callable by subclasses (#1784) * Update ApplicationEngine.cs separate call flag validation into separate function so it can be called by ApplicationEngine subclasses. This is needed for the debugger, which overrides a few of the standard service implementations * fix whitespace * protected internal addGas Co-authored-by: Harry Pierson <[email protected]> * Fix vote (#1792) * fix vote * add ut * throw exception * fix * fix ut Co-authored-by: Tommo-L <[email protected]> Co-authored-by: erikzhang <[email protected]> * preview3 * Update dependency Co-authored-by: Harry Pierson <[email protected]> Co-authored-by: Harry Pierson <[email protected]> Co-authored-by: Shargon <[email protected]> Co-authored-by: Luchuan <[email protected]> Co-authored-by: Tommo-L <[email protected]>
* Classes related to Neo.SmartContract types should not be internal (neo-project#1785) * Classes related to Neo.SmartContract types should not be internal Fixes neo-project#1782 * public callbacks + protected internal service methods * Add AsReadOnly() Co-authored-by: Harry Pierson <[email protected]> Co-authored-by: Shargon <[email protected]> * Add AppEngine.ValidateCallFlags to be callable by subclasses (neo-project#1784) * Update ApplicationEngine.cs separate call flag validation into separate function so it can be called by ApplicationEngine subclasses. This is needed for the debugger, which overrides a few of the standard service implementations * fix whitespace * protected internal addGas Co-authored-by: Harry Pierson <[email protected]> * Fix vote (neo-project#1792) * fix vote * add ut * throw exception * fix * fix ut Co-authored-by: Tommo-L <[email protected]> Co-authored-by: erikzhang <[email protected]> * preview3 * Update dependency Co-authored-by: Harry Pierson <[email protected]> Co-authored-by: Harry Pierson <[email protected]> Co-authored-by: Shargon <[email protected]> Co-authored-by: Luchuan <[email protected]> Co-authored-by: Tommo-L <[email protected]>
Close #1790