-
Notifications
You must be signed in to change notification settings - Fork 2
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
chore: refactor signature id and failed signatures #194
Conversation
…e/refactor-signature
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.
Lots of good stuff that I'm ready to merge, but I either do not understand or disagree with the core thing in this PR - SignId.
note, that since the near contract also uses |
recovery_id: 0, | ||
}; | ||
|
||
let request_json = format!( | ||
"'{}'", | ||
serde_json::to_string(&json!({"request": request, "response": response})).unwrap() | ||
serde_json::to_string(&json!({"sign_id": sign_id, "signature": signature})).unwrap() |
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.
Note: We have breaking changes in this PR too.
SignId
is now used everywhere instead of request_id. A couple things were made more efficient like no longer passing the part of sign requests around, instead we wholly pass aroundSignRequest
which contains all the info the node needs to do signature generation.SignId
is an id agnostic of any chain, just has rawrequest_id
,payload
andepsilon
.SignArgs
is all the info that is not present in theSignId
such asentropy
,key_version
, andpath
.IndexedSignRequest
is all the info from the indexer which includesSignId
andSignArgs
.SignRequest
has more info that is not present on the chain but only inside the node such asparticipants
andproposer
.ContractSignRequest
sinceSignArgs
basically replaces it.SignatureMessage
now usesSignId
and does not sendContractSignRequest
anymore since it was never used. This info is already indexed by each of the nodes so not necessary to confirm this, but the proposer and presignature id is still something that each node needs to come up with, so that is still being passed around. This is not a breaking serialization change since the messages are forward/backwards compatible as long as the field's names are unique/new which they are with this change.SignQueue
for better management of requests overall. All failed requests are put at the front of the queue as to be process immediately on the next try. Note that retrying uses the same participants, which is not ideal. In a separate PR, I'll have it so that the participants are reselected but the proposer will stay the same. This selection is deterministic where the seed for randomization isentropy + retry_count
.