[rosetta] implement balance tracking and redo tx construction #8729

fdymylja · 2021-03-01T10:47:08Z

Description

This PR fully implements balance tracking via data API using balance tracking events.

On top of that, now construction supports out of the box any message in any possible application that passes through the /tx endpoint without requiring any message extension.

closes: #8705

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

… type

codecov · 2021-03-05T10:10:40Z

Codecov Report

Merging #8729 (3e47bb4) into master (8f7cdaf) will decrease coverage by 0.28%.
The diff coverage is 32.70%.

@@            Coverage Diff             @@
##           master    #8729      +/-   ##
==========================================
- Coverage   59.44%   59.15%   -0.29%     
==========================================
  Files         563      570       +7     
  Lines       31286    31764     +478     
==========================================
+ Hits        18598    18791     +193     
- Misses      10522    10774     +252     
- Partials     2166     2199      +33

Impacted Files	Coverage Δ
crypto/keyring/keyring.go	`59.29% <0.00%> (-0.18%)`	⬇️
server/rosetta/client_offline.go	`0.00% <0.00%> (ø)`
server/rosetta/client_online.go	`0.00% <0.00%> (ø)`
server/rosetta/types.go	`0.00% <0.00%> (ø)`
server/rosetta/util.go	`0.00% <0.00%> (ø)`
types/codec.go	`50.00% <0.00%> (ø)`
x/bank/types/msgs.go	`96.87% <ø> (+42.96%)`	⬆️
x/distribution/types/msg.go	`47.54% <ø> (+15.67%)`	⬆️
x/staking/types/msg.go	`54.92% <ø> (+31.00%)`	⬆️
server/rosetta/converter.go	`54.44% <54.44%> (ø)`
... and 8 more

fdymylja · 2021-03-05T10:28:56Z

I'd like to get this merged because it has got really big, and do immediately a follow up PR with extra tests.

alessio · 2021-03-05T10:29:17Z

Linter is failing

jgimeno

Excellent job!! We are very close!!

server/rosetta/types.go

x/bank/types/msgs.go

fedekunze

LGTM pending changelog entry

robert-zaremba

only reviewed part of it.

robert-zaremba · 2021-03-10T16:17:57Z

server/rosetta/client_offline.go

 	}

-	txBytes, err := TxConfig.TxEncoder()(txBldr.GetTx())
+	// now we need to parse the operations to cosmos sdk messages
+	tx, err := c.converter.ToSDK().UnsignedTx(req.Operations)


Probably we can cache this operation (ToSDK) in the converter structure.

there's a lot of performance improvements that can be done (there's a double unmarshal on operations for example) but since I plan to do a follow up PR with extra tests immediately after this one is merged, I'd rather tackle the improvements in that, because this PR has got really big and its hard to review as it is. WDYT?

robert-zaremba · 2021-03-10T16:22:11Z

server/rosetta/client_online.go

+	txConfig := authtx.NewTxConfig(cfg.Codec, authtx.DefaultSignModes)
+
+	var supportedOperations []string
+	for _, ii := range cfg.InterfaceRegistry.ListImplementations("cosmos.base.v1beta1.Msg") {


please don't hardcode the message name. Instead:
let's create const in types/codec.go and export it.

do you have any preference in the naming? I was thinking of having a name which contains the version of the interface such as: MsgV1Beta1InterfaceName, although it looks ugly.

How about MsgProtoName? This is what we would get from the following call:

proto.MessageName((*Msg)(nil))

robert-zaremba

Added few more comments. For few general things:

godoc is missing for exported methods

robert-zaremba · 2021-03-11T10:35:17Z

docs/core/baseapp.md

@@ -104,7 +104,7 @@ Finally, a few more important parameterd:

 ```go
 func NewBaseApp(
-  name string, logger log.Logger, db dbm.DB, txDecoder sdk.TxDecoder, options ...func(*BaseApp),
+  name string, logger log.Logger, db dbm.DB, txDecode sdk.TxDecoder, options ...func(*BaseApp),


I think the original name is better.

i refactored something somewhere in the code and this got affected too! thanks for pointing it out!

robert-zaremba · 2021-03-11T10:38:42Z

server/rosetta/client_online.go

 	}, nil
 }

-func (c *Client) accountInfo(ctx context.Context, addr string, height *int64) (auth.AccountI, error) {
+// Bootstrap is gonna connect the client to the endpoints
+func (c *Client) Bootstrap() error {


Let's rename this method to Dial or Connect.

this is part of interface implementation

server/rosetta/client_online.go

robert-zaremba · 2021-03-11T10:40:00Z

server/rosetta/client_online.go

 }

-func (c *Client) Balances(ctx context.Context, addr string, height *int64) ([]*types.Amount, error) {
+func (c *Client) Balances(ctx context.Context, addr string, height *int64) ([]*rosettatypes.Amount, error) {


godoc is missing

godocs are missing because those methods are part of an interface implementation - which is in itself throughly documented - I will add the comments that those methods implement the said interface

server/rosetta/client_online.go

robert-zaremba · 2021-03-11T10:44:51Z

server/rosetta/client_online.go

-	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
-
-	"github.com/coinbase/rosetta-sdk-go/types"
+	rosettatypes "github.com/coinbase/rosetta-sdk-go/types"


imports are not ordered correctly

goimports does not give warnings, nor edit the files if I do run it against the rosetta dir.

do you use some other tooling?

Yes, goimports is not able to reorder lines that are blank-separated. It's a known limitation. We can fix this in a separate PR

robert-zaremba · 2021-03-11T10:45:48Z

server/rosetta/converter_test.go

+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
+	bank "github.com/cosmos/cosmos-sdk/x/bank/types"


See #8729 (comment)

server/rosetta/converter_test.go

robert-zaremba · 2021-03-11T10:48:59Z

server/rosetta/converter_test.go

+		s.Require().ErrorIs(err, crgerrs.ErrBadArgument)
+	})
+
+	s.Run("codec type but not sdk.Msg", func() {


this could be converted to table tests.

@fdymylja can you address this in your follow up PR, please?

server/rosetta/converter_test.go

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

fdymylja and others added 16 commits March 1, 2021 11:45

change(rosetta): data api block and tx parsing

702ea89

change(rosetta): finalize data API

8f711a1

add: converter

ab91ce3

fix: casting error message

b3bf68b

change: rework construction API to support every possible transaction…

593f26d

… type

change: make construction stateless

fbdeabc

chore: cleanup api

232d83f

chore: cleanup api

b43553e

chore: reorder methods declaration

e2b1a01

add: signed tx tests

5292379

add: ops and signers test

976f2e2

fix: begin and endblock tx conversions

a489652

add: begin and endblock invalid tests

18bd9e3

add: balance and signing components tests

a32c005

change: remove staking tests

9ad9d28

Merge branch 'master' into frojdi/rosetta-balance-tracking

52be187

fdymylja added 2 commits March 5, 2021 11:16

chore: lint

7375263

chore: lint

2246c6c

fdymylja marked this pull request as ready for review March 5, 2021 10:27

fdymylja requested review from aaronc, alessio and alexanderbez as code owners March 5, 2021 10:27

fdymylja requested review from jgimeno, sahith-narahari, fedekunze and amaury1093 March 5, 2021 10:27

revert: makefile rosetta test

4377a09

fdymylja and others added 3 commits March 5, 2021 11:41

chore: lint

8a9a140

chore: cleanup ci

bf428f4

Merge branch 'master' into frojdi/rosetta-balance-tracking

c7da349

alessio requested a review from robert-zaremba March 5, 2021 12:35

jgimeno approved these changes Mar 5, 2021

View reviewed changes

clevinson assigned robert-zaremba and amaury1093 Mar 5, 2021

Merge branch 'master' into frojdi/rosetta-balance-tracking

8af9112

fedekunze reviewed Mar 5, 2021

View reviewed changes

server/rosetta/types.go Outdated Show resolved Hide resolved

server/rosetta/types.go Outdated Show resolved Hide resolved

x/bank/types/msgs.go Show resolved Hide resolved

fdymylja and others added 3 commits March 5, 2021 21:26

chore: address documentation changes

b3f5410

chore: address documentation changes

0bb7b16

Merge branch 'master' into frojdi/rosetta-balance-tracking

daeb5f0

alessio requested a review from fedekunze March 9, 2021 18:05

Merge branch 'master' into frojdi/rosetta-balance-tracking

fe10978

fedekunze approved these changes Mar 10, 2021

View reviewed changes

robert-zaremba reviewed Mar 10, 2021

View reviewed changes

robert-zaremba reviewed Mar 11, 2021

View reviewed changes

fdymylja and others added 9 commits March 11, 2021 12:39

Update docs server/rosetta/client_online.go

57e684a

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

Update docs server/rosetta/client_online.go

0c3bad6

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

cleanup spacing server/rosetta/converter_test.go

d6e1622

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

revert: baseapp.md

f28f18d

add: docs for interface implementation

2f7a5c8

remove: converter_test.go utils anonymous type

9fda8a2

change: set interface name constant

958fba5

chore: add CHANGELOG.md entry

6354cfa

Merge branch 'master' into frojdi/rosetta-balance-tracking

3e47bb4

alessio added the A:automerge Automatically merge PR once all prerequisites pass. label Mar 11, 2021

alessio merged commit 288f8dd into master Mar 11, 2021

alessio deleted the frojdi/rosetta-balance-tracking branch March 11, 2021 15:01

dependabot bot mentioned this pull request Mar 18, 2021

Bump github.com/cosmos/cosmos-sdk from 0.40.0-rc5 to 0.42.2-rc0 celestiaorg/celestia-app#51

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rosetta] implement balance tracking and redo tx construction #8729

[rosetta] implement balance tracking and redo tx construction #8729

fdymylja commented Mar 1, 2021 •

edited

Loading

codecov bot commented Mar 5, 2021 •

edited

Loading

fdymylja commented Mar 5, 2021

alessio commented Mar 5, 2021

jgimeno left a comment

fedekunze left a comment

robert-zaremba left a comment

robert-zaremba Mar 10, 2021

fdymylja Mar 10, 2021

robert-zaremba Mar 10, 2021

fdymylja Mar 10, 2021

robert-zaremba Mar 10, 2021

robert-zaremba left a comment

robert-zaremba Mar 11, 2021

fdymylja Mar 11, 2021

robert-zaremba Mar 11, 2021

fdymylja Mar 11, 2021

robert-zaremba Mar 11, 2021

fdymylja Mar 11, 2021

robert-zaremba Mar 11, 2021

fdymylja Mar 11, 2021

alessio Mar 11, 2021

robert-zaremba Mar 11, 2021

alessio Mar 11, 2021

robert-zaremba Mar 11, 2021

alessio Mar 11, 2021

[rosetta] implement balance tracking and redo tx construction #8729

[rosetta] implement balance tracking and redo tx construction #8729

Conversation

fdymylja commented Mar 1, 2021 • edited Loading

Description

codecov bot commented Mar 5, 2021 • edited Loading

Codecov Report

fdymylja commented Mar 5, 2021

alessio commented Mar 5, 2021

jgimeno left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

robert-zaremba 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

robert-zaremba 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fdymylja commented Mar 1, 2021 •

edited

Loading

codecov bot commented Mar 5, 2021 •

edited

Loading