rm sig check #770
rm sig check #770
Conversation
| @@ -52,9 +52,6 @@ func (c *EigenDAClientConfig) CheckAndSetDefaults() error { | |||
| if c.ResponseTimeout == 0 { | |||
| c.ResponseTimeout = 30 * time.Second | |||
| } | |||
| if len(c.SignerPrivateKeyHex) != 64 { | |||
There was a problem hiding this comment.
Instead of removing this check, check length only when this value is not nil?
There was a problem hiding this comment.
SignerPrivateKeyHex is a string defined in the config, it cannot be nil.
There was a problem hiding this comment.
https://go.dev/play/p/6gLQTyOMg5o
empty string cannot be compared with nil. Not sure what I am missing here
There was a problem hiding this comment.
Oh sorry what I meant was we need to keep this check when the key is non-empty.
i.e. if len(c.SignerPrivateKeyHex) > 0 && len(c.SignerPrivateKeyHex) != 64 then return error
| @@ -107,6 +107,28 @@ func (c *disperserClient) DisperseBlob(ctx context.Context, data []byte, quorums | |||
| } | |||
|
|
|||
| func (c *disperserClient) DisperseBlobAuthenticated(ctx context.Context, data []byte, quorums []uint8) (*disperser.BlobStatus, []byte, error) { | |||
| // first check if signer is valid | |||
| accountId, err := c.signer.GetAccountID() | |||
There was a problem hiding this comment.
check if c.signer == nil?
There was a problem hiding this comment.
after some reflections, we could have just assign the signer interface as nil if privateKeyHex is not provided. This make NoopSigner impl looks like a over-do. But I kind of like noopSigner because it complies to the interface, and leave less space for error.
There was a problem hiding this comment.
my main thing about it is because the signer is a interface, if it is a struct pointer, I will fall immediately after nil check
There was a problem hiding this comment.
Yeah shouldn't it fail immediately if c.signer is nil?
There was a problem hiding this comment.
I did it, and the commit actually check it.
Not sure why github won't show it.
api/clients/eigenda_client.go
Outdated
| var signer core.BlobRequestSigner | ||
| if len(config.SignerPrivateKeyHex) == 64 { | ||
| signer = auth.NewLocalBlobRequestSigner(config.SignerPrivateKeyHex) | ||
| } else { |
There was a problem hiding this comment.
only do this when config.SignerPrivateKeyHex is nil, and return error when config.SignerPrivateKeyHex is anything other than nil or has length != 64
There was a problem hiding this comment.
I can add the error case, but config.SignerPrivateKeyHex is a string, what I am missing here, there is no reason to check nil
There was a problem hiding this comment.
I want to make sure we check the key length. If this key is provided (non-empty string), it should be length 64 and error out otherwise.
There was a problem hiding this comment.
yes, pushed that change
api/clients/disperser_client.go
Outdated
| // first check if signer is valid | ||
| accountId, err := c.signer.GetAccountID() | ||
| if err != nil { | ||
| return nil, nil, err |
There was a problem hiding this comment.
would it be even more clear if we add something like please configure signer key if you want to use authenticated endpoint?
Feature addition
Method
Test with proxy
Send without privateKey
Send with privateKey (i.e it is working)
retrieve without privatekey (working)
Checks