Growth supporting API routes #2961

iulianpascalau · 2021-04-06T13:58:38Z

added growth related API routes that can expose the staked addresses and their amounts and the delegators addresses and their amounts on each delegation system SC

Testing scenarios: start a testnet and then call these routes on the metachain observer:

http://x.x.x.x:8080/network/direct-staked-info
http://x.x.x.x:8080/network/delegated-info

the responses should contain all delegators and their delegated amount + the direct staked info. If no stake/unstake operation will be done, the values should match the genesis.json contents.

…s and their ammounts and the delegators addresses and their amounts on each delegation system sc

codecov · 2021-04-06T14:06:47Z

Codecov Report

Merging #2961 (faea4fa) into master (95e198a) will increase coverage by 0.03%.
The diff coverage is 88.81%.

❗ Current head faea4fa differs from pull request most recent head 5864fef. Consider uploading reports for the commit 5864fef to get more accurate results

@@            Coverage Diff             @@
##           master    #2961      +/-   ##
==========================================
+ Coverage   74.97%   75.01%   +0.03%     
==========================================
  Files         615      620       +5     
  Lines       58857    59055     +198     
==========================================
+ Hits        44130    44298     +168     
- Misses      10789    10803      +14     
- Partials     3938     3954      +16

Impacted Files	Coverage Δ
node/trieIterators/directStakedListProcessor.go	`70.00% <70.00%> (ø)`
node/trieIterators/delegatedListProcessor.go	`78.49% <78.49%> (ø)`
api/network/routes.go	`100.00% <100.00%> (ø)`
facade/nodeFacade.go	`43.16% <100.00%> (+1.18%)`	⬆️
node/external/nodeApiResolver.go	`96.29% <100.00%> (+6.29%)`	⬆️
node/trieIterators/commonStakingProcessor.go	`100.00% <100.00%> (ø)`
...ieIterators/factory/delegatedListHandlerFactory.go	`100.00% <100.00%> (ø)`
...terators/factory/directStakedListHandlerFactory.go	`100.00% <100.00%> (ø)`
...trieIterators/factory/stakeValuesHandlerFactory.go	`100.00% <100.00%> (ø)`
node/trieIterators/stakeValuesProcessor.go	`100.00% <100.00%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95e198a...5864fef. Read the comment docs.

bogdan-rosianu · 2021-04-06T14:10:32Z

api/mock/facade.go

@@ -188,7 +190,7 @@ func (f *Facade) SendBulkTransactions(txs []*transaction.Transaction) (uint64, e
 	return f.SendBulkTransactionsHandler(txs)
 }

-//ValidateTransaction --
+//ValidateTransaction -


// ValidateTransaction -

bogdan-rosianu · 2021-04-06T14:57:09Z

cmd/node/config/api.toml

-        { Name = "/config", Open = true }
+        { Name = "/config", Open = true },
+
+        # /network/direct-staked-info will return the metrics related to current direct staked list of addresses


# /network/direct-staked-info will return a list containing a list of current direct staked list of addresses # and their staked values

bogdan-rosianu · 2021-04-06T14:57:37Z

cmd/node/config/api.toml

+        # and their staked values
+        {Name = "/direct-staked-info", Open = true},
+
+        # /network/delegated-info will return the metrics related to current delegated list of addresses


same here. they are not "metrics" as we "know" them

bogdan-rosianu · 2021-04-06T14:58:21Z

data/api/apiBlock.go

+
+// DirectStakedValue holds the total staked value for an address
+type DirectStakedValue struct {
+	Address []byte   `json:"address"`


addresses: string and encode with bech32
values: string instead of big int

applicable in the entire file

bogdan-rosianu · 2021-04-06T15:01:49Z

node/external/nodeApiResolver.go

@@ -64,6 +79,16 @@ func (nar *NodeApiResolver) GetTotalStakedValue() (*api.StakeValues, error) {
 	return nar.totalStakedValueHandler.GetTotalStakedValue()
 }

+// GetDirectStakedList will output the list for the direct staked addresses


"will return". also below

bogdan-rosianu · 2021-04-06T15:05:47Z

node/trieIterators/delegatedListProcessor.go

+	}
+
+	if len(vmOutput.ReturnData) != 1 {
+		return nil, fmt.Errorf("%w, getActiveFund function should have at least one value", epochStart.ErrExecutingSystemScCode)


remove at least ? from the condition it seems that there should be a single value

bogdan-rosianu · 2021-04-06T15:07:32Z

node/trieIterators/delegatedListProcessor.go

+		keys = append(keys, key)
+	}
+
+	sort.Slice(keys, func(i, j int) bool {


why do you need to sort the elements in the slice? you use them as keys in a map.
Am I missing something?

Just wanted to be consistent between calls (same call will produce exactly the same output for the same data). Might be dropped.

bogdan-rosianu · 2021-04-06T15:09:47Z

node/trieIterators/errors.go

+// ErrNodeNotInitialized signals that the node is not initialized and can not compute the required task yet
+var ErrNodeNotInitialized = errors.New("the node is not fully initialized")
+
+// ErrNilMutex signals that a nil mutex hax been provided


bogdan-rosianu · 2021-04-06T15:10:36Z

node/trieIterators/factory/stakeValuesHandlerFactory.go

+	"github.com/ElrondNetwork/elrond-go/process"
+)
+
+// ArgStakeProcessors is struct that contains components that are needed to create a TotalStakedValueHandler


is a struct

bogdan-rosianu · 2021-04-06T15:11:49Z

node/trieIterators/stakeValuesProcessor.go

+	"github.com/ElrondNetwork/elrond-go/vm"
+)
+
+// AccountsWrapper extends the AccountsAdapter interfec


cmd/node/config/api.toml

bogdan-rosianu · 2021-04-06T15:26:53Z

node/trieIterators/delegatedListProcessor.go

+		return nil, err
+	}
+	if vmOutput.ReturnCode != vmcommon.Ok {
+		return nil, fmt.Errorf("%w, error: %v message: %s", epochStart.ErrExecutingSystemScCode, vmOutput.ReturnCode, vmOutput.ReturnMessage)


"%w, return code: %v message: %s" ?

bogdan-rosianu · 2021-04-06T15:28:02Z

node/trieIterators/delegatedListProcessor.go

+		return nil, err
+	}
+	if vmOutput.ReturnCode != vmcommon.Ok {
+		return nil, fmt.Errorf("%w, error: %v message: %s", epochStart.ErrExecutingSystemScCode, vmOutput.ReturnCode, vmOutput.ReturnMessage)


"%w, return code: %v message: %s" ?

bogdan-rosianu · 2021-04-06T15:28:56Z

node/trieIterators/commonStakingProcessor.go

+		return nil, nil, err
+	}
+	if vmOutput.ReturnCode != vmcommon.Ok {
+		return nil, nil, fmt.Errorf("%w, error: %v message: %s", epochStart.ErrExecutingSystemScCode, vmOutput.ReturnCode, vmOutput.ReturnMessage)


"%w, return code: %v message: %s" ?

bogdan-rosianu · 2021-04-06T15:32:56Z

node/trieIterators/stakeValuesProcessor.go

+	}
+
+	ctx := context.Background()
+	chLeaves, err := validatorAccount.DataTrie().GetAllLeavesOnChannel(rootHash, ctx)


can you add a TODO for analyzing if we can add a GetAllLeavesKeysOnChannel? Here and also in other parts of the code, we only use the keys, so no need to move high amount of data when we only need the keys

bogdan-rosianu · 2021-04-06T15:34:03Z

node/trieIterators/delegatedListProcessor.go

+
+type delegatedListProc struct {
+	*commonStakingProcessor
+	publicKeyConverter core.PubkeyConverter


oh.. a pub key converter. you might need it 😈

bogdan-rosianu · 2021-04-06T15:36:38Z

node/trieIterators/directStakedListProcessor.go

+	publicKeyConverter core.PubkeyConverter
+}
+
+var metachainIdentifier, _ = hex.DecodeString("ff")


why not declaring a const and set it with the bytes value?

bogdan-rosianu · 2021-04-06T15:37:33Z

node/trieIterators/stakeValuesProcessor_test.go

+	require.Equal(t, expectedErr, err)
+}
+
+func TestTotalStakedValueProcessor_GetTotalStakedValueilHeaderShouldError(t *testing.T) {


NilHeader ?

bogdan-rosianu · 2021-04-06T15:38:42Z

node/trieIterators/stakeValuesProcessor_test.go

@@ -0,0 +1,379 @@
+package trieIterators


following go principles, package names should not use camel case

I guess we should start refactoring half of the project then. I know it should have been one word there but I guess those 2 words better explain what the package should do.

it could have been trieiterators but it's fine

miiu96 · 2021-04-06T14:19:51Z

data/api/apiBlock.go

+
+// DirectStakedValue holds the total staked value for an address
+type DirectStakedValue struct {
+	Address []byte   `json:"address"`


address as type string and bech32 encoded?

miiu96 · 2021-04-06T14:21:24Z

data/api/apiBlock.go

+// Delegator holds the delegator address and the slice of delegated values
+type Delegator struct {
+	DelegatorAddress []byte           `json:"delegatorAddress"`
+	DelegatedTo      []DelegatedValue `json:"delegatedTo"`


Maybe use a pointer here?

miiu96 · 2021-04-06T14:24:23Z

data/api/apiBlock.go

+	Address []byte   `json:"address"`
+	Staked  *big.Int `json:"staked"`
+	TopUp   *big.Int `json:"topUp"`
+	Total   *big.Int `json:"total"`


also, return string on the API response instead of big.Int

… growth-releated-api-routes

node/trieIterators/directStakedListProcessor.go

miiu96 · 2021-04-08T19:25:23Z

node/trieIterators/delegatedListProcessor.go

+		delegatorInfo.Total = delegatorInfo.TotalAsBigInt.String()
+		delegatorInfo.DelegatedTo = append(delegatorInfo.DelegatedTo, &api.DelegatedValue{
+			DelegationScAddress: dlp.publicKeyConverter.Encode(delegationSC),
+			Value:               big.NewInt(0).Set(value).String(),


why you use big.NewInt(0).Set(value).String() instead value.String() ?

Remained from first implementation. Fixing

gabi-vuls

System test passed.

- added growth related API routes that can expose the staked addresse…

4a1fb40

…s and their ammounts and the delegators addresses and their amounts on each delegation system sc

bogdan-rosianu self-requested a review April 6, 2021 14:07

miiu96 self-requested a review April 6, 2021 14:17

bogdan-rosianu requested changes Apr 6, 2021

View reviewed changes

miiu96 reviewed Apr 7, 2021

View reviewed changes

iulianpascalau and others added 5 commits April 8, 2021 15:33

- fixes after review

08c4c32

- minor refactoring, added unit tests

29eeb07

Merge branch 'master' into growth-releated-api-routes

010b3fb

- added more unit tests

05a2116

Merge remote-tracking branch 'origin/growth-releated-api-routes' into…

134fc32

… growth-releated-api-routes

bogdan-rosianu requested changes Apr 8, 2021

View reviewed changes

node/trieIterators/directStakedListProcessor.go Show resolved Hide resolved

miiu96 reviewed Apr 8, 2021

View reviewed changes

- fixed an unnecessary big int set

5ae87e2

bogdan-rosianu approved these changes Apr 9, 2021

View reviewed changes

miiu96 approved these changes Apr 9, 2021

View reviewed changes

gabi-vuls approved these changes Apr 9, 2021

View reviewed changes

Merge branch 'master' into growth-releated-api-routes

5864fef

iulianpascalau merged commit daf18b8 into master Apr 9, 2021

iulianpascalau deleted the growth-releated-api-routes branch April 9, 2021 14:54

Growth supporting API routes #2961

Growth supporting API routes #2961

Conversation

iulianpascalau commented Apr 6, 2021 • edited

codecov bot commented Apr 6, 2021 • edited

Codecov Report

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

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

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

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

gabi-vuls left a comment

Choose a reason for hiding this comment

iulianpascalau commented Apr 6, 2021 •

edited

codecov bot commented Apr 6, 2021 •

edited