added method filtering to arc58 plugin rekey methods #13
Conversation
…ized for single transaction group loop on all checks & added jest config fix for bigint serialization bug
…ts to avoid extra compute & reorganize asserts for better error messaging
There was a problem hiding this comment.
So far I've only only reviewed the contract. Main feedback is that it's not really clear to me how the method indexes are used. I think breaking up the complex binary expressions and documenting with some examples might help.
Also not specific to this review but I'm starting to not like the use of global and local for defining the allowed called. Perhaps open and restricted or something along those lines? Could be address in a seperate PR though.
Finally the main thing I'd want to see before merging and putting this in the ARC is a reference implementation for parsing the TEAL and displaying what individual methods can do. I recognize this is non-trivial but I think it's essential that we have a way to display to a user exactly what they might be getting themselves into. We could just parse the entire TEAL for individual methods and not get grnaular, but I'm worried that will teach users to just ignore the automated parsing and erroneously trust contract authors/front-ends.
| methods: bytes<4>[]; | ||
| }; | ||
|
|
||
| type CallerUsed = { |
There was a problem hiding this comment.
Nit, but using uint8 values would be more efficient. Also avoids the implication that both can be true
const GLOBAL_CALLER = <uint8>1
const LOCAL_CALLER = <uint8>2| ); | ||
| } | ||
|
|
||
| private ensuresRekeyBack(txn: Txn): boolean { |
There was a problem hiding this comment.
Nit, but I think a name like txnRekeysBack is a more descriptive name for this function. ensures just feels like the wrong word here but might just be me.
| const txn = this.txnGroup[i]; | ||
|
|
||
| if (this.ensuresRekeyBack(txn)) { | ||
| rekeysBack = true; |
There was a problem hiding this comment.
We can return here to avoid going over more txns than needed
| if (txn.sender === this.app.address && txn.rekeyTo === this.app.address) { | ||
| rekeyedBack = true; | ||
| break; | ||
| if (this.ensuresRekeyBack(txn)) { |
There was a problem hiding this comment.
Just noting that this will end up using more ops because the callsub... proto... retsub... for each call, but I imagine this will be re-implemented in PuyaTS by the time the ARC is finalized which will support in-lining
| } | ||
|
|
||
| const globalValid = ( |
There was a problem hiding this comment.
This is pretty tough to read. I think it's preferable to put multiple bools in a var first that describes each condition.
| ) | ||
| ); | ||
|
|
||
| const localValid = ( |
|
|
||
| let rekeysBack = false; | ||
|
|
||
| const globalRestrictions = checkGlobal && this.plugins(gkey).size > 29; |
There was a problem hiding this comment.
I'm confused on why the size is being checked here
| */ | ||
| private verifyRekeyToAbstractedAccount(): void { | ||
| let rekeyedBack = false; | ||
| private assertValidGroup(app: AppID, methodOffsets: uint64[], checkGlobal: boolean, checkLocal: boolean): CallerUsed { |
There was a problem hiding this comment.
methodOffsets needs to be documented. After reading the code it's also not really clear to me how it's being used
| @@ -173,34 +304,71 @@ export class AbstractedAccount extends Contract { | |||
| * Temporarily rekey to an approved plugin app address | |||
| * | |||
| * @param plugin The app to rekey to | |||
| * @param methodOffsets The indices of the methods being used in the group | |||
There was a problem hiding this comment.
Honestly I'm still kinda confused on how methodOffsets is used
This PR adds support for restricting which methods are allowed to be called for a given plugin key.
Additional changes:
algokit-utils-tsversion 7tealscriptto 0.106.1