Feat/delayed close #20

merisman · 2025-04-08T20:02:05Z

No description provided.

gpxl-dev

See comments, a few subjective.

Most critical is that I don't think proposeAndSign can work?
Did not review tests.

src/CyberCorpFactory.sol

gpxl-dev · 2025-04-08T20:23:18Z

src/CyberDealRegistry.sol

@@ -199,7 +222,9 @@ contract CyberDealRegistry is Initializable, UUPSUpgradeable, BorgAuthACL {
        string[] memory globalValues,
        address[] memory parties,
        string[] memory creatingPartyValues,
-        string[] memory counterPartyValues
+        string[] memory counterPartyValues,


This assumes two parties - I wonder if we should account for more?

don't think so for dealmanager, escrow will only be for 2 parties.

src/CyberDealRegistry.sol

gpxl-dev · 2025-04-08T20:32:25Z

src/CyberDealRegistry.sol

-        string[] memory counterPartyValues
+        string[] memory counterPartyValues,
+        bytes32 secretHash,
+        address finalizer
    ) external returns (bytes32 contractId) {


I think a neater implementation would be:

have createContract always accept an array of arrays of party values: string[][] partyValues

check every array inside partyValues is the correct length, and check that the corresponding party is not the zero address.

loop through partyValues[] setting the mapping per the parties

This way you only have a single createContract function. If you wanted to have two explicity entry points, you could make it internal and call it _createContract, and keep the two functions you have that call through appropriately.

Yes, have made the string[][] param throughout and kept it all one create method.

gpxl-dev · 2025-04-08T20:38:20Z

src/CyberDealRegistry.sol

@@ -55,6 +58,12 @@ contract CyberDealRegistry is Initializable, UUPSUpgradeable, BorgAuthACL {
    // A mapping connecting an address to all the agreements they are a party to
    mapping(address => bytes32[]) public agreementsForParty;

+    mapping(bytes32 => address[]) public voidedBy;
+
+    mapping(bytes32 => uint256) public expiry;


This expiry never gets set AFAICT.

Is there a good reason that voidedBy and expiry are not in the AgreementData struct?

also for voidedBy if moved AgreementData, I prefer `mapping(address => bool) voidRequested.

Added expiry (and secretHash) but kept the voidRequestedBy mapping outside -- I think this is probably preferable than nested mappings/arrays.

alright. We still have signedAt as a mapping though, just poitning out it's inconsistent really

gpxl-dev · 2025-04-08T21:10:39Z

src/CyberDealRegistry.sol

+        if(!isParty(contractId, party)) revert NotAParty();
+
+        AgreementData storage agreementData = agreements[contractId];
+        if (agreementData.finalized) revert ContractAlreadyFinalized();


Since finalization is optional, I think we should probably check if there finalizer is a zero address, and if so, disallow voiding if the number of signatures is equal to the number of parties (i.e. it is fully signed)

That way you can't void if finalization of the contract is simply that it is fully signed.

I now set the finalized flag if finalizer is set to 0 and the contract is fully signed on the last signature.

gpxl-dev · 2025-04-08T21:16:11Z

src/DealManager.sol

            if (keccak256(abi.encode(counterPartyCheck)) != keccak256(abi.encode(partyValues))) revert CounterPartyValueMismatch();
        }

+        if(!conditionCheck(agreementId)) revert AgreementConditionsNotMet();


currently nowhere to set conditions

We need to allow these to be set on creation to remove any chance of signing before they are added. Also added editing by the issuer if the deal is pending.

gpxl-dev · 2025-04-08T21:20:27Z

src/DealManager.sol

        emit DealFinalized(
            agreementId,
            msg.sender,
            CORP,
            address(DEAL_REGISTRY),
-            _fillUnallocated
+            false


can remove this bool from the event I think - don't think it needs to be in here?

src/libs/LexScroWLite.sol

gpxl-dev · 2025-04-08T21:28:21Z

src/libs/LexScroWLite.sol

+        if(deal.status != EscrowStatus.PAID) revert EscrowNotPaid();
+        if(!ICyberDealRegistry(DEAL_REGISTRY).isVoided(agreementId)) revert DealNotVoided();
+
+        // Refund buyer assets first


Feels like something should be done with the corpAssets... I know in the case of the safe it will be burned, so this is ok for that... but wonder if we should still have the loop for corpassets regardless given we do in other functions.

Send them back to the printer contract, maybe? We won't burn the NFT's just void them.

gpxl-dev

See comments, a few subjective.

Most critical is that I don't think proposeAndSign can work?
Did not review tests.

Detoo

I need to think about @gpxl-dev 's comment on void workflow so flushing what I have for now

Detoo · 2025-04-08T22:09:10Z

src/DealManager.sol

        Escrow storage deal = escrows[agreementId];
        for(uint256 i = 0; i < deal.corpAssets.length; i++) {
            if(deal.corpAssets[i].tokenType == TokenType.ERC721) {
                IIssuanceManager(ISSUANCE_MANAGER).voidCertificate(deal.corpAssets[i].tokenAddress, deal.corpAssets[i].tokenId);
            }
        }
        voidEscrow(agreementId);
+        ICyberDealRegistry(DEAL_REGISTRY).voidContractFor(agreementId, signer, signature);
+    }


Missing expiry checks?

Not sure if it is intentional: it will make voidExpiredDeal exclusive to party members only. It used to be open to anyone

Updates: nvm, expiry checks is in voidContractFor();

however, what happens if none of the following conditions are met? (1) the number of void requests is equal to the number of parties, and (2) the expiry is in the past.

Wouldn't we end up with a weird state like this?

agreement.voided = false escrow.state = VOIDED

src/DealManager.sol

Detoo · 2025-04-09T17:07:01Z

src/DealManager.sol

        Escrow storage deal = escrows[agreementId];
        for(uint256 i = 0; i < deal.corpAssets.length; i++) {
            if(deal.corpAssets[i].tokenType == TokenType.ERC721) {
                IIssuanceManager(ISSUANCE_MANAGER).voidCertificate(deal.corpAssets[i].tokenAddress, deal.corpAssets[i].tokenId);
            }
        }
        voidEscrow(agreementId);
+        ICyberDealRegistry(DEAL_REGISTRY).voidContractFor(agreementId, signer, signature);
+    }


Updates: nvm, expiry checks is in voidContractFor();

however, what happens if none of the following conditions are met? (1) the number of void requests is equal to the number of parties, and (2) the expiry is in the past.

Wouldn't we end up with a weird state like this?

agreement.voided = false escrow.state = VOIDED

src/CyberAgreementRegistry.sol

Detoo

I missed the merge of closed/open offers. A few minor suggestions

Detoo · 2025-04-09T18:49:00Z

src/CyberAgreementRegistry.sol

+            }
+            //matching address cannot be 0
+            if (parties[i] == address(0)) {
+                revert FirstPartyZeroAddress();


Suggest refactoring it to PartyZeroAddress(uint256 index) due to its more general usage

Detoo · 2025-04-09T22:22:32Z

src/CyberAgreementRegistry.sol

+        }
+
+        if(agreementData.voided)
+            emit ContractVoided(contractId, voidRequestedBy[contractId], block.timestamp);


shouldn't it revert if agreementData not voided? Otherwise we'd end up with a non-void agreement + a voided escrow

merisman added 8 commits April 6, 2025 11:31

Updating Close Flow, condition checks

8d12549

More delayed close methods

3961384

Updating cyber registry for handling the different void/cancel cases

0d1e81a

Finalizing CyberCerts, Adding more state checks

f8c06b3

More state checks.

d462c2f

More state and updates to lexscrow lite

cdd7354

cert ID in deal manager fix

da9a97d

Adding secret, adding tests, fixing void flows.

6701b50

merisman requested a review from Detoo April 8, 2025 20:02

gpxl-dev approved these changes Apr 8, 2025

View reviewed changes

gpxl-dev requested changes Apr 8, 2025

View reviewed changes

Detoo reviewed Apr 8, 2025

View reviewed changes

merisman added 5 commits April 9, 2025 00:02

Added string[][] for partyValues throughout, other pr fixes.

4004282

adding auth on add/remove conditions

2a9177c

Updates to voiding, adding timestamped event on void

3cb1722

Adding finalization events.

fce5c0f

Renaming CyberDealRegistry => CyberAgreementRegistry

89d0945

Detoo reviewed Apr 9, 2025

View reviewed changes

Updates to voidRequestedBy, adding counterparty values

973108c

Detoo reviewed Apr 9, 2025

View reviewed changes

merisman and others added 4 commits April 10, 2025 16:07

Updating Void Signature

305322a

Event tweaks

93895d5

Fix toJSON function

7d09c00

Skip duplicate revert for multiple zeroAddress parties

d88409a

Feat/delayed close #20

Are you sure you want to change the base?

Feat/delayed close #20

Uh oh!

Conversation

merisman commented Apr 8, 2025

Uh oh!

gpxl-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpxl-dev Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpxl-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Detoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Detoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gpxl-dev Apr 8, 2025 •

edited

Loading