add default user for old endpoints #1665

zkokelj · 2023-11-24T11:26:51Z

Why this change is needed

Following test was failing after latest changes: TestCannotInvokeSensitiveMethodsWithoutViewingKey
To get it working as expected we need to add a default user that is used with old endpoints and if userID was not found.

What changes were made as part of this PR

Added default userID to the database at startup

PR checks pre-merging

Please indicate below by ticking the checkbox that you have read and performed the required
PR checks

PR checks reviewed and performed

coderabbitai · 2023-11-24T11:26:56Z

Walkthrough

The changes across the codebase involve the integration of Ethereum cryptographic functions, adjustments to user account management, and modifications to logging practices. A new import for Ethereum's crypto package is added to generate key pairs for user accounts, and a default user is set up and managed within the system. Logging levels are shifted from error to informational in certain handlers, and test scenarios are updated to reflect new conditions. Additionally, there's a focus on preventing the exposure of the default user's ID.

Changes

File Path	Change Summary
`.../walletextension_container.go`	Introduced Ethereum crypto package for key generation, added default user account management, and implemented a temporary fix for database compatibility.
`.../api/routes.go`	Changed logging level from "Error" to "Info" in various request handlers.
`.../test/wallet_extension_test.go`	Updated the `invalidUserID` value to a longer string in test scenarios.
`.../wallet_extension.go`	Added import for account manager and included checks to prevent default user ID exposure.

Tips

Chat with CodeRabbit Bot (`@coderabbitai`)

If you reply to a review comment from CodeRabbit, the bot will automatically respond.
To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
Note: Review comments are made on code diffs or files, not on the PR overview.
Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
The JSON schema for the configuration file is available here.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

coderabbitai

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 378aec6 and 79e7e90.

Files selected for processing (1)

tools/walletextension/container/walletextension_container.go (2 hunks)

Additional comments: 2

tools/walletextension/container/walletextension_container.go (2)

7-14:
The addition of the crypto package is consistent with the changes described in the summary.

57-57:
Ensure that the generated key pair is securely handled and that the private key is not exposed or logged.

coderabbitai · 2023-11-24T11:28:30Z

tools/walletextension/container/walletextension_container.go

+	// add default user to the database (temporary fix before removing wallet extension endpoints)
+	accountPrivateKey, err := crypto.GenerateKey()
+	if err != nil {
+		logger.Error("Unable to generate hey pair for default user", log.ErrKey, err)


Typo in the log message.

- logger.Error("Unable to generate hey pair for default user", log.ErrKey, err) + logger.Error("Unable to generate key pair for default user", log.ErrKey, err)

Commitable suggestion

❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change

logger.Error("Unable to generate hey pair for default user", log.ErrKey, err)

logger.Error("Unable to generate key pair for default user", log.ErrKey, err)

@zkokelj Great, if there's anything else that needs attention, feel free to let me know!

coderabbitai · 2023-11-24T11:28:30Z

tools/walletextension/container/walletextension_container.go

+	err = databaseStorage.AddUser([]byte(wecommon.DefaultUser), crypto.FromECDSA(accountPrivateKey))
+	if err != nil {
+		logger.Error("Unable to save default user to the database", log.ErrKey, err)


The error from databaseStorage.AddUser is not handled; it should exit or return the error instead of just logging it.

- logger.Error("Unable to save default user to the database", log.ErrKey, err) + return fmt.Errorf("unable to save default user to the database: %w", err)

Commitable suggestion

❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change

err = databaseStorage.AddUser([]byte(wecommon.DefaultUser), crypto.FromECDSA(accountPrivateKey))

if err != nil {

logger.Error("Unable to save default user to the database", log.ErrKey, err)

err = databaseStorage.AddUser([]byte(wecommon.DefaultUser), crypto.FromECDSA(accountPrivateKey))

if err != nil {

return fmt.Errorf("unable to save default user to the database: %w", err)

@zkokelj Great, if the fix has been applied as suggested, it should now properly handle the error when saving the default user to the database. If there's anything else that needs attention, feel free to let me know!

(\__/) (•ㅅ•)

coderabbitai

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 79e7e90 and 9f1d117.

Files selected for processing (1)

tools/walletextension/container/walletextension_container.go (2 hunks)

Additional comments: 2

tools/walletextension/container/walletextension_container.go (2)

7-14:
The addition of the "github.com/ethereum/go-ethereum/crypto" package is appropriate for generating cryptographic key pairs as described in the summary.

61-65:
The error handling for databaseStorage.AddUser now correctly exits the program if an error occurs, which aligns with the previous review comments.

…rror to info when user is not found

coderabbitai

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 9f1d117 and e4be3d2.

Files selected for processing (4)

tools/walletextension/api/routes.go (3 hunks)
tools/walletextension/container/walletextension_container.go (2 hunks)
tools/walletextension/test/wallet_extension_test.go (1 hunks)
tools/walletextension/wallet_extension.go (2 hunks)

Files skipped from review due to trivial changes (2)

tools/walletextension/api/routes.go
tools/walletextension/test/wallet_extension_test.go

Additional comments: 2

tools/walletextension/wallet_extension.go (2)

9-13:
The import statement for accountmanager is correctly added.

340-354:
The logic to prevent exposing the default user's userID externally is correctly implemented.

tudor-malene

left a comment around error handling

tudor-malene · 2023-11-24T16:08:39Z

tools/walletextension/container/walletextension_container.go

+	// add default user to the database (temporary fix before removing wallet extension endpoints)
+	accountPrivateKey, err := crypto.GenerateKey()
+	if err != nil {
+		logger.Error("Unable to generate key pair for default user", log.ErrKey, err)


for all these error handlings in the constructor, I think the gateway should panic.
There is no point continuing

coderabbitai

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between e4be3d2 and 0817028.

Files selected for processing (1)

tools/walletextension/container/walletextension_container.go (2 hunks)

Additional comments: 1

tools/walletextension/container/walletextension_container.go (1)

79-99: The TODO comments indicate that the code related to the default user is a temporary fix and should be removed in the future. Ensure that the removal of this code is tracked and managed properly to avoid regressions or the unintentional removal of necessary functionality.

coderabbitai · 2023-11-27T07:48:01Z

tools/walletextension/container/walletextension_container.go

+	// add default user to the database (temporary fix before removing wallet extension endpoints)
+	accountPrivateKey, err := crypto.GenerateKey()
+	if err != nil {
+		logger.Error("Unable to generate key pair for default user", log.ErrKey, err)


The use of os.Exit(1) is consistent with the error handling strategy in the constructor, effectively halting the program on critical errors. However, consider using panic to provide a stack trace for better debugging.

tudor-malene

lgtm

add default user for old endpoints

79e7e90

zkokelj marked this pull request as ready for review November 24, 2023 11:26

coderabbitai bot reviewed Nov 24, 2023

View reviewed changes

minor fixes

9f1d117

coderabbitai bot reviewed Nov 24, 2023

View reviewed changes

add clients for default user, don't return default user, change log e…

e4be3d2

…rror to info when user is not found

coderabbitai bot reviewed Nov 24, 2023

View reviewed changes

ten-protocol deleted a comment from coderabbitai bot Nov 24, 2023

tudor-malene reviewed Nov 24, 2023

View reviewed changes

panic if there are errors in constructor

0817028

zkokelj requested a review from tudor-malene November 27, 2023 07:44

coderabbitai bot reviewed Nov 27, 2023

View reviewed changes

tudor-malene approved these changes Nov 27, 2023

View reviewed changes

zkokelj merged commit 3af59e7 into main Nov 27, 2023
1 of 2 checks passed

zkokelj deleted the ziga/fix_default_user_for_old_endpoints branch November 27, 2023 09:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add default user for old endpoints #1665

add default user for old endpoints #1665

zkokelj commented Nov 24, 2023

coderabbitai bot commented Nov 24, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

coderabbitai bot left a comment

coderabbitai bot Nov 24, 2023 •

edited

Loading

zkokelj Nov 24, 2023

coderabbitai bot Nov 24, 2023

coderabbitai bot Nov 24, 2023 •

edited

Loading

zkokelj Nov 24, 2023

coderabbitai bot Nov 24, 2023

coderabbitai bot left a comment

coderabbitai bot left a comment

tudor-malene left a comment

tudor-malene Nov 24, 2023

zkokelj Nov 27, 2023

coderabbitai bot left a comment

coderabbitai bot Nov 27, 2023

tudor-malene left a comment

	logger.Error("Unable to generate hey pair for default user", log.ErrKey, err)
	logger.Error("Unable to generate key pair for default user", log.ErrKey, err)

add default user for old endpoints #1665

add default user for old endpoints #1665

Conversation

zkokelj commented Nov 24, 2023

Why this change is needed

What changes were made as part of this PR

PR checks pre-merging

coderabbitai bot commented Nov 24, 2023 • edited Loading

Walkthrough

Changes

Chat with CodeRabbit Bot (@coderabbitai)

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

zkokelj Nov 24, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 24, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

zkokelj Nov 24, 2023

Choose a reason for hiding this comment

coderabbitai bot Nov 24, 2023

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

tudor-malene left a comment

Choose a reason for hiding this comment

tudor-malene Nov 24, 2023

Choose a reason for hiding this comment

zkokelj Nov 27, 2023

Choose a reason for hiding this comment

coderabbitai bot left a comment

Choose a reason for hiding this comment

coderabbitai bot Nov 27, 2023

Choose a reason for hiding this comment

tudor-malene left a comment

Choose a reason for hiding this comment

coderabbitai bot commented Nov 24, 2023 •

edited

Loading

Chat with CodeRabbit Bot (`@coderabbitai`)

CodeRabbit Configration File (`.coderabbit.yaml`)

coderabbitai bot Nov 24, 2023 •

edited

Loading

coderabbitai bot Nov 24, 2023 •

edited

Loading