-
Notifications
You must be signed in to change notification settings - Fork 141
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
Extend isAuthorized to process current transaction in HAS System Contract #792
base: main
Are you sure you want to change the base?
Extend isAuthorized to process current transaction in HAS System Contract #792
Conversation
…n in HAS System Contract Signed-off-by: Brendan Graetz <bguiz@users.noreply.github.com>
✅ Deploy Preview for hedera-hips ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
1157da5
to
e62adc7
Compare
Signed-off-by: Brendan Graetz <bguiz@users.noreply.github.com>
e62adc7
to
c26b90c
Compare
Signed-off-by: Brendan Graetz <bguiz@users.noreply.github.com>
Signed-off-by: Brendan Graetz <bguiz@users.noreply.github.com>
Signed-off-by: Brendan Graetz <bguiz@users.noreply.github.com>
Signed-off-by: Brendan Graetz <bguiz@users.noreply.github.com>
Signed-off-by: Brendan Graetz <bguiz@users.noreply.github.com>
bb29bae
to
2fae253
Compare
Signed-off-by: Brendan Graetz <bguiz@users.noreply.github.com>
2fae253
to
652fa26
Compare
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.
Thanks fro the proposal.
Left some questions and comments
|
||
- When an account's admin key is "complex", and is comprised of multiple EdDSA keys and/or ECDSA keys and/or **smart contract IDs**, combined using one or more `KeyList`s or `ThresholdKey`s. | ||
|
||
The proposed `hederaAccountService.isAuthorizedCurrentTransaction()` method aims to fulfil the above scenario, and specific use cases pertaining to this scenario will be elaborated upon in the [use cases section](#use-cases) below. |
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.
@bguiz is the goal to check if the calling smart contract satisfies the last key slot of the calling EOA's complex key (keyList or threshold)?
That is what is really being asked is "Given a collection of signatures, does making a call from this contract satisfy the the required keys for the transaction origin caller?"
This is most an informative query and doesn't map to a specific transaction but rather the initiating EOA authorization?
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 necessarily last key slot. consider these scenario where:
- the account has a
ThresholdKey([edDsaKey, ecDsaKey, scFooId, scBarId], 1)
- --> transaction is signed using either of the cryptographic keys, does not invoke either of the smart contracts --> ✅
- --> transaction is signed using either of the cryptographic keys, does not invoke either of the smart contracts --> ✅
- --> transaction is unsigned using either of the cryptographic keys, but invokes smart contract
scFooId
--> ✅ - --> transaction is unsigned using either of the cryptographic keys, but invokes smart contract
scBarId
--> ✅
- the account has a
ThresholdKey([edDsaKey, ecDsaKey, scFooId, scBarId], 2)
- --> transaction is unsigned using either of the cryptographic keys, but invokes smart contract
scFooId
--> ❌ - --> transaction is unsigned using either of the cryptographic keys, but invokes smart contract
scBarId
--> ❌ - --> transaction is signed using either of the cryptographic keys, but invokes smart contract
scFooId
--> ✅ - --> transaction is signed using either of the cryptographic keys, but invokes smart contract
scBarId
--> ✅
- --> transaction is unsigned using either of the cryptographic keys, but invokes smart contract
- the account has a
ThresholdKey(ThresholdKey([edDsaKey, ecDsaKey], 1), ThresholdKey([scFooId, scBarId], 1), 2)
- --> transaction is signed using either of the cryptographic keys, and invokes either smart contract --> ✅
- --> transaction is not signed using either of the cryptographic keys, and invokes either smart contract --> ❌
is the goal to check if the calling smart contract satisfies the last key slot of the calling EOA's complex key (keyList or threshold)?
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, the idea is whether the current transaction is authorised according to the Key
, KeyList
, or ThresholdKey
of the account that has submitted a smart contract transaction. (Where the Account is the equivalent of the from
field of the EthereumTransaction
, or equivalent).
This is most an informative query and doesn't map to a specific transaction but rather the initiating EOA authorization?
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 only your last scenario should ever return true. Since SCs don't actually sign and their authorization is based on the matching contract making the call there's no way to authorize if an operation needs more than 1 contractId to match and if all the necessary cryptographic keys haven't signed.
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, precisely - I was enumerating different examples of when a smart contract ID may not be in the last key slot of an account's complex key, and where there may be more than one contract ID in an account's complex key.
Since SCs don't actually sign and their authorization is based on the matching contract making the call there's no way to authorize if an operation needs more than 1 contractId to match and if all the necessary cryptographic keys haven't signed.
|
||
(1) All authorization is performed using cryptographic signatures (EdDSA and/or ECDSA in the case of Hedera). | ||
|
||
(2) All authorization is performed "off-chain", that is among clients, and not within (or by) smart contracts. |
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 you mean all signing here. The network will still check for authorization
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.
Yeah exactly. So authorisation checks are performed by the nodes on the network - that of course does not change. But the "thing that generates the authorisation" is what is referred to here. EdDSA keys and ECDSA keys here, and below dDSA key, ECDSA keys, and smart contract functions which invoke the hederaAccountService
system contract.
DO you mean all signing here. The network will still check for authorization
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.
Okay you should say signing then. Saying authorization is done off-chain might give the wrong impression
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.
Hmm, the reason I use the word authorisation over signing is because the ECDSA and EdDSA keys can sign a transaction, but a smart contract ID does not. So I'm using a term that is inclusive of both "cryptographic signature based authorisation" and "smart contract invocation based authorisation".
Is there perhaps another term (other than "authorisation") which covers both of these?
|
||
The authorization methods described in HIP-632 are sufficient to support | ||
the "complex" keys on accounts, as described above, with one exception: | ||
When the "leaf nodes" of a "complex" key include one or more **smart contract IDs**. |
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 elaborate on where more than 1 contract ID is part of the key and needed for auth.
Indeed the system wouldn't support this at all. The exception might be the direction that HIP 755 is leaning in capturing auth from contracts asynchronously.
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.
In agreement - I don't think that we need to support this scenario.
That being said, there are scenarios in which smart contract IDs could appear in an account's keys more than once which should be supported, see my comment above starting with "not necessarily last key slot. consider these scenario where..."
Can you elaborate on where more than 1 contract ID is part of the key and needed for auth.
Indeed the system wouldn't support this at all.
I say the above without having first read HIP-755, will need to check that out shortly.
The exception might be the direction that HIP 755 is leaning in capturing auth from contracts asynchronously.
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.
Okay makes sense.
Also when i say last key slot i don't mean in order of the actual key structure, but rather the ContractID has to be the last remaining authorization needed. If not the auth check should always return false.
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, gotcha, your previous comment makes more sense now 😅
Also when i say last key slot i don't mean in order of the actual key structure
Yes exactly, this is what I intend to say here too!
but rather the ContractID has to be the last remaining authorization needed.
- implement batch processing of multiple transactions | ||
|
||
(2) | ||
As a user of Hedera networks, |
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 user story is already possible no, is there an edge case where this HIP proposes additional capability to create an account with this setup?
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.
Currently this is only possible - that I've been able to verify at least - with the HTS system contract,
being the only system contract that exposes non-view
functions.
Whether or not this is possible depends on the implementation on HIP-632,
which is a pre-requisite for this HIP.
Also, even with HIP-632 implemented, this would still be blocked from happening,
because Solidity/ EVM op codes only expose a subset of transaction information,
which is insufficient to determine whether or not the current transaction is authorised.
This is the critical difference between isAuthorized
and isAuthorizedCurrentTransaction
.
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.
To clarify i'm saying it's possible for a contract to perform actions on behalf of an account if the contractId satisfies the account key requirement e.g. key is ContractID or 1 of 2 threshold with ContractId as 1 option as you've shown in some examples.
This 2nd User Story makes it seem like it's not possible and this HIP would make it possible.
Might need rewording or removal.
|
||
| hash | signature | return | description | | ||
| --- | --- | --- | --- | | ||
| | isAuthorizedCurrentTransaction() | bool | `true` if account is authorized to carry out transaction execution on account. Accepts protobuf key signature blobs. May be used for ECDSA, EdDSA simple key flows, and complex key flows which include any of ECDSA keys, EdDSA keys, and smart contract IDs. | |
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 add the selector hash
|
||
| hash | signature | return | description | | ||
| --- | --- | --- | --- | | ||
| | isAuthorizedCurrentTransaction() | bool | `true` if account is authorized to carry out transaction execution on account. Accepts protobuf key signature blobs. May be used for ECDSA, EdDSA simple key flows, and complex key flows which include any of ECDSA keys, EdDSA keys, and smart contract IDs. | |
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.
In terms of naming what should CurrentTransaction
imply and why does it matter?
There's no reliable way for the system to know what inner transaction is implied without the contract actually intending it.
If my understanding is correct it's not about a specific parent transaction but more so if an EOA's (tx origin) public key authorization requirements will be satisfied by this contract making a call?
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 exactly, it is only about whether the authorisation requirements stipulated by the account's key has been satisfied.
If my understanding is correct it's not about a specific parent transaction but more so if an EOA's (tx origin) public key authorization requirements will be satisfied by this contract making a call?
Yes, and this is not needed, only the main transaction is of concern, not that of inner/child transactions.
There's no reliable way for the system to know what inner transaction is implied without the contract actually intending it.
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.
Okay.
Becoming clearer.
The naming doesn't point to the focus on the EOA (tx origin) signing requirements. It currently suggests there's a transaction or operation to be carried out which you may want to check is possible.
That's how it reads to me.
Without caring about length and describing the intent I'd consider isCallContextTxOriginAuthorized()
, now I just have to figure out how to be more concise.
Naming is hard! 🤔
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.
Yeah, open to renaming if it makes it easier to understand/ document!
Without caring about length and describing the intent I'd consider isCallContextTxOriginAuthorized() ,
This function behaves identically to `isAuthorized(address, messageHash, signatureBlob)` as defined in HIP-632, with the following key differences: | ||
|
||
- It is called without specifying any parameters | ||
- This function extracts the values that it needs in order to validate if a transaction is authorized from the current transaction |
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.
Please be explicit about which values are extracted and from where in the submitted HAPI transaction since you aren't utilizing any function params
- It is called without specifying any parameters | ||
- This function extracts the values that it needs in order to validate if a transaction is authorized from the current transaction | ||
- Therefore it designed to be used exclusively on the current transaction, which is still in-flight | ||
- This is clearly communicated by the `CurrentTransaction` suffix in the function name |
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 above that this may not be as clear as intended. The current transaction will be a ContractCall
/ContractCreate
/EthereumTransaction
as that's the submitted HAPI transaction.
How would the system know whether the contract intends to next execute a transfer next.
Actually right now the only thing a contract could do on behalf of an EOA is transfer tokens using a ContractID. Maybe in the future and with an expanded H.A.S system contract there might be more
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 indeed. As noted above, the HTS system contract is currently the only one that exposes a non-view
function (that I'm aware of), and hence this is the case.
Actually right now the only thing a contract could do on behalf of an EOA is transfer tokens using a ContractID.
Exactly! (and I'd consider this HIP to the first requested expansion of the HAS system contract)
Maybe in the future and with an expanded H.A.S system contract there might be more
which uses `TxnAwareEvmSigsVerifier` to determine that `tx` is authorised, because: | ||
- `tx` has been signed by `edDsaKey`, which is a member of account `0.0.12345`'s `ThresholdKey` | ||
- `tx` has also been signed by `ecDsaKey`, which is a member of account `0.0.12345`'s `ThresholdKey` | ||
- `tx` invokes `customSc`, which is a member of account `0.0.12345`'s `ThresholdKey` |
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.
In a sense it's a readiness check for this contract being the final authorizer correct?.
With the cryptographic key signatures are not provided in the parent transaction and if there are more than 1 contractIDs then this method will always return false.
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.
Given this, a contract would well have attempted the transaction and based on a success or error response code it would know.
The difference here would be contract flow and maybe cost in the query cost vs the failed transaction.
Given the current gas strategy the incentive for this seems to be more around cost efficiency
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 cryptographic keys are not provided, and this key requires 3-of-3,
we know (before even getting to the smart contract invocation) that the best case scenario is to get to 1-of-3,
so this would ultimately be destined to unauthorised.
In such a case (and others of a similar vein) the implementation can perform a pre-check, and "fail fast".
With the cryptographic key signatures are not provided in the parent transaction and if there are more than 1 contractIDs then this method will always return false.
I think this would apply for an account with a different key requirement, for example ThresholdKey([edDsaKey, ecDsaKey, scId], 1)
.
In such a case, if the transaction is not signed by any of the cryptographic keys,
since the requirement is 1-of-3, and it is still possible to get to 1-of-3 by executing the smart contract,
then it would need to proceed with the execution attempt.
Given this, a contract would well have attempted the transaction and based on a success or error response code it would know.
Yes, you're right - I haven't quite considered execution cost (and optimisation) yet.
I think we can use the cost of the verification that the HTS system contract presently has as a starting point.
The difference here would be contract flow and maybe cost in the query cost vs the failed transaction.
Given the current gas strategy the incentive for this seems to be more around cost efficiency
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 would apply for an account with a different key requirement, for example ThresholdKey([edDsaKey, ecDsaKey, scId], 1).
In such a case, if the transaction is not signed by any of the cryptographic keys,
since the requirement is 1-of-3, and it is still possible to get to 1-of-3 by executing the smart contract,
then it would need to proceed with the execution attempt.
Correct what i mean is the check will only even return true if all cryptographic requirements are met whether 0, 1 or n.
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.
Other point is why query when if the only time you're get true is if you were actually authorized.
Only other consideration is price but that might be case by gas based on gasLimit and how expensive the operation is vs the query.
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.
Querying ahead is nice if possible in many scenarios though.
- When an account's admin key is "complex", and is comprised of multiple EdDSA keys and/or ECDSA keys, combined using one or more `KeyList`s or `ThresholdKey`s. | ||
|
||
`hederaAccountService.isAuthorized(address, messageHash, signatureBlob)` does **not** work for the following scenarios: | ||
|
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 not exactly clear as to why this scenario would not work with isAuthorized
in HIP-632. The signature blob is a protobuf serialized SignatureMap
which I believe supports adding contract keys.
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 idea here is that the client invoking the smart contract does not need to pass in signatureBlob
, and that any values that it needs, including signatureBlob
are read from the current transaction.
- implement advanced multisig use cases without the need to split across multiple transactions | ||
- implement atomic multisig use cases | ||
- implement batch processing of multiple transactions | ||
|
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 batch processing use case is clear to me but the others less so. Are there more concrete examples of why this function is necessary to enable those use cases as opposed to simply failing if the signature is insufficient?
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 batch scenario is actually an interesting one where let's say there are 3 accounts,
AccA --> ThresholdKey([ecDsaKeyA, customSc], 1)
AccB --> ThresholdKey([ecDsaKeyB, customSc], 1)
AccC --> ThresholdKey([ecDsaKeyC, customSc], 1)
They could individually sign 3 separate transactions with their cryptographic keys.
Let's say that customSc
can perform that same transaction when passed in an array of account addresses.
Since all these accounts require 1-of-2, and customSc
's smart contract ID is among their keys, those indiidual transactions can be performed in a single transaction as a batch.
(To be clear, this can also be accomplished with isAuthorized
and isAuthorizedRaw
from HIP-632 too, with clients taking a different approach)
Co-authored-by: Nana Essilfie-Conduah <56320167+Nana-EC@users.noreply.github.com> Signed-off-by: Brendan Graetz <bguiz@users.noreply.github.com>
Just to be clear (to me!): This is not a method that should be exposed via the HAS proxy - where you call an account's address - because you're not trying to see if some arbitrary account authorizes this transaction, but the specific account that sent the transaction? Or maybe not? Is there a use case for pointing at a specific account and asking: Did this account authorize this transaction taking into account contract id keys? Or is that just |
Description:
This PR adds a new HIP, which augments HIP-632
WIP:
Related issue(s):
Nil
Notes for reviewer:
Hedera Account Service (HAS) System Contract
Checklist