-
Notifications
You must be signed in to change notification settings - Fork 23
feat: Endorsement Certificates #443
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: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Jakub Sztandera <[email protected]>
Signed-off-by: Jakub Sztandera <[email protected]>
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
synapse-dev | 72047d7 | Commit Preview URL Branch Preview URL |
Nov 26 2025, 07:50 PM |
| notAfter: bytesToBigInt(data.slice(8, 16)), | ||
| signature: bytesToHex(data.slice(16)), | ||
| } | ||
| const address = await recoverTypedDataAddress({ |
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 looked into why this was async (in principle it should not be). It is only because of this async import:
export async function recoverPublicKey({
hash,
signature,
}: RecoverPublicKeyParameters): Promise<RecoverPublicKeyReturnType> {
const hashHex = isHex(hash) ? hash : toHex(hash)
const { secp256k1 } = await import('@noble/curves/secp256k1')
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.
oh, that's nasty, but oh well
Signed-off-by: Jakub Sztandera <[email protected]>
Signed-off-by: Jakub Sztandera <[email protected]>
Co-authored-by: Jakub Sztandera <[email protected]>
Co-authored-by: Rod Vagg <[email protected]>
Co-authored-by: Rod Vagg <[email protected]>
The reason not to move this is that it would then be exported, and I don't think we intend to export it. if you want to export it I can make the change.
@rvagg @hugomrdias let me know if you agree |
| if (hexData.length !== 164) { | ||
| return { | ||
| address: null, | ||
| endorsement: { | ||
| nonce: 0n, | ||
| notAfter: 0n, | ||
| signature: '0x', | ||
| }, | ||
| } | ||
| } |
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 not just throw 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.
I want to reduce use of try/catch for expected situations because it can mask unexpected situations
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 you pls move this to examples/cli and make a command for this
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.
all of those commands in cli are only using calibration. None of them consult env. How do you want those to select the chain?
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.
ah yes will fix !
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.
@hugomrdias are we blocked on that? should we land this in place and then move it later or do we need to resolve #469 first?
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 just dont like random cli files in the core folder we have the utils folder for that and now the examples/cli for more structured cli/nodejs stuff.
but yes we can land this and fix later
| notAfter: EXPIRY, | ||
| }) | ||
| console.log('Provider:', providerId) | ||
| console.log('Endorsement:', encoded) |
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.
TODO: curio interface for hex
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.
|
|
||
| const PRIVATE_KEY = process.env.PRIVATE_KEY | ||
| const ETH_RPC_URL = process.env.ETH_RPC_URL || 'https://api.calibration.node.glif.io/rpc/v1' | ||
| const EXPIRY = process.env.EXPIRY || BigInt(Math.floor(Date.now() / 1000)) + 10368000n |
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.
comment that this is (now + 4 months)
| } | ||
|
|
||
| const PRIVATE_KEY = process.env.PRIVATE_KEY | ||
| const ETH_RPC_URL = process.env.ETH_RPC_URL || 'https://api.calibration.node.glif.io/rpc/v1' |
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 these should be ??
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 strictly necessary but would provide clearer error cases - if you set ETH_RPC_URL to something falsy then it's going to use the default where it probably should error, e.g. "can't connect to RPC provider '0'
| } | ||
| } catch { | ||
| // Skip invalid endorsements | ||
| } |
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 we can remove this catch now
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.
Would need to make sure that signatures that fail to recover don't throw. Should have a test case covering this in case that behavior changes.
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.
👍 looks to me like a failure here would be of the fatal kind now since you're doing so much in decodeEndorsement
| const signature = await signTypedData(client, { | ||
| account: client.account, | ||
| domain: { | ||
| name: 'Storage Endorsement', |
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 be a const
| (results: [ProviderInfo[], ProviderInfo[]], provider: ProviderInfo) => { | ||
| results[ | ||
| preferEndorsements.some( | ||
| (endorsement: Address) => endorsement in (provider.products.PDP?.data.endorsements ?? {}) |
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.
could be cleaned up by pulling provider.products.PDP?.data.endorsements ?? {} into a local variable inside the arrow func, then we might be able to avoid the multi-line mess that's making this hard to read
|
Overall fine by me, just a few minor suggestions in there and one major concern to be resolved regarding the CLI tool. |
rvagg
left a comment
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.
Blocked by decision @ #540
Comes down to resourcing, do we spend the time to implement an alternative and can we afford the time for that.
Much of this work can be reused with the Endorsement Registry approach. |
Co-authored by @Kubuxu
Reviewer @rvagg @hugomrdias
Context
For #312, when we are selecting the providers, we prefer that at least one provider has a prime endorsement.
If there aren't any prime endorsements, we do not panic.
We will surface these endorsements as product capabilities in the service provider registry.
Changes
nullon failure instead of throwing an exceptioncreateContextandcreateContextstopreferEndorsementsinsmartSelectProviderif no other providers have been selectedref: FIL-13, FIL-14