GH-2463: Provide smart contract access to associated keys that signed a deploy #2468
Conversation
mpapierski
requested review from
Fraser999,
darthsiroftardis and
EdHastingsCasperAssociation
mpapierski
force-pushed
the
gh-2463-deploy-signatures
branch
from
215d0c3
to
6bc1c1d
Compare
mpapierski
force-pushed
the
gh-2463-deploy-signatures
branch
from
94af17d
to
427abc2
Compare
|
good start, quick turn around, but see notes |
EdHastingsCasperAssociation
changed the title
| Vec::from_iter(self.context.authorization_keys().clone().into_iter()); | ||
|
|
||
| let total_keys = authorization_keys.len() as u32; | ||
| let total_keys_bytes = total_keys.to_le_bytes(); |
There was a problem hiding this comment.
Shouldn't we be using to_bytes() instead? It will call to_le_bytes() on u32 as well but we would be using a higher-level API that we had defined ourselves.
There was a problem hiding this comment.
Currently, bytesrepr represents u32 as four little-endian bytes, but it may not be the case in the future. The intention is to write u32 as LE bytes under a pointer, rather than 'ABI serialized u32 integer', which is different (and more expensive). This approach also saves us from calling bytesrepr::deserialize on this memory.
There was a problem hiding this comment.
So what are the rules about this? When do we choose ToBytes/FromBytes and when we do not?
There was a problem hiding this comment.
When you need to pass a single value of a type statically known on the Wasm side like a pointer to u32, then write to_le_bytes(), but if you need something more complex like Vec<AccountHash>, you should consider bytesrepr. Historically we weren't always careful, and we overused bytesrepr a lot on the Wasm side, but I hope to address this in 2.0 - for now, I'm trying to stay consistent and pass plain values as LE bytes
- Replaces `authorized-keys` test contract with `set-action-thresholds` to avoid duplication of test code - Few small renames
smart_contracts/contracts/test/list-authorization-keys/src/main.rs
Outdated
Show resolved
Hide resolved
|
My comment (#2468 (comment)) still stands. Why are we not exposing keys instead? They're more valuable to the smart contracts than hashes (one can always go key -> hash but not the other way around). |
Consistency with other associated keys APIs with |
If exposing keys is superior functionality we shouldn't disregard it b/c we've exposed hashes until now. |
Can we address this is in separate work perhaps? I have some concerns with exposing |
|
I agree with @goral09 here: #2468 (comment) @mpapierski @piotr-dziubecki can we create a ticket and plan for this? |
|
@mpapierski any actions you need to take before merging the PR? |
|
bors r+ |
I just notified Piotr about exposing PublicKeys in future work (2.0.0 perhaps). Merging now |
|
Build succeeded: |
| @operator(">") | ||
| graterThan(other: AccountHash): bool { | ||
| for (let i = 0; i < 32; i++) { | ||
| if (this.bytes[0] > other.bytes[0]) { |
There was a problem hiding this comment.
Should that be i instead of 0? (Also in lowerThan.)
Closes #2463
This PR adds
list_authorization_keysto the contract_api / wasm ffi, allowing smart contracts access to the set of associatedAccountHashes that signed a deploy.This is necessary for certain types of more advanced smart contracts that utilize multi signature accounts and need to impose some form of role based security in their app logic. For instance, a smart contract that has defined a "clerk", a "supervisor", and "registrar" roles and contains certain entrypoints or operations that are only allowed if one or more signatories of specific role(s) have signed a deploy.