-
Notifications
You must be signed in to change notification settings - Fork 202
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
Sc delegation integration #1730
Conversation
iulianpascalau
commented
May 13, 2020
•
edited by andreibancioiu
Loading
edited by andreibancioiu
- Reference latest version of Arwen.
- Reference latest version of delegation contract.
- Adjust gas metering for deployments (less gas used now).
- Fix long tests.
…elrond-go into sc-delegation-integration
…elrond-go into sc-delegation-integration
void bigIntGetCallValue(bigInt destination); | ||
void bigIntGetExternalBalance(byte *address, bigInt result); | ||
void bigIntgetExternalBalance(byte *address, bigInt result); |
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.
Get instead get ?
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.
I checked in Arwen and it is indeed with a capital G
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.
Fixed.
void bigIntGetCallValue(bigInt destination); | ||
void bigIntGetExternalBalance(byte *address, bigInt result); | ||
void bigIntgetExternalBalance(byte *address, bigInt result); |
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.
Get instead get ?
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.
Fixed.
# Conflicts: # genesis/process/genesisBlockCreator_test.go
} | ||
|
||
func (dp *delegationProcessor) sameElements(slice1 [][]byte, slice2 [][]byte) error { | ||
for _, elem1 := range slice1 { |
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 you have sorted here slice1 and slice2 you could have only one for for this check. And also I would add a check for len before any computation if len(slice1) != len(slice2) return err
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.
the len check was done on lines L429:L432. Refactoring...
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.
done, refactored and added unit tests for that area
core/version/versionComparator.go
Outdated
release string | ||
} | ||
|
||
// NewVersionComparator return a new version comparator instance |
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.
returns
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.
fixed
genesis/errors.go
Outdated
@@ -130,3 +130,6 @@ var ErrNilQueryService = errors.New("nil query service") | |||
|
|||
// ErrMissingElement signals a missing element event | |||
var ErrMissingElement = errors.New("missing element") | |||
|
|||
// ErrGetVersionFromSC signals that a coll to "version" function on a contract resulted in an unexpected result |
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.
"call" instead "coll" and "on a smart contract" instead "on a contract"
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.
fixed
cmd/node/config/nodesSetup.json
Outdated
@@ -1,49 +1,517 @@ | |||
{ |
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.
Do we need to change the default values in this file ?
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.
yes, the node, at the current moment can not be started directly (to check the binary works) due to the fact that there is a mismatch between the staked value per node in genesis files (500k) and the economics.toml (2.5 mil)
@@ -1,122 +1,1322 @@ | |||
[ |
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.
Do we need to change the default values in this file ?
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.
yes, the node, at the current moment can not be started directly (to check the binary works) due to the fact that there is a mismatch between the staked value per node in genesis files (500k) and the economics.toml (2.5 mil)
…dded new unit tests.
…Network/elrond-go into sc-delegation-integration
} | ||
] | ||
] |
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.
add empty line to end of file
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.
done
cmd/node/config/nodesSetup.json
Outdated
"metaChainConsensusGroupSize": 3, | ||
"metaChainMinNodes": 3, | ||
"hysteresis": 0.2, | ||
"hysteresis": 0, |
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.
maybe leave it to 0.2 ?
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.
reverted
cmd/node/config/nodesSetup.json
Outdated
@@ -1,49 +1,49 @@ | |||
{ | |||
"startTime": 0, | |||
"roundDuration": 6000, | |||
"roundDuration": 4000, |
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 to 6seconds ?
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.
reverted
cmd/node/config/nodesSetup.json
Outdated
} | ||
] | ||
} | ||
} |
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.
add empty line to end of file
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.
done
ChainID string | ||
SystemSCConfig config.SystemSmartContractsConfig | ||
|
||
//TODO remove this: at genesis time the genesis block creator should not write other shard's data in the same storage manager |
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.
May delete the TODO - this is a must for hardFork when importing your own trie and accounts. As you have to import to the same storage the node will work after hardfork.
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.
moved TODO
"github.com/ElrondNetwork/elrond-go/core" | ||
) | ||
|
||
const numComponents = 3 |
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.
numComponents maybe removed
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.
I will have a magic number as 3
return err | ||
} | ||
|
||
err = dp.executeSetNodePrice(sc) |
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.
maybe rename to something more specific as this works only with Elrond stlye delegation contract - maybe standardDelegationProcess ? :)
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.
You mean the whole file? like standardDelegationProcessor?
} | ||
|
||
func (dp *delegationProcessor) executeSetNumNodes(numNodes int, sc genesis.InitialSmartContractHandler) error { | ||
setNumNodesTxData := fmt.Sprintf("%s@%x", setNumNodesFunction, numNodes) |
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.
maybe use strings and plus, instead of fmt.sprintf.
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.
I prefer to use fmt.Sprintf where I have more than one addition. I also have a %x and this should get uglier
return hex.EncodeToString(node.PubKeyBytes()) | ||
} | ||
|
||
func (dp *delegationProcessor) activate(_ sharding.GenesisNodeInfoHandler) string { |
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.
maybe rename the function name.
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.
getBlsKeySig
} | ||
|
||
return totalDelegated, nil | ||
} | ||
|
||
func (dp *delegationProcessor) setBlsKey(node sharding.GenesisNodeInfoHandler) string { |
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.
getBlsKey
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.
done
…sc-delegation-integration
…Network/elrond-go into sc-delegation-integration
87548eb
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.
Green to merge & release.
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.
System tests passed.