-
Notifications
You must be signed in to change notification settings - Fork 2
Add endpoints for event based decryption triggers #79
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?
Conversation
Note: it changed from 'use 3rd party tooling' but does not mention, that the provided compiler implementation is opinionated, and 3rd party tooling could provide more flexible event triggers. Namely triggers that allow for some parameters to be omitted, e.g. topic0
… get_data_for_encryption endpoint
| ) *CryptoService { | ||
| return &CryptoService{ | ||
| CryptoUsecase: usecase.NewCryptoUsecase(db, contract.ShutterRegistryContract, contract.KeyperSetManagerContract, contract.KeyBroadcastContract, ethClient, config), | ||
| CryptoUsecase: usecase.NewCryptoUsecase(db, contract.ShutterRegistryContract, contract.ShutterEventRegistryContract, contract.KeyperSetManagerContract, contract.KeyBroadcastContract, ethClient, config), |
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.
I understand the API can still be deployed even if the event registry contract isn’t deployed; event endpoints will just fail at runtime with a 500 error. Nice‑to‑have: add a feature flag to enable/disable event triggers while still serving endpoints. If the contract isn’t deployed, return a 503 Service Unavailable error. Not required for the upcoming deployment.
| // @Description Retrieves all the necessary data required by clients for encrypting any message. Supports both time-based and event-based identity computation. If triggerDefinition is provided, the identity will be computed for event-based triggers. Otherwise, it uses time-based identity computation. | ||
| // @Tags Crypto | ||
| // @Produce json | ||
| // @Param address query string true "Ethereum address associated with the identity. If you are registering the identity yourself, pass the address of the account making the registration. If you want the API to register the identity on gnosis mainnet, pass the address: 0x228DefCF37Da29475F0EE2B9E4dfAeDc3b0746bc. For chiado pass the address: 0xb9C303443c9af84777e60D5C987AbF0c43844918" |
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 should clarify here that users are not able to register event-based identities by themselves, and should always use the API address. Please find below a suggested update:
Ethereum address associated with the identity.
Time‑based: use the address that will register the identity (your account if self‑registering, or the API signer address below if you are using the API register endpoint).
Event‑based (triggerDefinition provided): users cannot self‑register because the registry is owner‑only, please use the API signer address below.
Gnosis Mainnet API address: 0x228DefCF37Da29475F0EE2B9E4dfAeDc3b0746bc
Chiado API address: 0xb9C303443c9af84777e60D5C987AbF0c43844918
|
|
||
| identity := common.ComputeIdentity(identityPrefix[:], newSigner.From) | ||
|
|
||
| // TODO: check for already registered identities also against event based triggers! |
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 should implement this cross-check in both time-based and event-based identity registrations to avoid identity collisions where a timestamp trigger could release an event-based identity and vice-versa.
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.
This might not be needed if we compute the identities following my comment on identity calculation.
| return crypto.Keccak256(imageBytes) | ||
| } | ||
|
|
||
| func ComputeEventIdentity(prefix []byte, sender common.Address, triggerDefinition []byte) []byte { |
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 event identity calculation is inconsistent with the calculation in rolling shutter: https://github.com/shutter-network/rolling-shutter/blob/main/rolling-shutter/keyperimpl/shutterservice/eventtriggerregisteredprocessor.go#L123C6-L123C33
To make event triggers safe long‑term, we could:
-
Align identity derivation across repos (update rolling‑shutter to match API), and additionally add a domain separator, and enforce uniqueness on‑chain (requires a contract update). Suggested formula: eventIdentity = keccak(prefix + sender + 0x01 + triggerDefinition) (time‑based stays keccak(prefix + sender)). This ensures time-based and event-based identities don't collide if the trigger definition is empty.
-
Reject empty/0x triggerDefinition for event registrations.
-
Add a mapping in ShutterEventTriggerRegistry to revert on duplicate identity. This prevents event‑event duplicates without relying on API/DB race checks. We currently update on conflict: https://github.com/shutter-network/rolling-shutter/blob/main/rolling-shutter/keyperimpl/shutterservice/database/sql/queries/shutterservice.sql#L31
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.
As discussed: the 0x01 infix is not necessary; an empty triggerDefinition is useless and ignored by the API, therefore the collision case can not happen in the current framework. If - at some later point - the registry contract was opened to the public, then keypers should check for empty trigger definitions and discard such registrations, or the registration contract could disallow empty values (for a little bit higher gas cost).
|
|
||
| identity := common.ComputeIdentity(identityPrefix[:], ethCommon.HexToAddress(address)) | ||
| var identity []byte | ||
| if len(triggerDefinitionHex) > 0 { |
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 should check for 0x here and treat it as an invalid trigger definition
.env.example
Outdated
| METRICS_PORT=4000 | ||
| METRICS_PORT=4000 | ||
| SHUTTER_EVENT_REGISTRY_CONTRACT_ADDRESS= | ||
| WHITELISTED_TRIGGER_CONTRACT_ADDRESSES= |
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.
Maybe add a comment here on how to add addresses to this string, that they need to be comma-separated
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.
As discussed: Whitelisting only certain contracts for event based decryption triggers does not help with reducing or limiting load on keypers. Instead, if there is need for it, we would need to limit the total number of active event based triggers.
Therefore I removed the whitelist feature.
| // - "name": <matching argument name from signature> | ||
| // - "op": <one of: lt, lte, eq, gte, gt> | ||
| // - "number": <integer argument for numeric comparison> | ||
| // - "bytes": <hex encoded byte argument for non numeric matches with 'op==eq'> |
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.
It would be helpful to add a short note to clarify how different params should be passed: indexed params (topics) are eq‑only. For indexed static types (address, uint256, bytes32), pass the hex representation. For indexed dynamic types (string, bytes, arrays), pass keccak256(value) as hex. For non‑indexed uint256, use number with lt/lte/eq/gte/gt. For other non‑indexed types, use bytes with op=eq hex‑encoded value.
internal/usecase/eventtrigger.go
Outdated
| return shs.UintGt | ||
| case "gte": | ||
| return shs.UintGte | ||
| default: |
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.
Unknown ops should return an explicit error here
internal/usecase/eventtrigger.go
Outdated
| } | ||
| lp.ValuePredicate.Op = shs.BytesEq | ||
| lp.ValuePredicate.ByteArgs = [][]byte{Align(val)} | ||
| length = uint64(len([]byte(arg.Bytes)) / 32) |
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.
len([]byte(arg.Bytes)) is the length of the hex string, not the decoded bytes, and would cause offset miscalculation later on.
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.
:o good catch! thanks!
| identity_prefix bytea NOT NULL, | ||
| sender text NOT NULL, | ||
| event_trigger_definition bytea NOT NULL, | ||
| expiration_block_number bigint, |
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.
is there a reason why this is not NOT NULL? The contract seems to always set it.
docs/swagger.yaml
Outdated
| name: identityPrefix | ||
| required: true | ||
| type: string | ||
| - description: 'Ethereum address associated with the identity. For gnosis mainnet, |
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.
Not clear what this address is supposed to be, the registry contract?
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.
I'm also not particular happy with this interface. The address is supposed to reflect the internal signer of the API used (e.g. gnosis main API or chiado API). Imo we should hide this complexity from the user.
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 should either
- derive the address parameter automatically/internally OR
- use the computed identity as parameter
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.
I went with deriving from the configured signing key and removed the address parameter from the endpoint.
| type EventArgument struct { | ||
| Name string `json:"name" example:"amount"` | ||
| Operator string `json:"op" example:"gte"` | ||
| Number int `json:"number" example:"25433"` |
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.
I think this should be a *big.Int (and probably encoded as a decimal string), not an int because of potential overflows
Whitelisting only certain contracts for event based decryption triggers does not help with reducing or limiting load on keypers. Instead, if there is need for it, we would need to limit the total number of active event based triggers.
The parameter is not necessary for event based identities, because it can be derived from signing key.
The specialized func was used in only one place and could produce semantically incorrect results. I inlined the switch statement and added an explicit error case on default.
This implements API endpoints for event based decryption triggers.
WIP/TODO:
TODOcommentsRegisterEventIdentityis untested so far/compile_event_trigger_definition,/register_event_trigger) to rate limitingFixes #74
See also https://github.com/shutter-network/shutter-api?tab=readme-ov-file#1b-register-an-identity-with-event-based-decryption-triggers-wip