-
Notifications
You must be signed in to change notification settings - Fork 28
feat: market 2.0 #508
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
base: main
Are you sure you want to change the base?
feat: market 2.0 #508
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.
Just reviewing types for now, didn't look at much beyond that, but generally things make sense.
6d720c7
to
0e6bd97
Compare
35ba248
to
a2e3a77
Compare
|
00870ce
to
92e644b
Compare
func verifySignature(db *harmonydb.DB, keyType string, pubKey, signature []byte, cfg *config.CurioConfig) (bool, string, error) { | ||
now := time.Now().Truncate(time.Hour) | ||
minus1 := now.Add(-59 * time.Minute) | ||
plus1 := now.Add(59 * time.Minute) |
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.
Why 59 minutes instead of 1 hour?
Go playground
I believe the intent is to allow messages from the previous and next hour, giving a validity duration between 2 and 3 hours. But by using 59 minutes instead, the validity duration is between 0 and 1 hours, which could also be achieved by only checking one msg.
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.
We can change it to 1 hour. I should be fine. This was just to limit the time for testing period. Since, we are planning to switch to session keys, this would hold less significance as timestamp signing will move to original client signing the Auth for session key side. It allows us to allow auth with some expiry period.
} | ||
|
||
var allowed bool | ||
err := db.QueryRow(ctx, `SELECT EXISTS (SELECT 1 FROM market_mk20_clients WHERE client = $1 AND allowed = TRUE)`, client).Scan(&allowed) |
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.
If we don't enable the whitelist with DenyUnknownClients
, can we still have a blacklist? I imagine some sp's may want to blacklist bad actors but might not want a strict whitelist.
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.
List is flexible. You can mark a client allow/deny without separate lists.
return "", nil, nil, errors.New("missing CustomAuth prefix") | ||
} | ||
|
||
parts := strings.SplitN(strings.TrimPrefix(header, Authprefix), ":", 3) |
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.
if the prefix had a colon instead of a space, this step would be slightly simpler.
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.
We can switch. Again, this is just basic Auth for testing purposes.
PR is ready to review
Test contractForce PoRep interfaceUpdate retrieval to work with MPD for PDPPDP pipeline UIDocsDelete old PDP tablesWrite deal validation logic testsNotification pathwayTests: