fix(evm): define 'startedSigning' event name and print command ID decimal #1219

jack0son · 2022-01-16T00:14:14Z

Cross referencing command ID to transfer ID in logs was difficult because they were printed in hex and decimal formats respectively.

Description

Todos

Steps to Test

Expected Behaviour

Other Notes

jack0son · 2022-01-16T01:18:11Z

x/evm/keeper/msg_server.go

-	for _, commandID := range commandList {
-		s.Logger(ctx).Debug("signing command batch", "commandBatchID", batchedCommandsIDHex, "commandID", commandID)
+	for _, commandIDHex := range commandList {
+		commandID, err := strconv.ParseUint(commandIDHex, 16, 64)


@jcs47 I made a mistake here - ParseUint expects bit size not byte size.

Why convert to uint? not all command id origins from transfer id

Same question. Also, these command IDs are of bytes32 in the contracts which means it would overflow for ParseUint.

This was a naive quick fix to try and address the mapping issue on call 😢. Reverting.

milapsheth · 2022-01-16T03:32:22Z

x/evm/keeper/msg_server.go

+			return nil, err
+		}
+
+		s.Logger(ctx).Debug(fmt.Sprintf("signing command batch %s", batchedCommandsIDHex), "commandBatchID", batchedCommandsIDHex, "commandIDHex", commandIDHex, "commandID",


Haha, related fix: https://github.com/axelarnetwork/axelar-core/pull/1220/files#diff-4a401bcb508aac66739d477b623d017e5d9aa737701e568f8f947775965938d7R1465
I prefer adding the values in the log itself rather than attributes since it's much easier to search datadog with that.

I can drop my line change here if you wanna combine that logic

Think we should always at minimum have attributes as the formal method for querying logs. Prefer to have both as logging some info in the message makes quick searches easier.

Need to agree on these points in the issue you made.

facets definitely seem like the best way to correlate logs through various stages since it doesn't require the user to add that key value information into the message string itself and is automatically appended to every message within a given stage, but I will say that a downside is if we get into a situation where we are using many different facet names for the same information, forcing us to union many different facets to get a complete picture versus just being able to search for one string value. that being said, we just need to coordinate on naming conventions (how to capitalize, when to use underscores, etc) -- it may be useful to have a sort of internal library that can codify some of these standards, but we can flesh that requirement out more down the line once more of these multi-facets appear and we have a real need for it

👍 👍 👍 could you please add this comment here #1227 @wilcollins ?

haiyizxx · 2022-01-16T03:36:37Z

x/evm/keeper/msg_server.go

-	for _, commandID := range commandList {
-		s.Logger(ctx).Debug("signing command batch", "commandBatchID", batchedCommandsIDHex, "commandID", commandID)
+	for _, commandIDHex := range commandList {
+		commandID, err := strconv.ParseUint(commandIDHex, 16, 64)


Why convert to uint? not all command id origins from transfer id

fish-sammy · 2022-01-17T18:55:53Z

x/evm/keeper/msg_server.go

-	for _, commandID := range commandList {
-		s.Logger(ctx).Debug("signing command batch", "commandBatchID", batchedCommandsIDHex, "commandID", commandID)
+	for _, commandIDHex := range commandList {
+		commandID, err := strconv.ParseUint(commandIDHex, 16, 64)


Same question. Also, these command IDs are of bytes32 in the contracts which means it would overflow for ParseUint.

…imal add batch id to message content strconv.ParseUint uses bit size NOT byte size revert commandID hex -> uint conversion

jack0son · 2022-01-19T22:22:11Z

x/evm/keeper/msg_server.go

+			types.AttributeKeyChain, chain.Name,
+			tsstypes.AttributeKeyKeyID, string(commandBatch.GetKeyID()),
+			"commandBatchID", batchedCommandsIDHex,
+			"commandID", commandID,


@milapsheth

Can we please keep the values in the log message for now? We should refactor logs at the same time for consistency, otherwise it's gonna be harder during debugging.

I'm not sure if the logging refactor will go in to the next ERC-20 release.

Reverted to the original log message with the attributes included as well

wilcollins · 2022-01-21T19:09:14Z

x/evm/types/events.go

@@ -9,6 +9,7 @@ const (
 	EventTypeTokenConfirmation             = "tokenConfirmation"
 	EventTypeTransferKeyConfirmation       = "transferKeyConfirmation"
 	EventTypeLink                          = "link"
+	EventTypeStartedSigning                = "startedSigning"


Suggested change

EventTypeStartedSigning = "startedSigning"

EventTypeStartSigning = "startSigning"

nit: the past tense seems superfluous and not really within the current conventions

cgorenflo · 2022-01-26T17:18:46Z

x/evm/types/events.go

@@ -9,6 +9,7 @@ const (
 	EventTypeTokenConfirmation             = "tokenConfirmation"
 	EventTypeTransferKeyConfirmation       = "transferKeyConfirmation"
 	EventTypeLink                          = "link"
+	EventTypeStartedSigning                = "startedSigning"


event type should be sign and action should be start

milapsheth

Don't want to change the log style until the grand refactor

addressed

jack0son requested a review from haiyizxx January 16, 2022 00:14

jack0son requested review from cgorenflo, jcs47 and fish-sammy as code owners January 16, 2022 00:14

jcs47 approved these changes Jan 16, 2022

View reviewed changes

jack0son commented Jan 16, 2022

View reviewed changes

jack0son requested a review from jcs47 January 16, 2022 02:56

milapsheth reviewed Jan 16, 2022

View reviewed changes

haiyizxx requested changes Jan 16, 2022

View reviewed changes

fish-sammy requested changes Jan 17, 2022

View reviewed changes

jack0son added 2 commits January 20, 2022 08:33

fix(evm): define 'startedSigning' event name and print command ID dec…

ed64c85

…imal add batch id to message content strconv.ParseUint uses bit size NOT byte size revert commandID hex -> uint conversion

add log message attributes

144437e

jack0son force-pushed the fix/started-signing-event branch from 23ec2a9 to 144437e Compare January 19, 2022 22:20

jack0son commented Jan 19, 2022

View reviewed changes

jack0son requested review from fish-sammy, haiyizxx and milapsheth January 19, 2022 22:22

wilcollins reviewed Jan 21, 2022

View reviewed changes

cgorenflo previously requested changes Jan 26, 2022

View reviewed changes

jack0son added 3 commits January 28, 2022 18:18

Merge branch 'main' into fix/started-signing-event

89c0e33

fix sign event type

7233e10

remove variables from log message

3d4e990

jack0son enabled auto-merge (squash) January 28, 2022 07:52

jack0son requested review from cgorenflo and wilcollins January 31, 2022 21:09

jack0son added 2 commits February 1, 2022 07:39

Merge branch 'main' into fix/started-signing-event

3dbc0fe

Merge branch 'main' into fix/started-signing-event

bd5d924

jcs47 approved these changes Feb 1, 2022

View reviewed changes

fish-sammy approved these changes Feb 1, 2022

View reviewed changes

milapsheth requested changes Feb 1, 2022

View reviewed changes

original log message

fc577b3

milapsheth approved these changes Feb 1, 2022

View reviewed changes

Merge branch 'main' into fix/started-signing-event

58ab783

Merge branch 'main' into fix/started-signing-event

584099c

haiyizxx approved these changes Feb 2, 2022

View reviewed changes

jack0son merged commit 8e7cbdc into main Feb 2, 2022

jack0son deleted the fix/started-signing-event branch February 2, 2022 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(evm): define 'startedSigning' event name and print command ID decimal #1219

fix(evm): define 'startedSigning' event name and print command ID decimal #1219

jack0son commented Jan 16, 2022 •

edited

jack0son Jan 16, 2022

haiyizxx Jan 16, 2022

fish-sammy Jan 17, 2022

jack0son Jan 19, 2022

milapsheth Jan 16, 2022

milapsheth Jan 16, 2022

jack0son Jan 19, 2022

wilcollins Jan 19, 2022 •

edited

jack0son Jan 19, 2022

haiyizxx Jan 16, 2022

fish-sammy Jan 17, 2022

jack0son Jan 19, 2022

milapsheth Feb 1, 2022

milapsheth Feb 1, 2022

jack0son Feb 1, 2022

wilcollins Jan 21, 2022

cgorenflo Jan 26, 2022

milapsheth left a comment

	EventTypeStartedSigning = "startedSigning"
	EventTypeStartSigning = "startSigning"

fix(evm): define 'startedSigning' event name and print command ID decimal #1219

fix(evm): define 'startedSigning' event name and print command ID decimal #1219

Conversation

jack0son commented Jan 16, 2022 • edited

Description

Todos

Steps to Test

Expected Behaviour

Other Notes

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

wilcollins Jan 19, 2022 • edited

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

milapsheth left a comment

Choose a reason for hiding this comment

jack0son commented Jan 16, 2022 •

edited

wilcollins Jan 19, 2022 •

edited