Update to Cadence v0.14.1, implement new Account Key API #529
Conversation
| case hash.SHA2_384: | ||
| return hash.NewSHA2_384() | ||
| case hash.SHA3_384: | ||
| return hash.NewSHA3_384() |
There was a problem hiding this comment.
Isn't better to leave these 2 out from the fvm?
There was a problem hiding this comment.
What is the reason for leaving them out? We added them to Cadence, so I thought we could support them?
There was a problem hiding this comment.
I am thinking of deprecating them from the crypto library since they are not used anywhere (I'll use them if Cadence needs them). They are also one for the reasons the type crypto.Hash is a slice and not a constant-length array. Their usage is rather rare in applications outside Flow too.
The reason why they existed in the first place is that BLS signatures used them in the first implementation, before switching to KMAC128.
There was a problem hiding this comment.
We can leave them if they were left in Cadence for now.
There was a problem hiding this comment.
I actually thought they were removed from Cadence after the comment here.
There was a problem hiding this comment.
Do you think we should rather just remove the non-SHA2/3-256bit hashes from Cadence?
There was a problem hiding this comment.
Soon. we will need a KMAC for BLS, so we will have a non-SHA2/3-256 hasher for sure.
If the work has been done already, we can keep them. I just want to make sure they remain Cadence tools and won't interfere with flow accounts.
There was a problem hiding this comment.
However, BLS could be removed from Cadence for now if you confirm it's not usable.
| case hash.SHA3_384: | ||
| return runtime.HashAlgorithmSHA3_384 | ||
| case hash.KMAC128: | ||
| return runtime.HashAlgorithmKMAC_128 |
| hashAlgo := RuntimeToCryptoHashingAlgorithm(hashAlgorithm) | ||
| if hashAlgo == hash.UnknownHashingAlgorithm { | ||
| return false, fmt.Errorf("invalid hash algorithm: %s", hashAlgorithm) | ||
| } |
There was a problem hiding this comment.
I see that BLS can be returned by RuntimeToCryptoSigningAlgorithm while there is currently no hasher that supports it. It's probably better to disable it for now till we work on the proper hasher for it.
There was a problem hiding this comment.
Isn't it implicitly disabled as the call of Verify below (https://github.com/onflow/flow-go/pull/529/files#diff-f7f88b8bf4e799b156f025158a797c81b52b0874b07f942a4e6935b39643162cR162 ) will fail?
I don't think we should add explicit checks of what is supported at call-sites (like here), but just return an error in the function that performs the actual logic (Verify below)
There was a problem hiding this comment.
I agree BLS would fail, as you mentioned, if it's used for Flow account keys.
I am wondering about the case where a user writes a contract, uses BLS (since it's supported by Cadence) but is unable to use any hasher that goes with it. Is that scenario possible? From a usability point of view (not security), it would be better to give users the full set of tools.
turbolent
force-pushed
the
bastian/cadence-v0.14.0
branch
from
c6eeccc
to
919831a
Compare
| @@ -612,6 +619,51 @@ func (e *hostEnv) RemoveAccountKey(address runtime.Address, index int) (publicKe | |||
| return e.transactionEnv.RemoveAccountKey(address, index) | |||
| } | |||
|
|
|||
| func (e *hostEnv) AddAccountKey( | |||
There was a problem hiding this comment.
Ideally these logic should be moved into an accountKey handler and all unit tests related to this functionality happens there. and here we just redirect the calls to a account key manager, similar to the way we handle contracts.
There was a problem hiding this comment.
Great idea!
Could we do that in a separate PR, so this one can get sipped and also doesn't get too large?
There was a problem hiding this comment.
yeah, sounds like a good idea, I can take care of that, I have to break several parts into handlers so I can move this as well in a separate PR.
| if e.transactionEnv == nil { | ||
| return nil, errors.New("getting account keys is not supported") | ||
| } |
There was a problem hiding this comment.
is it possible an script need to get account key ?
There was a problem hiding this comment.
Yes, that would be great. How could the accounts (Accounts) be accessed in a script?
There was a problem hiding this comment.
I was mostly asking the question, I was not sure if scripts can call getKeys
There was a problem hiding this comment.
Yeah, it's currently now possible to get key for a public account in a script and it will fail because it is not implemented
|
@tarakby Thank you for the review. Could you please have another look and check the comments? Thank you! |
Joint work with @SupunS 🙌