96 update accusal flow #101

gh-23378 · 2022-11-18T07:15:53Z

No description provided.

…phagus-v2-contracts into 96-update-accusal-flow

…ists

…nteractions, archaeologist generation and registration, and sarco creation. add a few tests for accuse

…rdpartyfacet tests

gh-23378 · 2022-11-18T07:29:18Z

test/facets/helpers/archaeologist.ts

+  archaeologistParams: ArchaeologistParameters
+): Promise<SignerWithAddress> => {
+  // calculate archaeologist's minimum digging fee and free bond in sarquitos
+  const archMinDiggingFeeSarquitos = ethers.utils


Calling quintillionths of a sarco token sarquitos until we have official naming from product. Easy find and replace once we've agreed on terminology.

gh-23378 · 2022-11-18T07:30:26Z

test/facets/third-party-facet.spec.ts

@@ -1,568 +0,0 @@
-import "@nomiclabs/hardhat-waffle";


Deleted the entire file because we didn't have any other third party facet functions being tested here

gh-23378 · 2022-11-18T07:34:34Z

contracts/facets/ThirdPartyFacet.sol

+            for (uint256 i = 0; i < archaeologistAddresses.length; i++) {
+                // if the archaeologist has never been accused, release their locked bond back to them
+                if (!s.sarcophagusArchaeologists[sarcoId][archaeologistAddresses[i]].accused) {
+                     LibBonds.freeArchaeologist(sarcoId, archaeologistAddresses[i]);


In the original version of this function, we don't refund the digging fees allocated to well-behaved archaeologists back to the embalmer. In this PR I've preserved that functionality but I think we ought to provide the full refund because the archaeologists aren't receiving those digging fees if the sarcophagus is being closed down and the embalmer isn't receiving the services they've paid for.

Also I'm wondering, could we not remove the need for this extra loop by doing the freeArchaeologist in the loop on 172 above?
...
No, no we can't. We can only free if we know the sarco is compromised.

How about instead using an in-memory list of goodArchAddresses that is added to during the first loop, then this loop would only go through that list and free them all?

Accuse has always been a loop-loving hell-hole, I remember now.

The first loop only runs through archaeologists that have leaked keyshares and not the full list of archaeologists on the sarcophagus, so unfortunately there's no way to track historical accusations at that stage. Good thought though, I optimized as much as possible but the looping in this function is pretty aggressive anyway.

gh-23378 · 2022-11-18T07:39:22Z

Besides the accuse function itself, the bulk of this PR is in accuse.spec.ts which is collapsed by default and comes up at the very end of the other tests so don't miss it! There's some duplicate functionality between the utilities I've created and the existing fixtures but my goal with these utilities is to break things up into smaller pieces so we can more easily generate single components of our tests. They're a WIP and I'll be adding to them as I knock out some of the other contract tasks.

gh-23378 · 2022-11-18T07:44:20Z

test/facets/thirdparty/accuse.spec.ts

+    });
+  });
+
+  // todo: can an async context be used with mocha?


I don't think that this is something that can actually be done with mocha, but if anybody has suggestions on better ways to organize a set of tests where there's some asynchronous configuration and then several tests that use that configuration I'd like to hear them. Currently, there's a lot of duplication across tests because I'm having to set up the sarcophagus and accusal call over and over again.

Below is an example of what I'd like to be able to write, but context is an alias for describe which cannot take an async callback. We have the option to run mocha with --delay to do some asynchronous configuration before running the suite but I don't think it gives us the organizational advantage that context/describe do.

beforeEach, maybe?

beforeEach would execute code but I'd like to have access to the things being created in the beforeEach within the tests themselves. I think the best I could do would be to pull the common code out into another function and then call that at the start of each it callback. I just really liked the test structure we could have achieved if context worked asynchronously.

I think this is what Kelvin is suggesting

let sarcohpagus: Sarcophagus; ... beforeEach(async () => { sarcohpagus = await getSarcophagus(); ... }) it("should do stuff", () => { // use sarcophagus var })

What you're describing has always been a pain point for me that has made me rip what little hair I have left out of my head. I still haven't come to a satisfactory answer for this, but extracting common code into fixtures is the way I usually go. This is what the current test fixtures are supposed to do, but I'm not sure if they've retained their original purpose over time.

DarksightKellar

Whoo. This is a beefy one, and I'm ashamed to say I didn't read through every line.
The accuse tests are nice and specific, as they should be.

I did start to get the feeling that a number of the new helper functions are already implemented in some form in the existing fixtures. Is the intention to phase them out eventually, or to use both (maybe the new functions solve a different problem)?
EDIT: Never mind I somehow missed your comment saying this exact same thing.

DarksightKellar · 2022-11-18T12:03:13Z

contracts/facets/ThirdPartyFacet.sol

+            for (uint256 i = 0; i < archaeologistAddresses.length; i++) {
+                // if the archaeologist has never been accused, release their locked bond back to them
+                if (!s.sarcophagusArchaeologists[sarcoId][archaeologistAddresses[i]].accused) {
+                     LibBonds.freeArchaeologist(sarcoId, archaeologistAddresses[i]);


Also I'm wondering, could we not remove the need for this extra loop by doing the freeArchaeologist in the loop on 172 above?
...
No, no we can't. We can only free if we know the sarco is compromised.

How about instead using an in-memory list of goodArchAddresses that is added to during the first loop, then this loop would only go through that list and free them all?

Accuse has always been a loop-loving hell-hole, I remember now.

DarksightKellar · 2022-11-18T12:21:12Z

contracts/libraries/LibTypes.sol

@@ -65,6 +65,7 @@ library LibTypes {
    // of the archaeologist uploading to arweave, which will be stored directly
    // on the sarcophagus.
    struct ArchaeologistStorage {
+        bool accused;


isAccused? 😬

DarksightKellar · 2022-11-18T13:30:18Z

test/facets/thirdparty/accuse.spec.ts

+  });
+
+  describe("Supports accusal of fewer than k archaeologists", function () {
+    it("On a successful accusal of an archaeologist, should transfer the correct amount of funds to embalmer and accuser, slash the archaeologist's bond, mark the arch as accused, and emit an AccuseArchaeologist event", async function () {


I'd very much rather split up these (and other similarly combined) assertions into their own it-test blocks -- even if it means duplicate code.

Specifically, either a describe or context with "On successful accusal of an archaeologist", within which we have a distinct it "should do xyz" for each expectation.

I'm sure beforeEach would be enough to do the prerequisite setup being done below.

It's much easier to digest that way, and easier to debug as well.

100%, I would love to be able to set up this test set with a context and then have an individual it() for most of these expect calls. I thought about breaking these up into more its and duplicating the setup, but even with utility methods it ends up being a pretty significant amount of boilerplate getting copied between cases.

DarksightKellar · 2022-11-18T13:42:07Z

test/facets/helpers/accounts.ts

+        "Not enough test accounts. Increase value in hardhat config."
+      );
+    return await ethers.getSigner(unnamedAccounts[index++]);
+  };


DarksightKellar · 2022-11-18T13:50:12Z

contracts/facets/ThirdPartyFacet.sol

-                totalCursedBond,
-                diggingFeesToBeDistributed
+                totalDiggingFees,
+                totalDiggingFees


Could we not now remove one of these params?

haha definitely

knoll3 · 2022-11-18T16:02:07Z

contracts/facets/ThirdPartyFacet.sol

+
+        // if the current call hasn't resulted in at least k archaeologists being accused
+        // check if total number of historical accusals on sarcophagus is greater than k
+        uint historicalAccusals = 0;


Is it possible to store historicalAccusals in sarcoState to avoid the loop below?

I'm thinking that it's better to have the accuser incur the cost of the loop than it is to add to the data model because accuse isn't going to be called often (and the caller is getting a payout) but an increase in the size of the Sarcophagus model impacts all users.

knoll3 · 2022-11-18T16:08:45Z

contracts/facets/ThirdPartyFacet.sol

     * @param sarcoId The identifier of the sarcophagus having leaked shards
-     * @param unencryptedShardHashes At least 'm' unencrypted shard hashes as proof of bad behaviour
+     * @param unencryptedShardHashes hashes of the leaked keyshares
     * @param paymentAddress the address to which rewards should be sent if successful
     */
    function accuse(
        bytes32 sarcoId,
        bytes32[] memory unencryptedShardHashes,


What is the purpose for being able to accuse multiple archs in one function? Is it just to save on transaction costs if an accuser has multiple accusals to make?

savings on transaction cost and also if we required accusals to be one at a time, we'd be limiting rewards to the bonds for k archaeologists. With the array, an accuser could get a payout for all n if they'd all leaked their keyshare but with individual calls, the remaining n-k would be freed as soon as the sarcophagus was deemed compromised.

sethhrbek · 2022-11-18T16:04:28Z

contracts/facets/ThirdPartyFacet.sol

+        uint historicalAccusals = 0;
+        if (!isSarcophagusCompromised) {
+            for (uint256 i = 0; i < archaeologistAddresses.length; i++) {
+                if (s.sarcophagusArchaeologists[sarcoId][archaeologistAddresses[i]].accused) {
+                    historicalAccusals++;
                }
            }
+            // the sarcophagus is compromised if k or more archaeologists have been accused over the lifetime of the sarcophagus
+            if (historicalAccusals >= sarco.minShards) {
+                isSarcophagusCompromised = true;
+            }
+        }


This function looks good. My only thought is whether we could get significant gas savings for the user if they were only submitting a single hash --- this would require an alternate accuseSingle function, where there's only a single hash passed in. In that scenario, we would probably need a lot less code (could just iterate over all the archaeologists on the sarcophagus once, then do the distributions).

I'm not suggesting we necessarily do this, but could discuss.

I don't think we'd actually be able to remove any of the logic other than the part that says to loop over the set of supplied keyshare hashes. We'd still need to take the same actions on the single supplied share, so the logic inside of the loop would stay, and then we'd also need to evaluate whether or not the single archaeologist being accused brought the sarcophagus over the k threshold and if yes free the other archaeologists.

Breads added 9 commits November 13, 2022 14:58

remove calculateCursedBond, fall back to digging fees

c76df63

Merge branch '94-remove-transfer' of github.com:sarcophagus-org/sarco…

7c262c8

…phagus-v2-contracts into 96-update-accusal-flow

make first attempt at updating accuse logic to support < k archaeolog…

9e5d85a

…ists

prepping test fixtures for accuse

ff8e8d6

create new utilities for contract and signer retrieval, sarco token i…

fec30ae

…nteractions, archaeologist generation and registration, and sarco creation. add a few tests for accuse

finish happy path test on a single arch accusal

cc3f700

readability improvements to accuse

59a79db

add test cases for a sarcophagus that is compromised by accusals

1b7d230

add more test cases for accuse on a compromised sarco, remove old thi…

baf254e

…rdpartyfacet tests

gh-23378 requested review from DarksightKellar and sethhrbek November 18, 2022 07:15

gh-23378 linked an issue Nov 18, 2022 that may be closed by this pull request

Update accusal flow #96

Closed

gh-23378 commented Nov 18, 2022

View reviewed changes

fix a few failing test cases

8aaf118

gh-23378 commented Nov 18, 2022

View reviewed changes

gh-23378 requested a review from knoll3 November 18, 2022 07:36

gh-23378 commented Nov 18, 2022

View reviewed changes

gh-23378 requested a review from mobiusTripper-crypto November 18, 2022 07:44

gh-23378 marked this pull request as ready for review November 18, 2022 07:45

DarksightKellar suggested changes Nov 18, 2022

View reviewed changes

DarksightKellar reviewed Nov 18, 2022

View reviewed changes

knoll3 reviewed Nov 18, 2022

View reviewed changes

sethhrbek reviewed Nov 18, 2022

View reviewed changes

rename accused to isAccused

a005f69

sethhrbek approved these changes Nov 21, 2022

View reviewed changes

gh-23378 requested a review from DarksightKellar November 21, 2022 21:00

sethhrbek merged commit fa31111 into main Nov 21, 2022

sethhrbek deleted the 96-update-accusal-flow branch November 21, 2022 21:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

96 update accusal flow #101

96 update accusal flow #101

gh-23378 commented Nov 18, 2022

gh-23378 Nov 18, 2022

gh-23378 Nov 18, 2022

gh-23378 Nov 18, 2022

DarksightKellar Nov 18, 2022

DarksightKellar Nov 18, 2022

gh-23378 Nov 18, 2022

gh-23378 commented Nov 18, 2022

gh-23378 Nov 18, 2022

DarksightKellar Nov 18, 2022

gh-23378 Nov 18, 2022

knoll3 Nov 21, 2022

knoll3 Nov 21, 2022

DarksightKellar left a comment •

edited

Loading

DarksightKellar Nov 18, 2022

DarksightKellar Nov 18, 2022

DarksightKellar Nov 18, 2022

gh-23378 Nov 18, 2022

DarksightKellar Nov 18, 2022

DarksightKellar Nov 18, 2022

gh-23378 Nov 18, 2022

knoll3 Nov 18, 2022 •

edited

Loading

gh-23378 Nov 18, 2022

knoll3 Nov 18, 2022

gh-23378 Nov 18, 2022

sethhrbek Nov 18, 2022

gh-23378 Nov 18, 2022

96 update accusal flow #101

96 update accusal flow #101

Conversation

gh-23378 commented Nov 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gh-23378 commented Nov 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarksightKellar left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knoll3 Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarksightKellar left a comment •

edited

Loading

knoll3 Nov 18, 2022 •

edited

Loading