Skip to content

Remote wallet#5144

Closed
alonmuroch wants to merge 6 commits into
OffchainLabs:masterfrom
alonmuroch:remote_wallet
Closed

Remote wallet#5144
alonmuroch wants to merge 6 commits into
OffchainLabs:masterfrom
alonmuroch:remote_wallet

Conversation

@alonmuroch

Copy link
Copy Markdown
Contributor

A PR for remote wallet key manager

TODO(you): choose "part of" or "resolves" and the associated github issue #.

[Part of|Resolves] #531


Description

Write why you are making the changes in this pull request

Write a summary of the changes you are making

Link anything that would be helpful or relevant to the reviewers

@CLAassistant

CLAassistant commented Mar 20, 2020

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

import (
"context"
"fmt"
"github.com/prysmaticlabs/prysm/shared/bls"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

goimports

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please run goimports -w validator/client/validator_aggregate.go these imports are not sorted properly.

return bls.SignatureFromBytes(r.Sig)
}

// SignSlot signs an aggregate attestation for the validator to broadcast.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// SignSlot signs an aggregate attestation for the validator to broadcast.
// SignSlot signs the selection proof of an aggregated attestation for the validator to broadcast.

c := RW.NewRemoteWalletClient(conn)

km := &RemoteWallet{
keys: make([][48]byte,0),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can we define 48 as a constant somewhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that as well. It makes a lot of sense.
I'll try and do a different PR for that

Url string `json:"url"`
}

// TODO

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is the TODO here?

Comment thread WORKSPACE
importpath = "github.com/wealdtech/go-indexer",
)

local_repository(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please update

@prestonvanloon

prestonvanloon commented Mar 23, 2020

Copy link
Copy Markdown
Member

Is this PR relevant #5133?

@alonmuroch

Copy link
Copy Markdown
Contributor Author

@prestonvanloon I saw you merged @mcdee PR so I guess this one is irrelevant.

@prestonvanloon

Copy link
Copy Markdown
Member

Alright, thanks @alonmuroch! Closing this in favor of #5133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants