artblocks-contracts
artblocks-contracts copied to clipboard
ArtBlocks
Updated Art Blocks Engine spec (V3)
This PR updates the Art Blocks Engine template to make use of the benefits of V3.
This PR does not satisfy all outstanding TODOs in the CHANGELOG (to be addressed in follow on PRs), nor does it update our ENGINE_FLEX or PRTNR templates (also to be addressed in follow on PRs).
Diff-log between GenArt721CoreV3_Engine and GenArt721CoreV3: https://www.diffchecker.com/nPTETDlP
Diff-log between IGenArt721CoreContractV3 and IGenArt721CoreContractV3_Engine: https://www.diffchecker.com/2Ug02HxI
- To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1203222636168458
Definitely not ready for review yet, but up on Github for visibility / discussion (and so that I don't loose it again if I have another computer get rekd... 😅 ).
@ryley-o This is definitely not in a merge-ready state (most importantly haven't yet added tests) but I do think that it is in a review ready state from the perspective of asking for you to TAL over the set of diffs captured above (and the changelog) and making sure that we are directionally on the same page about the above before getting deeper into test coverage.
LMK if this plan sounds good to you, and if so if you could do an initial review as an initial a sign off before I start getting into writing tests, that would be super appreciated.
LMK if this plan sounds good to you, and if so if you could do an initial review as an initial a sign off before I start getting into writing tests, that would be super appreciated.
Will take a look now!
In the process of writing tests today, I realized that the new Engine contract as currently written exceeds deployment size limits... going to need to dive a little bit deeper and loop back to sort out if this means that the required digging makes it totally unreasonable (was already a stretch) to try to get the desired Collaborations contracts deployed before EOY 😓
In the process of writing tests today, I realized that the new Engine contract as currently written exceeds deployment size limits... going to need to dive a little bit deeper and loop back to sort out if this means that the required digging makes it totally unreasonable (was already a stretch) to try to get the desired Collaborations contracts deployed before EOY 😓
I've resolved the issue – a by-EOY deployment is still potentially possible. 😓 😅
Still working on tests–once those are done and there is proper coverage + peace of mind from said coverage, will update the Diffchecker diffs above between V3 and V3_Engine.
- Update the EIP-173-conforming `owner()` method to proxy the `superAdmin` value for the currently assigned ACL contract, rather than returning the contract itself.
@ryley-o @yoshiwarab based on a conversation that Ben and I were having based on some weird behavior we realized was implied both in this PR and over in #395, we've decided to remove this behavior from the _Engine fork and from #395.
tl;dr: It is a better division of responsibility to not leak the implementation details of the ACL management contract into the Ownable behavior within the Core.
Updated the diffchecker diff log to account for https://github.com/ArtBlocks/artblocks-contracts/pull/361/commits/276460c67dcf1e0660f3f8097673d9d8b1c931a6
V3 engine specific tests have been added in https://github.com/ArtBlocks/artblocks-contracts/pull/361/commits/73f958aebe6376a6144ea3c0dab9bfcc35395203
This is not 100% coverage fully comprehensive of every code path, but it is comprehensive enough coverage that I feel comfortable merging this PR and follow up with a pre-EOY yeet-deploy of V3 Engine (contract only, infra work will come in '23) for BMG, Pace, and an internal use case.
Mergin' will send separate PR will deployment scripts as an isolated smol PR.
This is wonderful 🙏 Gave this a look over, and everything LGTM. Some minor nits in a follow-on #416 (Consider this my approval ✅ since I cannot approve an already-merged PR) Cc @jakerockland