fix: misc fixes for cosmos-rosetta #13583

rllola · 2022-10-18T14:45:48Z

Description

Closes:
#13083
#11402
#10678
#12358
#10776
#12934

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

fix missing coma should work print block height answer why index null should fix getHeight forgot fmt fix get account at current height check rosetta request misc misc misc misc misc misc misc misc infinite loop

* Add go.mod for rosetta server * Update go.mod files * Fix go.mod

* Print messages when rosetta is online and when it cannot connect to rpc server * Fix masking underlaying error * Delete fmt output * Avoid getting genesis block at Ready(), use Health and Status calls instead

* Get genesis hash from env * Print error message if hash env is not set

julienrbrt · 2022-10-18T15:13:11Z

Could you add the new module here too:

server/rosetta/go.mod

tac0turtle

left a few comments, seems to be some straggling print statements. Otherwise looks good to me

server/rosetta/client_online.go

rllola · 2022-10-20T17:50:28Z

@tac0turtle I have removed the "Println".

codecov · 2022-10-20T17:58:56Z

Codecov Report

Merging #13583 (d23da2f) into main (2bb114a) will increase coverage by 0.01%.
The diff coverage is 23.07%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13583      +/-   ##
==========================================
+ Coverage   56.03%   56.05%   +0.01%     
==========================================
  Files         646      659      +13     
  Lines       55790    57393    +1603     
==========================================
+ Hits        31264    32169     +905     
- Misses      21984    22631     +647     
- Partials     2542     2593      +51

Impacted Files	Coverage Δ
tools/rosetta/client_offline.go	`0.00% <ø> (ø)`
tools/rosetta/client_online.go	`1.30% <0.00%> (ø)`
tools/rosetta/codec.go	`100.00% <ø> (ø)`
tools/rosetta/config.go	`0.00% <ø> (ø)`
tools/rosetta/lib/errors/errors.go	`69.84% <ø> (ø)`
tools/rosetta/lib/errors/registry.go	`100.00% <ø> (ø)`
tools/rosetta/types.go	`0.00% <ø> (ø)`
tools/rosetta/util.go	`0.00% <ø> (ø)`
simapp/simd/cmd/root.go	`70.23% <100.00%> (ø)`
tools/rosetta/converter.go	`56.75% <100.00%> (ø)`
... and 16 more

server/rosetta/converter.go

facundomedica · 2022-10-20T19:50:41Z

LGTM, let's change the mod url and we can merge it :)

tac0turtle

LGTM,

julienrbrt

Please add the go mod here too: #13583 (comment)

julienrbrt · 2022-10-21T14:46:56Z

docs/docs/run-node/04-rosetta.md

@@ -77,7 +77,7 @@ import (

 "context"
 "github.com/coinbase/rosetta-sdk-go/types"
-"github.com/cosmos/cosmos-sdk/server/rosetta/lib"
+"github.com/cosmos/cosmos-sdk/rosetta/lib"


Can you update this?

done!

Can you have a look at the last comments and the tests? Then it's ready to be merged :)

done with the comments, taking a look to the tests now

julienrbrt · 2022-10-21T14:48:09Z

tools/rosetta/go.mod

+)
+
+replace (
+	github.com/coinbase/rosetta-sdk-go => github.com/coinbase/rosetta-sdk-go v0.8.2-0.20221007214527-e03849ba430a


Can we add a comment on why this is necessary?

julienrbrt · 2022-10-21T14:48:34Z

tools/rosetta/go.mod

+
+replace (
+	github.com/coinbase/rosetta-sdk-go => github.com/coinbase/rosetta-sdk-go v0.8.2-0.20221007214527-e03849ba430a
+	github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1


This should not be need anymore. We've moved to cosmos/gogoproto. Can you verify you use it too in Rosetta?

I think we need it because we're importing this version of the sdk
https://github.com/Zondax/cosmos-sdk/blob/cee3d9f0e05e8658cd4908402a14805c5237cac1/tools/rosetta/go.mod#L9
is there a release/tag that is actually using cosmos/gogoproto ?

I see, we don't have a tag yet. Cosmovisor uses v0.46.0-beta2.0.20220909113810-4882f933b1a1, which is fine to use I think, until we tag 0.47. IMO we should do the same here.

tools/rosetta/client_online.go

odeke-em

Thank you for this change @rllola! I've added some comments but the most important one is to examine if we can .Server's logger to avoid spamming os.Stdout yet the logs are inside a library/package and not in cmd/* code.

odeke-em · 2022-10-31T20:31:09Z

server/start.go

-			offlineMode = true
+		// If GRPC is not enabled rosetta cannot work in online mode, so we throw an error.
+		if !config.GRPC.Enable && !offlineMode {
+			return fmt.Errorf("'grpc' must be enable in online mode for Rosetta to work")


odeke-em · 2022-10-31T20:32:59Z

tools/rosetta/lib/internal/service/online.go

+	// using tendermint API
+	genesisHash := os.Getenv(genesisHashEnv)
+	if genesisHash == "" {
+		_ = fmt.Sprintf("[Warning]- Genesis hash env '%s' is not properly set!", genesisHashEnv)


Did you mean to invoke fmt.Printf instead? Why this if we are ignoring the string? I see a "Warning" hence we should figure out whether to return an error or not.

odeke-em · 2022-10-31T20:33:32Z

tools/rosetta/lib/server/server.go

@@ -41,6 +41,7 @@ type Server struct {
 }

 func (h Server) Start() error {
+	fmt.Printf("Rosetta server listening on add %s", h.addr)


is the server allowed to be chatted and just print out to os.Stdout? If so, is there a way to add a logger to Server and instead use that?

Hi! is there any logging service inside the sdk that we can use to avoid these printf ?

@raynaudoe probably github.com/tendermint/tendermint/libs/log

odeke-em · 2022-10-31T20:33:42Z

tools/rosetta/lib/server/server.go

@@ -108,6 +109,7 @@ func newOnlineAdapter(settings Settings) (crgtypes.API, error) {
 	for i := 0; i < settings.Retries; i++ {
 		err = settings.Client.Ready()
 		if err != nil {
+			fmt.Printf("[Rosetta]- Client is not ready: %s. Retrying ...", err.Error())


Same about using a logger.

raynaudoe · 2022-10-31T21:27:20Z

I've fixed the rosetta-tests, now the only test failing is Code scanning results / gosec but I'm seeing errors that were not introduced by this PR (maybe moving the affected files reactivated them?). Are they safe to skip?

facundomedica · 2022-11-02T17:04:30Z

I've fixed the rosetta-tests, now the only test failing is Code scanning results / gosec but I'm seeing errors that were not introduced by this PR (maybe moving the affected files reactivated them?). Are they safe to skip?

I don't think the errors detected by gosec are too bad, I would say they are safe to skip. Ideally we would like to have none but I don't want to block this because of that.

Could you please allow maintainers to push to this branch? So we can pull update from main as many times as needed so we can merge it.
Were the concerns pointed out by @odeke-em solved?

Thank you!

raynaudoe · 2022-11-02T19:26:54Z

I've fixed the rosetta-tests, now the only test failing is Code scanning results / gosec but I'm seeing errors that were not introduced by this PR (maybe moving the affected files reactivated them?). Are they safe to skip?

I don't think the errors detected by gosec are too bad, I would say they are safe to skip. Ideally we would like to have none but I don't want to block this because of that.

Could you please allow maintainers to push to this branch? So we can pull update from main as many times as needed so we can merge it.

Were the concerns pointed out by @odeke-em solved?

Thank you!

Sure, we're on it!

JulianToledano · 2022-11-03T19:12:08Z

Thank you for this change @rllola! I've added some comments but the most important one is to examine if we can .Server's logger to avoid spamming os.Stdout yet the logs are inside a library/package and not in cmd/* code.

Changes are ready :) @odeke-em

odeke-em

Nice updates, thank you Zondax team. Just 3 more comments where 1 of them is critical to perhaps return an error when the genesis hash is empty, but otherwise now LGTM!

odeke-em · 2022-11-03T19:14:43Z

tools/rosetta/converter.go

@@ -349,8 +349,8 @@ func sdkEventToBalanceOperations(status string, event abci.Event) (operations []
 		accountIdentifier = spender.String()

 	case banktypes.EventTypeCoinReceived:
-		receiver := sdk.MustAccAddressFromBech32(event.Attributes[0].Value)
-		coins, err := sdk.ParseCoinsNormalized(event.Attributes[1].Value)
+		receiver := sdk.MustAccAddressFromBech32(string(event.Attributes[0].Value))


Is this conversion necessary? Did this type change?

odeke-em · 2022-11-03T19:15:59Z

tools/rosetta/lib/internal/service/online.go

+	// using tendermint API
+	genesisHash := os.Getenv(genesisHashEnv)
+	if genesisHash == "" {
+		logger.Error(fmt.Sprintf("Genesis hash env '%s' is not properly set!", genesisHashEnv))


After this log, should this return an error given we expected the genesis hash which is critical?

Not having the genesis hash is not critical right now, however we have a fix ready for a next PR that contemplates the proper genesis block height and 'trust height' depending on the sync method used to sync the node.

odeke-em · 2022-11-03T19:18:04Z

tools/rosetta/lib/server/server.go

@@ -108,10 +115,11 @@ func newOnlineAdapter(settings Settings) (crgtypes.API, error) {
 	for i := 0; i < settings.Retries; i++ {
 		err = settings.Client.Ready()
 		if err != nil {
+			logger.Error(fmt.Sprintf("[Rosetta]- Client is not ready: %s. Retrying ...", err.Error()))


Please use %v for the error and no need for err.Error()

julienrbrt · 2022-11-04T06:14:45Z

Putting the automerge tag. When you've rebased and fixed the conflicts, it'll automerge.
I'll tag rosetta after. Any preference qua version number? :)

raynaudoe · 2022-11-04T10:51:00Z

great! I'll fix the conflicts ASAP.
Regarding version number, no preferences at all!

### Description Closes: cosmos#13083 cosmos#11402 cosmos#10678 cosmos#12358 cosmos#10776 cosmos#12934 ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [x] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [x] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

rllola and others added 7 commits October 18, 2022 16:42

hardcode block identifier (#3)

24f8cef

fix missing coma should work print block height answer why index null should fix getHeight forgot fmt fix get account at current height check rosetta request misc misc misc misc misc misc misc misc infinite loop

feat: Add go.mod for rosetta server (#4)

4eaa2bd

* Add go.mod for rosetta server * Update go.mod files * Fix go.mod

fix: minor fixes (#18)

8ff651c

* Print messages when rosetta is online and when it cannot connect to rpc server * Fix masking underlaying error * Delete fmt output * Avoid getting genesis block at Ready(), use Health and Status calls instead

throw an error in case of config conflict (rosetta offline mode) (#20)

fc58c6d

Fix blockidentifier not specify (#33)

be4facc

Parse base64 encoded events (#38)

d6b9645

Fix genesis block response (#42)

edf20cd

* Get genesis hash from env * Print error message if hash env is not set

rllola requested a review from a team as a code owner October 18, 2022 14:45

raynaudoe mentioned this pull request Oct 18, 2022

fix: misc fixes for cosmos-rosetta #13578

Closed

19 tasks

julienrbrt reviewed Oct 18, 2022

View reviewed changes

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

tac0turtle reviewed Oct 18, 2022

View reviewed changes

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

tac0turtle reviewed Oct 20, 2022

View reviewed changes

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

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

tac0turtle assigned facundomedica and julienrbrt Oct 20, 2022

remove println

0c2779f

facundomedica reviewed Oct 20, 2022

View reviewed changes

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

Move rosetta to tools dir

cee3d9f

tac0turtle approved these changes Oct 21, 2022

View reviewed changes

raynaudoe added 2 commits October 21, 2022 16:16

Rename variable

47e0dab

Merge branch 'main' into misc/zondax-rosetta

8aa02d3

julienrbrt requested changes Oct 21, 2022

View reviewed changes

ainhoa-a mentioned this pull request Oct 25, 2022

EPIC: Zondax maintenance 08/09 #13327

Closed

20 tasks

raynaudoe added 3 commits October 25, 2022 10:39

update module name

e07319d

Add rosetta entry

83d60a4

Add rosetta to go.work.example

56288da

raynaudoe added 3 commits October 27, 2022 07:40

Run mod tidy

7f75a36

Fix lint errors

91882fb

Set genesis hash env for rosetta tests

0285cee

github-actions bot added the C:Rosetta Issues and PR related to Rosetta label Oct 27, 2022

raynaudoe added 4 commits October 27, 2022 08:34

Update dockerfile to include tools/rosetta

bdb125f

Fix ToRosetta func

468b92c

Revert decoding events

17aecca

Fix supply pagination

527a020

github-advanced-security bot found potential problems Oct 31, 2022

View reviewed changes

tools/rosetta/client_online.go Fixed Show fixed Hide fixed

raynaudoe added 2 commits October 31, 2022 09:21

Fix potential int overflow

69f9558

Merge branch 'main' into misc/zondax-rosetta

ab148f2

odeke-em requested changes Oct 31, 2022

View reviewed changes

raynaudoe and others added 3 commits November 3, 2022 14:08

Merge branch 'main' into misc/zondax-rosetta

fdb6613

fix: use errors.New instead fmt.Error

66707bb

add: logger rosetta server

709662b

odeke-em approved these changes Nov 3, 2022

View reviewed changes

julienrbrt added the A:automerge Automatically merge PR once all prerequisites pass. label Nov 4, 2022

fix: log %v err

22fc835

Merge branch 'main' into misc/zondax-rosetta

d23da2f

mergify bot merged commit 39c5c07 into cosmos:main Nov 4, 2022

julienrbrt mentioned this pull request Nov 4, 2022

chore: use tagged rosetta version #13765

Merged

19 tasks

coderabbitai bot mentioned this pull request Apr 23, 2024

feat: bump cosmos-sdk to v0.50.6 cosmos/rosetta#114

Merged

coderabbitai bot mentioned this pull request May 3, 2024

fix: update blockTxs to correctly parse finalizeblock events cosmos/rosetta#123

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: misc fixes for cosmos-rosetta #13583

fix: misc fixes for cosmos-rosetta #13583

rllola commented Oct 18, 2022 •

edited by raynaudoe

Loading

julienrbrt commented Oct 18, 2022

tac0turtle left a comment

rllola commented Oct 20, 2022

codecov bot commented Oct 20, 2022 •

edited

Loading

facundomedica commented Oct 20, 2022

tac0turtle left a comment

julienrbrt left a comment

julienrbrt Oct 21, 2022

raynaudoe Oct 27, 2022

julienrbrt Oct 21, 2022

julienrbrt Oct 21, 2022

raynaudoe Oct 25, 2022 •

edited

Loading

julienrbrt Oct 25, 2022 •

edited

Loading

raynaudoe Oct 27, 2022

odeke-em left a comment

odeke-em Oct 31, 2022

odeke-em Oct 31, 2022

odeke-em Oct 31, 2022

raynaudoe Oct 31, 2022

facundomedica Oct 31, 2022

odeke-em Oct 31, 2022

raynaudoe commented Oct 31, 2022 •

edited

Loading

facundomedica commented Nov 2, 2022

raynaudoe commented Nov 2, 2022

JulianToledano commented Nov 3, 2022 •

edited

Loading

odeke-em left a comment

odeke-em Nov 3, 2022

odeke-em Nov 3, 2022

raynaudoe Nov 4, 2022

odeke-em Nov 3, 2022

julienrbrt commented Nov 4, 2022

raynaudoe commented Nov 4, 2022

fix: misc fixes for cosmos-rosetta #13583

fix: misc fixes for cosmos-rosetta #13583

Conversation

rllola commented Oct 18, 2022 • edited by raynaudoe Loading

Description

Author Checklist

Reviewers Checklist

julienrbrt commented Oct 18, 2022

tac0turtle left a comment

Choose a reason for hiding this comment

rllola commented Oct 20, 2022

codecov bot commented Oct 20, 2022 • edited Loading

Codecov Report

facundomedica commented Oct 20, 2022

tac0turtle left a comment

Choose a reason for hiding this comment

julienrbrt 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

raynaudoe Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

julienrbrt Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odeke-em 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

raynaudoe commented Oct 31, 2022 • edited Loading

facundomedica commented Nov 2, 2022

raynaudoe commented Nov 2, 2022

JulianToledano commented Nov 3, 2022 • edited Loading

odeke-em 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

julienrbrt commented Nov 4, 2022

raynaudoe commented Nov 4, 2022

rllola commented Oct 18, 2022 •

edited by raynaudoe

Loading

codecov bot commented Oct 20, 2022 •

edited

Loading

raynaudoe Oct 25, 2022 •

edited

Loading

julienrbrt Oct 25, 2022 •

edited

Loading

raynaudoe commented Oct 31, 2022 •

edited

Loading

JulianToledano commented Nov 3, 2022 •

edited

Loading