feat(agglayer): correct script allowlists#3335
Conversation
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you for fixing this!
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good!
Minor nit (feel free to ignore): Looking forward, we will need to integrate parts or all of the fee manager into AuthNetworkAccount in the future, meaning it will require more parameters to build this component (fee policy, fee asset ID). I think it might end up being nicer for ergonomics if we keep a builder-style approach for building AuthNetworkAccount rather than passing all of these parameters through NetworkAccount::builder. I believe this change was mainly about including the ExpirationTransactionScript root into the allowlist by default? Since AuthNetworkAccount itself reflects the requirements of the network tx standard, we could also add this script by default when building AuthNetworkAccount.
mmagician
left a comment
There was a problem hiding this comment.
Thanks for providing such a comprehensive fix! ✅
Avoids the implicit From conversion so call sites in NetworkAccount::new stay explicit about which allowlist failed to decode.
Discovered by @juan518munoz here: the agglayer integration tests were not working on the client because the network accounts were not allowing the script that the NTX builder invariably uses.
This PR adds:
NetworkAccountNetworkAccountvalidates that the expiration script root is allowed by it. Right now the NTX builder always attaches this script, so it makes sense to make it part of the standardNetworkAccount::builder()which returns an account builder seeded with the correct componentsNetworkAccount::try_fromto pick up allowlists with the expiration script (this is used by the node to pick up network accounts so it should avoid picking up unusable network accounts)These changes are somewhat opinionated as all we really needed was to add the correct script to the allowlists, so we can discard whatever we think is unnecessary (and also, maybe there were more specific plans for this). In short, it makes
NetworkAccounta more specific of a standard.This will require a new release