From 13bccaa9c66d6966745760c05f1018fcf4895d74 Mon Sep 17 00:00:00 2001 From: vsmk98 Date: Tue, 12 Feb 2019 09:54:13 +0800 Subject: [PATCH] code changes to fix review comments --- controls/cluster/cluster.go | 14 +++---- controls/permission/permission.go | 14 +++---- core/quorum/api.go | 69 ++++++++++++++++--------------- core/tx_pool.go | 13 +----- core/types/permissions_cache.go | 7 +--- eth/api_backend.go | 2 - eth/bind.go | 4 +- ethclient/ethclient.go | 2 - 8 files changed, 54 insertions(+), 71 deletions(-) diff --git a/controls/cluster/cluster.go b/controls/cluster/cluster.go index c82bc72fb..d702e849d 100644 --- a/controls/cluster/cluster.go +++ b/controls/cluster/cluster.go @@ -4,14 +4,14 @@ import ( "crypto/ecdsa" "math/big" - "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/accounts/abi/bind" - "github.com/ethereum/go-ethereum/params" + "github.com/ethereum/go-ethereum/controls" + pbind "github.com/ethereum/go-ethereum/controls/bind/cluster" + "github.com/ethereum/go-ethereum/core/types" "github.com/ethereum/go-ethereum/ethclient" "github.com/ethereum/go-ethereum/log" "github.com/ethereum/go-ethereum/node" - "github.com/ethereum/go-ethereum/controls" - pbind "github.com/ethereum/go-ethereum/controls/bind/cluster" + "github.com/ethereum/go-ethereum/params" ) type OrgKeyCtrl struct { @@ -46,14 +46,14 @@ func (k *OrgKeyCtrl) Start() error { return nil } err = k.checkIfContractExists() - if (err != nil ){ + if err != nil { return err } k.manageClusterKeys() return nil } -func(k *OrgKeyCtrl) checkIfContractExists() error { +func (k *OrgKeyCtrl) checkIfContractExists() error { auth := bind.NewKeyedTransactor(k.key) clusterSession := &pbind.ClusterSession{ Contract: k.km, @@ -110,7 +110,7 @@ func (k *OrgKeyCtrl) populatePrivateKeys() error { } opts = &bind.FilterOpts{} - pastDeleteEvents, err := cluster.FilterOrgKeyDeleted(opts) + pastDeleteEvents, _ := cluster.FilterOrgKeyDeleted(opts) recExists = true for recExists { diff --git a/controls/permission/permission.go b/controls/permission/permission.go index 99d03bda0..2e5943786 100644 --- a/controls/permission/permission.go +++ b/controls/permission/permission.go @@ -97,6 +97,9 @@ func (p *PermissionCtrl) init() error { return err } + // set the default access to ReadOnly + types.SetDefaultAccess() + // call populates the node details from contract to KnownNodes // this is not required as the permissioned node info is persisted at // file level @@ -220,7 +223,7 @@ func (p *PermissionCtrl) updatePermissionedNodes(enodeId, ipAddrPort, discPort, path := filepath.Join(p.dataDir, PERMISSIONED_CONFIG) if _, err := os.Stat(path); err != nil { log.Error("Read Error for permissioned-nodes.json file. This is because 'permissioned' flag is specified but no permissioned-nodes.json file is present.", "err", err) - return + return } // Load the nodes from the config file blob, err := ioutil.ReadFile(path) @@ -232,7 +235,7 @@ func (p *PermissionCtrl) updatePermissionedNodes(enodeId, ipAddrPort, discPort, nodelist := []string{} if err := json.Unmarshal(blob, &nodelist); err != nil { log.Error("updatePermissionedNodes: Failed to load nodes list", "err", err) - return + return } newEnodeId := p.formatEnodeId(enodeId, ipAddrPort, discPort, raftPort) @@ -261,7 +264,7 @@ func (p *PermissionCtrl) updatePermissionedNodes(enodeId, ipAddrPort, discPort, blob, _ = json.Marshal(nodelist) mu.Lock() - if err:= ioutil.WriteFile(path, blob, 0644); err!= nil{ + if err := ioutil.WriteFile(path, blob, 0644); err != nil { log.Error("updatePermissionedNodes: Error writing new node info to file", "err", err) } mu.Unlock() @@ -446,7 +449,7 @@ func (p *PermissionCtrl) populateInitPermission() error { tx, err := permissionsSession.GetNetworkBootStatus() if err != nil { // handle the scenario of no contract code. - if err.Error() == "no contract code at given address"{ + if err.Error() == "no contract code at given address" { return err } log.Warn("Failed to retrieve network boot status ", "err", err) @@ -482,15 +485,12 @@ func (p *PermissionCtrl) populateInitPermission() error { return err } - // update network status to boot completed err = p.updateNetworkStatus(permissionsSession) if err != nil { return err } - // set the default access to ReadOnly - types.SetDefaultAccess() } return nil } diff --git a/core/quorum/api.go b/core/quorum/api.go index 8d2d5ba85..bd0b5cb90 100644 --- a/core/quorum/api.go +++ b/core/quorum/api.go @@ -2,8 +2,8 @@ package quorum import ( "crypto/ecdsa" - "fmt" "errors" + "fmt" "math/big" "strings" @@ -75,6 +75,7 @@ const ( Active VoterAccessType = iota Inactive ) + // QuorumControlsAPI provides an API to access Quorum's node permission and org key management related services type QuorumControlsAPI struct { txPool *core.TxPool @@ -148,8 +149,8 @@ var ( ErrKeyInUse = ExecStatus{false, "Key already in use in another master organization"} ErrKeyNotFound = ExecStatus{false, "Key not found for the organization"} ErrNothingToApprove = ExecStatus{false, "Nothing to approve"} - ErrNothingToCancel = ExecStatus{false, "Nothing to cancel"} - ErrNodeProposed = ExecStatus{false, "Node already proposed for the action"} + ErrNothingToCancel = ExecStatus{false, "Nothing to cancel"} + ErrNodeProposed = ExecStatus{false, "Node already proposed for the action"} ErrAccountIsNotVoter = ExecStatus{false, "Not a voter account"} ErrBlacklistedNode = ExecStatus{false, "Blacklisted node. Operation not allowed"} ErrOpNotAllowed = ExecStatus{false, "Operation not allowed"} @@ -314,7 +315,7 @@ func (s *QuorumControlsAPI) VoterList() []string { if err != nil { log.Error("error getting voter info", "err", err) } else { - if voter.VoterStatus == uint8(Active){ + if voter.VoterStatus == uint8(Active) { voterArr = append(voterArr, voter.Addr.String()) } } @@ -456,13 +457,12 @@ func (s *QuorumControlsAPI) SetAccountAccess(acct common.Address, access uint8, return s.executePermAction(SetAccountAccess, txArgs{acctId: acct, accessType: access, txa: txa}) } - -func getNodeDetailsFromEnode(nodeId string) (string, string, string, string, error){ +func getNodeDetailsFromEnode(nodeId string) (string, string, string, string, error) { node, err := discover.ParseNode(nodeId) if err != nil { log.Error("invalid node id: %v", err) return "", "", "", "", err - } + } enodeID := node.ID.String() ipAddr := node.IP.String() port := fmt.Sprintf("%v", node.TCP) @@ -475,7 +475,7 @@ func getNodeDetailsFromEnode(nodeId string) (string, string, string, string, err // checks if the input node details for approval is matching with details stored // in contract -func checkNodeDetails(ps *pbind.PermissionsSession, nodeId string, action PermAction) (error, ExecStatus){ +func checkNodeDetails(ps *pbind.PermissionsSession, nodeId string, action PermAction) (error, ExecStatus) { enodeID, discPort, raftPort, ipAddrPort, err := getNodeDetailsFromEnode(nodeId) cnode, err := ps.GetNodeDetails(enodeID) @@ -492,11 +492,11 @@ func checkNodeDetails(ps *pbind.PermissionsSession, nodeId string, action PermAc return errors.New("operation cannot be performed"), ErrOpNotAllowed } - newNode := false; - if nodeStatus == "NotInNetwork" && len(cnode.IpAddrPort) == 0{ + newNode := false + if nodeStatus == "NotInNetwork" && len(cnode.IpAddrPort) == 0 { newNode = true } - detailsMatch := false; + detailsMatch := false if strings.Compare(ipAddrPort, cnode.IpAddrPort) == 0 && strings.Compare(discPort, cnode.DiscPort) == 0 && strings.Compare(raftPort, cnode.RaftPort) == 0 { detailsMatch = true } @@ -512,9 +512,9 @@ func checkNodeDetails(ps *pbind.PermissionsSession, nodeId string, action PermAc } // if propose action, check if node status allows the operation - if ((action == ProposeNode && nodeStatus != "NotInNetwork") || + if (action == ProposeNode && nodeStatus != "NotInNetwork") || (action == ProposeNodeDeactivation && nodeStatus != "Approved") || - (action == ProposeNodeActivation && nodeStatus != "Deactivated")) { + (action == ProposeNodeActivation && nodeStatus != "Deactivated") { return errors.New("operation cannot be performed"), ErrOpNotAllowed } @@ -526,9 +526,9 @@ func checkNodeDetails(ps *pbind.PermissionsSession, nodeId string, action PermAc return errors.New("Nothing to approve"), ErrNothingToApprove } - if (action == CancelPendingOperation && nodeStatus != "PendingApproval" && + if action == CancelPendingOperation && nodeStatus != "PendingApproval" && nodeStatus != "PendingDeactivation" && nodeStatus != "PendingActivation" && - nodeStatus != "PendingBlacklisting") { + nodeStatus != "PendingBlacklisting" { return errors.New("Nothing to cancel"), ErrNothingToCancel } @@ -539,12 +539,12 @@ func checkNodeDetails(ps *pbind.PermissionsSession, nodeId string, action PermAc return errors.New("Node already proposed"), ErrNodeProposed } - } + } return nil, ExecSuccess } -func(s *QuorumControlsAPI) validateOpDetails(ps *pbind.PermissionsSession, enodeID string, from common.Address, action PermAction) (error, ExecStatus) { +func (s *QuorumControlsAPI) validateOpDetails(ps *pbind.PermissionsSession, enodeID string, from common.Address, action PermAction) (error, ExecStatus) { // check if the input node is fine err, execStatus := checkNodeDetails(ps, enodeID, action) @@ -553,20 +553,21 @@ func(s *QuorumControlsAPI) validateOpDetails(ps *pbind.PermissionsSession, enode } // if action is propose type then check if voter nodes are there in the network - if (action == ProposeNode || action == ProposeNodeDeactivation || action == ProposeNodeActivation || action == ProposeNodeBlacklisting){ - if !checkVoterExists(ps){ + if action == ProposeNode || action == ProposeNodeDeactivation || action == ProposeNodeActivation || action == ProposeNodeBlacklisting { + if !checkVoterExists(ps) { return errors.New("No voter account"), ErrNoVoterAccount } } // if approval process, check if the account is a voter account - if (action == ApproveNode || action == ApproveNodeDeactivation || action == ApproveNodeActivation || action == ApproveNodeBlacklisting || action == CancelPendingOperation){ + if action == ApproveNode || action == ApproveNodeDeactivation || action == ApproveNodeActivation || action == ApproveNodeBlacklisting || action == CancelPendingOperation { if !checkIsVoter(ps, from) { return errors.New("Not a voter account"), ErrAccountNotAVoter } } return nil, ExecSuccess } + // executePermAction helps to execute an action in permission contract func (s *QuorumControlsAPI) executePermAction(action PermAction, args txArgs) ExecStatus { @@ -585,7 +586,7 @@ func (s *QuorumControlsAPI) executePermAction(action PermAction, args txArgs) Ex var node *discover.Node var execStatus ExecStatus - if action != SetAccountAccess && action != AddVoter && action != RemoveVoter { + if action != SetAccountAccess && action != AddVoter && action != RemoveVoter { err, execStatus = s.validateOpDetails(ps, args.nodeId, args.txa.From, action) if err != nil { return execStatus @@ -594,19 +595,19 @@ func (s *QuorumControlsAPI) executePermAction(action PermAction, args txArgs) Ex switch action { case AddVoter: - if locErr, execStatus := valAccountAccessVoter(args.txa.From, args.voter); locErr != nil{ + if locErr, execStatus := valAccountAccessVoter(args.txa.From, args.voter); locErr != nil { return execStatus } - if checkIsVoter(ps, args.voter){ + if checkIsVoter(ps, args.voter) { return ErrVoterExists } tx, err = ps.AddVoter(args.voter) case RemoveVoter: - if locErr, execStatus := valAccountAccessVoter(args.txa.From, common.Address{}); locErr != nil{ + if locErr, execStatus := valAccountAccessVoter(args.txa.From, common.Address{}); locErr != nil { return execStatus } - if !checkIsVoter(ps, args.voter){ + if !checkIsVoter(ps, args.voter) { return ErrAccountIsNotVoter } tx, err = ps.RemoveVoter(args.voter) @@ -616,7 +617,7 @@ func (s *QuorumControlsAPI) executePermAction(action PermAction, args txArgs) Ex if locerr != nil { log.Error("invalid node id: %v", err) return ErrInvalidNode - } + } tx, err = ps.ProposeNode(enodeID, ipAddrPort, discPort, raftPort) @@ -686,7 +687,7 @@ func (s *QuorumControlsAPI) executePermAction(action PermAction, args txArgs) Ex tx, err = ps.BlacklistNode(enodeID) case SetAccountAccess: - if (args.accessType > 3){ + if args.accessType > 3 { return ErrInvalidAccountAccess } if locErr, execStatus := validateAccoutOp(ps, args.txa.From, args.acctId, args.accessType); locErr != nil { @@ -851,28 +852,28 @@ func (s *QuorumControlsAPI) executeOrgKeyAction(action OrgKeyAction, args txArgs tx, err = ps.AddSubOrg(args.orgId, args.morgId) case AddOrgVoter: - if locErr, execStatus := valAccountAccessVoter(args.txa.From, args.acctId); locErr != nil{ + if locErr, execStatus := valAccountAccessVoter(args.txa.From, args.acctId); locErr != nil { return execStatus } ret, _ := ps.CheckMasterOrgExists(args.morgId) if !ret { return ErrInvalidMasterOrg } - ret, _, _ = ps.CheckIfVoterExists(args.morgId, args.acctId) + ret, _ = ps.CheckIfVoterExists(args.morgId, args.acctId) if ret { return ErrVoterExists } tx, err = ps.AddVoter(args.morgId, args.acctId) case RemoveOrgVoter: - if locErr, execStatus := valAccountAccessVoter(args.txa.From, common.Address{}); locErr != nil{ + if locErr, execStatus := valAccountAccessVoter(args.txa.From, common.Address{}); locErr != nil { return execStatus } ret, _ := ps.CheckMasterOrgExists(args.morgId) if !ret { return ErrInvalidMasterOrg } - ret, _, _ = ps.CheckIfVoterExists(args.morgId, args.acctId) + ret, _ = ps.CheckIfVoterExists(args.morgId, args.acctId) if !ret { return ErrInvalidAccount } @@ -1047,12 +1048,12 @@ func validateAccoutOp(ps *pbind.PermissionsSession, from, targetAcct common.Addr retVal = true } else if fromAcctAccess == types.Transact && (accessType == uint8(types.Transact) || accessType == uint8(types.ReadOnly)) { retVal = true - } + } if retVal && fromAcctAccess != types.FullAccess { - if ((fromAcctAccess == types.ContractDeploy && targetAcctAccess == types.FullAccess) || + if (fromAcctAccess == types.ContractDeploy && targetAcctAccess == types.FullAccess) || (fromAcctAccess == types.Transact && - (targetAcctAccess == types.ContractDeploy || targetAcctAccess == types.FullAccess))){ + (targetAcctAccess == types.ContractDeploy || targetAcctAccess == types.FullAccess)) { retVal = false } diff --git a/core/tx_pool.go b/core/tx_pool.go index 1a23f69b4..7d317d301 100644 --- a/core/tx_pool.go +++ b/core/tx_pool.go @@ -1288,23 +1288,14 @@ func checkAccount(fromAcct common.Address, toAcct *common.Address) error { return nil case types.ReadOnly: - err := errors.New("Account Does not have transaction permissions") - return err + return errors.New("Account does not have transaction permissions") case types.Transact: if toAcct == nil { - err := errors.New("Account Does not have contract create permissions") - return err + return errors.New("Account does not have contract create permissions") } else { return nil } - // case types.ContractDeploy: - // if toAcct != nil { - // err := errors.New("Account Does not have transacte permissions") - // return err - // } else { - // return nil - // } } return nil } diff --git a/core/types/permissions_cache.go b/core/types/permissions_cache.go index fa06929dc..9aca0f101 100644 --- a/core/types/permissions_cache.go +++ b/core/types/permissions_cache.go @@ -12,7 +12,7 @@ const ( ReadOnly AccessType = iota Transact ContractDeploy - FullAccess + FullAccess ) type PermStruct struct { @@ -50,11 +50,6 @@ func GetAcctAccess(acctId common.Address) AccessType { } } return DefaultAccess - // if AcctMap.Len() == 0 { - // return FullAccess - // } else { - // return ReadOnly - // } } func AddOrgKey(orgId string, key string) { diff --git a/eth/api_backend.go b/eth/api_backend.go index 930c1d08e..67b9fb83b 100644 --- a/eth/api_backend.go +++ b/eth/api_backend.go @@ -35,7 +35,6 @@ import ( "github.com/ethereum/go-ethereum/event" "github.com/ethereum/go-ethereum/params" "github.com/ethereum/go-ethereum/rpc" - "github.com/ethereum/go-ethereum/log" ) // EthAPIBackend implements ethapi.Backend for full nodes @@ -186,7 +185,6 @@ func (b *EthAPIBackend) SubscribeLogsEvent(ch chan<- []*types.Log) event.Subscri } func (b *EthAPIBackend) SendTx(ctx context.Context, signedTx *types.Transaction) error { - log.Info("inside SendTx") return b.eth.txPool.AddLocal(signedTx) } diff --git a/eth/bind.go b/eth/bind.go index e55aba060..858aa3860 100644 --- a/eth/bind.go +++ b/eth/bind.go @@ -116,8 +116,8 @@ func (b *ContractBackend) PendingNonceAt(ctx context.Context, account common.Add // SuggestGasPrice implements bind.ContractTransactor retrieving the currently // suggested gas price to allow a timely execution of a transaction. func (b *ContractBackend) SuggestGasPrice(ctx context.Context) (*big.Int, error) { - price, error := b.eapi.GasPrice(ctx) - return price.ToInt(), error + price, err := b.eapi.GasPrice(ctx) + return price.ToInt(), err } // EstimateGasLimit implements bind.ContractTransactor triing to estimate the gas diff --git a/ethclient/ethclient.go b/ethclient/ethclient.go index 9b9e20e0a..49c5e8cfd 100644 --- a/ethclient/ethclient.go +++ b/ethclient/ethclient.go @@ -31,7 +31,6 @@ import ( "github.com/ethereum/go-ethereum/private" "github.com/ethereum/go-ethereum/rlp" "github.com/ethereum/go-ethereum/rpc" - "github.com/ethereum/go-ethereum/log" ) // Client defines typed wrappers for the Ethereum RPC API. @@ -485,7 +484,6 @@ func (ec *Client) EstimateGas(ctx context.Context, msg ethereum.CallMsg) (uint64 // If the transaction was a contract creation use the TransactionReceipt method to get the // contract address after the transaction has been mined. func (ec *Client) SendTransaction(ctx context.Context, tx *types.Transaction) error { - log.Info("Inside SendTransaction") data, err := rlp.EncodeToBytes(tx) if err != nil { return err