-
Notifications
You must be signed in to change notification settings - Fork 169
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.
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 eth accounts to allowlist #175
Add eth accounts to allowlist #175
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, can we add a test like https://github.com/Layr-Labs/eigenda/blob/master/disperser/apiserver/server_test.go#L282?
Yeah, we can, but it will take mocking the GRPC stream so it will be a bit more work. I'll add it later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, left couple questions
disperser/apiserver/server.go
Outdated
// Get the ethereum address associated with the public key. This is just for convenience so we can put addresses instead of public keys in the allowlist. | ||
// Decode public key | ||
publicKeyBytes, err := hexutil.Decode(blob.RequestHeader.AccountID) | ||
if err != nil { | ||
return fmt.Errorf("failed to decode public key (%v): %v", blob.RequestHeader.AccountID, err) | ||
} | ||
|
||
pubKey, err := crypto.UnmarshalPubkey(publicKeyBytes) | ||
if err != nil { | ||
return fmt.Errorf("failed to decode public key (%v): %v", blob.RequestHeader.AccountID, err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we move this validation at the beginning of this method so that we return immediately when AccountID is not something that's expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving
disperser/apiserver/server.go
Outdated
@@ -257,45 +273,73 @@ func (s *DispersalServer) disperseBlob(ctx context.Context, blob *core.Blob) (*p | |||
}, nil | |||
} | |||
|
|||
func (s *DispersalServer) getAccountRate(origin string, quorumID core.QuorumID) (*PerUserRateInfo, error) { | |||
type RateKeys struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we expect to have two different types of the allowlisted account for the same request?
i.e. for one account that has IP A and eth address B, can they specify throughput limit with A and blob limit with B?
for simplicity, why don't we consolidate it so that it has to be from the same type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I think it makes sense to always prefer the authenticatedAddress
if it is in the allowlist, and only check the ip address if no authenticatedAddress
was provided or it was not found in the allowlist.
disperser/apiserver/server.go
Outdated
@@ -128,8 +130,22 @@ func (s *DispersalServer) DisperseBlobAuthenticated(stream pb.Disperser_Disperse | |||
return fmt.Errorf("failed to authenticate blob request: %v", err) | |||
} | |||
|
|||
// Get the ethereum address associated with the public key. This is just for convenience so we can put addresses instead of public keys in the allowlist. | |||
// Decode public key | |||
publicKeyBytes, err := hexutil.Decode(blob.RequestHeader.AccountID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check here and below assume the account id is in hex format ethereum. address. It'd be helpful to make this assumption clear at the interface: https://github.com/Layr-Labs/eigenda/blob/master/api/proto/disperser/disperser.proto#L83
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
disperser/apiserver/server.go
Outdated
|
||
if rateInfo.BlobRate > rates.BlobRate { | ||
rates.BlobRate = rateInfo.BlobRate | ||
keys.BlobRateKey = "address:" + authenticatedAddress |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the key just be authenticatedAddress for both throughput and blob rates? They look the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm going to make this change.
Why are these changes needed?
Adds ETH accounts to the allowlist so we can provision throughput to authenticated requests based on the corresponding ETH account.
Checks