Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add default user for old endpoints #1665

Merged
merged 4 commits into from
Nov 27, 2023
Merged

add default user for old endpoints #1665

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
79e7e90
add default user for old endpoints
zkokelj Nov 24, 2023
9f1d117
minor fixes
zkokelj Nov 24, 2023
e4be3d2
add clients for default user, don't return default user, change log e…
zkokelj Nov 24, 2023
0817028
panic if there are errors in constructor
zkokelj Nov 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions tools/walletextension/container/walletextension_container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"net/http"
"os"

"github.com/ethereum/go-ethereum/crypto"

"github.com/ten-protocol/go-ten/go/common/log"
"github.com/ten-protocol/go-ten/go/common/stopcontrol"
"github.com/ten-protocol/go-ten/go/rpc"
Expand Down Expand Up @@ -51,6 +53,17 @@ func NewWalletExtensionContainerFromConfig(config config.Config, logger gethlog.
// add default user (when no UserID is provided in the query parameter - for WE endpoints)
userAccountManager.AddAndReturnAccountManager(hex.EncodeToString([]byte(wecommon.DefaultUser)))

// 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
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)
Copy link

@coderabbitai coderabbitai bot Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!

(\__/) 
(•ㅅ•)

os.Exit(1)
}

// get all users and their private keys from the database
allUsers, err := databaseStorage.GetAllUsers()
if err != nil {
Expand Down
Loading