Split interfaces #115

wangdayong228 · 2021-10-11T11:17:04Z

split client interface to seperated smaller interface
use rpc namespace name be sub level same with full-node
remove internal contract related codes, it will readd after abi-gen completed

This change is

use rpc namespace name be sub level same with full-node remove internal contract related codes, it will readd after abi-gen completed

Pana

Reviewed 45 of 45 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @boqiu, @wangdayong228, and @wanliqun)

a discussion (no related file):
Consider this is a big upgrade, it's better to provide a document about the difference with the previous version, And how to migrate from previous version .

cfxclient/client.go, line 5 at r1 (raw file):

// See http://www.gnu.org/licenses/

package cfxclient

better name this package as "confluxclient" or "client", cause this client not only include cfx, also trace and pos

cfxclient/client.go, line 41 at r1 (raw file):

}

func (c *Client) Cfx() interfaces.RpcCfxCaller {

Through a method get the client, and then call it's method seems not friendly to developers. Is it possible to make cfx public ?

cfxclient/client_helper.go, line 20 at r1 (raw file):

// =========Helper==========
// NewAddress create conflux address by base32 string or hex40 string, if base32OrHex is base32 and networkID is passed it will create cfx Address use networkID of current c.rpcCaller.

It's better to move NewAddress to common helper or utils

wanliqun

Fix typo;
Use clean codes;
Please split this PR as small as possible (too big to review).

wanliqun · 2021-10-12T03:16:34Z

accounts/account.go

@@ -347,8 +318,28 @@ func (m *AccountManager) SignTransactionWithPassphrase(tx types.UnsignedTransact
 	return signdTx, nil
 }

-// Sign signs tx by passphrase and returns the signature
-func (m *AccountManager) Sign(tx types.UnsignedTransaction, passphrase string) (v byte, r, s []byte, err error) {
+// SignTransaction signs tx and returns its RLP encoded data.


Comment doesn't match function name.

wanliqun · 2021-10-12T03:19:14Z

accounts/account.go

-// Sign signs tx by passphrase and returns the signature
-func (m *AccountManager) Sign(tx types.UnsignedTransaction, passphrase string) (v byte, r, s []byte, err error) {
+// SignTransaction signs tx and returns its RLP encoded data.
+func (m *KeystoreWallet) SignTransactionAndEncode(tx types.UnsignedTransaction) ([]byte, error) {


Seems this function is not necessary, since the rlp encoding can be left to the user with one line code only.

wanliqun · 2021-10-12T03:19:56Z

accounts/account.go

+// SignTransactionWithPassphraseAndEcode signs tx with given passphrase and return its RLP encoded data.
+func (m *KeystoreWallet) SignTransactionWithPassphraseAndEcode(tx types.UnsignedTransaction, passphrase string) ([]byte, error) {


wanliqun · 2021-10-12T03:27:04Z

cfxclient/client.go

+	// pos    *RpcPosClient
+}
+
+// NewClient creates an instance of Client with specified conflux node url, it will creat account manager if option.KeystorePath not empty.


typo, creat=>create

wanliqun · 2021-10-12T03:42:13Z

cfxclient/client_core.go

+	RequestTimeout time.Duration
+}
+
+// NewClient creates an instance of Client with specified conflux node url, it will creat account manager if option.KeystorePath not empty.


typo creat=>create

NewClientCore?

wanliqun · 2021-10-14T06:48:27Z

cfxclient/client.go

+	client.debug = &RpcDebugClient{&core}
+	client.pubsub = &RpcPubsubClient{&core}
+	client.trace = &RpcTraceClient{&core}
+	// client.pos = &RpcPosClient{&core}


wanliqun · 2021-10-14T06:53:34Z

cfxclient/client_core.go

+	realOption := ClientOption{}
+	// if len(option) > 0 {
+	// 	realOption = option[0]
+	// }


How about using functional options (https://golang.cafe/blog/golang-functional-options-pattern.html) to build client?

wanliqun · 2021-10-14T07:14:53Z

cfxclient/client_core.go

+	realOption := ClientOption{}
+	// if len(option) > 0 {
+	// 	realOption = option[0]
+	// }


How about using functional options pattern to build client?
https://golang.cafe/blog/golang-functional-options-pattern.html

wanliqun · 2021-10-14T07:19:35Z

cfxclient/client_core.go

+			if tmp, ok := args[i].(cfxaddress.Address); ok {
+				tmp.CompleteByNetworkID(client.networkID)
+				args[i] = tmp
+				// fmt.Printf("complete by networkID,%v; after %v\n", client.networkID, args[i])
+			}
+
+			if tmp, ok := args[i].(*cfxaddress.Address); ok {
+				tmp.CompleteByNetworkID(client.networkID)
+				// fmt.Printf("complete by networkID,%v; after %v\n", client.networkID, args[i])
+			}
+
+			if tmp, ok := args[i].(types.CallRequest); ok {
+				tmp.From.CompleteByNetworkID(client.networkID)
+				tmp.To.CompleteByNetworkID(client.networkID)
+				args[i] = tmp
+			}
+
+			if tmp, ok := args[i].(*types.CallRequest); ok {
+				tmp.From.CompleteByNetworkID(client.networkID)
+				tmp.To.CompleteByNetworkID(client.networkID)
+			}


How about using type switch to make the codes more clean?

wanliqun · 2021-10-14T07:20:35Z

cfxclient/client_core.go

+	params := []interface{}{}
+	for i := range args {
+		// fmt.Printf("args %v:%v\n", i, args[i])
+		if !utils.IsNil(args[i]) {


How about use the following to skip embedding?
if utils.IsNil() {
continue
}

Rename package cfxclient to client Simplify setting client option related functions Set config of ClientCore, Client, SignableClient by chain methods

wangdayong228

Reviewable status: 24 of 45 files reviewed, 16 unresolved discussions (waiting on @boqiu, @Pana, and @wanliqun)

a discussion (no related file):

Previously, Pana (W) wrote…

Consider this is a big upgrade, it's better to provide a document about the difference with the previous version, And how to migrate from previous version .

I will document after this version is stable

cfxclient/client.go, line 5 at r1 (raw file):

Previously, Pana (W) wrote…

better name this package as "confluxclient" or "client", cause this client not only include cfx, also trace and pos

Done.

cfxclient/client.go, line 23 at r1 (raw file):

Previously, wanliqun (wanliqun) wrote…

typo, creat=>create

Done.

cfxclient/client.go, line 35 at r1 (raw file):

Previously, wanliqun (wanliqun) wrote…

Add TODO?

Done.

cfxclient/client.go, line 41 at r1 (raw file):

Previously, Pana (W) wrote…

Through a method get the client, and then call it's method seems not friendly to developers. Is it possible to make cfx public ?

Done. Also could directly use client.xxx to call cfx namespace related rpcs

cfxclient/client.go, line 48 at r1 (raw file):

Previously, wanliqun (wanliqun) wrote…

Add TODO?

Done.

cfxclient/client_core.go, line 20 at r1 (raw file):

Previously, wanliqun (wanliqun) wrote…

ClientCore?

Done.

cfxclient/client_core.go, line 43 at r1 (raw file):

Previously, wanliqun (wanliqun) wrote…

NewClientCore?

Done.

cfxclient/client_core.go, line 48 at r1 (raw file):

Previously, wanliqun (wanliqun) wrote…

How about using functional options pattern to build client?
https://golang.cafe/blog/golang-functional-options-pattern.html

Functional options is good, but here becuase there are Client and SignableClient inherited ClientCore, it is not very applicative. Here used chain methods to set options

cfxclient/client_core.go, line 58 at r1 (raw file):

Previously, wanliqun (wanliqun) wrote…

Comment function name does not match real func name

Done.

cfxclient/client_core.go, line 206 at r1 (raw file):

Previously, wanliqun (wanliqun) wrote…

How about use the following to skip embedding?
if utils.IsNil() {
continue
}

Done.

cfxclient/client_core.go, line 229 at r1 (raw file):

Previously, wanliqun (wanliqun) wrote…

How about using type switch to make the codes more clean?

Done.

cfxclient/client_helper.go, line 20 at r1 (raw file):

Previously, Pana (W) wrote…

It's better to move NewAddress to common helper or utils

Done.

accounts/account.go, line 321 at r1 (raw file):

Previously, wanliqun (wanliqun) wrote…

Comment doesn't match function name.

Done.

accounts/account.go, line 322 at r1 (raw file):

Previously, wanliqun (wanliqun) wrote…

Seems this function is not necessary, since the rlp encoding can be left to the user with one line code only.

Done.

accounts/account.go, line 332 at r1 (raw file):

Previously, wanliqun (wanliqun) wrote…

ditto

Done.

Pana

Reviewed 1 of 21 files at r2, all commit messages.
Reviewable status: 25 of 45 files reviewed, 12 unresolved discussions (waiting on @boqiu, @Pana, and @wanliqun)

wanliqun

Reviewed 20 of 21 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @boqiu and @wangdayong228)

cfxclient/rpc_batch.go, line 54 at r2 (raw file):

 reflect.DeepEqual(be.Result, &types.BlockSummary{}

Should be reflect.DeepEqual(be.Result, &types.Transaction{})? Better do some testing here.

cfxclient/signalbe_client.go, line 27 at r2 (raw file):

//FixMe: Is there better name
func NewSignalbeClientByPath(nodeURL string, keyStorePath string) (SignableClient, error) {

NewSignableClientByPath?

wangdayong228

Reviewable status: 42 of 45 files reviewed, 2 unresolved discussions (waiting on @boqiu and @wanliqun)

cfxclient/signalbe_client.go, line 27 at r2 (raw file):

Previously, wanliqun (wanliqun) wrote…

NewSignableClientByPath?

Done.

cfxclient/rpc_batch.go, line 54 at r2 (raw file):

Previously, wanliqun (wanliqun) wrote…

 reflect.DeepEqual(be.Result, &types.BlockSummary{}
Should be reflect.DeepEqual(be.Result, &types.Transaction{})? Better do some testing here.

Removed batch functions and will be replaced by the bulk caller

dayong added 2 commits October 11, 2021 18:45

split client interface to seperated smaller interface

a0605cd

use rpc namespace name be sub level same with full-node remove internal contract related codes, it will readd after abi-gen completed

split client helper from client

704b505

wangdayong228 requested review from boqiu, Pana and wanliqun October 11, 2021 11:18

Pana requested changes Oct 12, 2021

View reviewed changes

wanliqun requested changes Oct 14, 2021

View reviewed changes

dayong added 2 commits October 15, 2021 17:22

Remove Encode related functions in KeystoreWallet

58a574f

Rename package cfxclient to client Simplify setting client option related functions Set config of ClientCore, Client, SignableClient by chain methods

clean code

3b76b72

wangdayong228 commented Oct 15, 2021

View reviewed changes

Pana reviewed Oct 18, 2021

View reviewed changes

wanliqun requested changes Oct 25, 2021

View reviewed changes

dayong added 2 commits November 10, 2021 10:59

remove rpc_batch

f48368c

Add method MustNewClient for conveniently new client

7b50e7e

wangdayong228 commented Nov 10, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split interfaces #115

Split interfaces #115

wangdayong228 commented Oct 11, 2021 •

edited by boqiu

Loading

Pana left a comment

wanliqun left a comment

wanliqun Oct 12, 2021

wanliqun Oct 12, 2021

wanliqun Oct 12, 2021

wanliqun Oct 12, 2021

wanliqun Oct 12, 2021

wanliqun Oct 14, 2021

wanliqun Oct 14, 2021

wanliqun Oct 14, 2021

wanliqun Oct 14, 2021

wanliqun Oct 14, 2021

wanliqun Oct 14, 2021

wangdayong228 left a comment

Pana left a comment

wanliqun left a comment

wangdayong228 left a comment

		// SignTransactionWithPassphraseAndEcode signs tx with given passphrase and return its RLP encoded data.
		func (m *KeystoreWallet) SignTransactionWithPassphraseAndEcode(tx types.UnsignedTransaction, passphrase string) ([]byte, error) {

Split interfaces #115

Are you sure you want to change the base?

Split interfaces #115

Conversation

wangdayong228 commented Oct 11, 2021 • edited by boqiu Loading

Pana left a comment

Choose a reason for hiding this comment

wanliqun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangdayong228 left a comment

Choose a reason for hiding this comment

Pana left a comment

Choose a reason for hiding this comment

wanliqun left a comment

Choose a reason for hiding this comment

wangdayong228 left a comment

Choose a reason for hiding this comment

wangdayong228 commented Oct 11, 2021 •

edited by boqiu

Loading