Feat/legion conditions#115
Conversation
… Add tests against real CyberAgreementRegistry
…ry. Misc housekeeping to library interfaces & dependencies
…st coverage for misc conditions
…ger-secondary-trades # Conflicts: # src/interfaces/ICyberCertPrinter.sol
Feat/cert legal owner indexing
…ger-secondary-trades
…econdary trading
…dger building. Misc fixes. Add more tests
…with scripify events
fix: scripifying cert should respect reserved units
…ger-secondary-trades
…nes, and unit reservation should be managed end-to-end by secondary transfer controller (aka deal manager) itself
…-trades deal manager secondary trades
…nal-audit Fix/secondary trading internal audit
…hs only, lots of mocks)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9880b12a99
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (from == to) return; | ||
|
|
||
| if (from != address(0)) { | ||
| uint256 fromBalance = s.holderTokenCount[from] - 1; |
There was a problem hiding this comment.
For printers upgraded with already-minted certificates, holderTokenCount starts at zero and there is no migration/backfill for it (the new backfillLegalOwners only populates the legal-owner enumeration). The first post-upgrade transfer or burn of any legacy token enters this branch with s.holderTokenCount[from] == 0, so 0 - 1 reverts under Solidity 0.8 checked arithmetic, blocking legacy certificate transfers and any pending deal finalization that tries to move a pre-upgrade escrowed cert.
Useful? React with 👍 / 👎.
| uint256 currentCount = usResidentOnlyCount | ||
| ? _usResidentHolderCount(printer) | ||
| : printer.holderCount(); |
There was a problem hiding this comment.
When HolderCapCondition is configured with usResidentOnlyCount == false, this uses printer.holderCount(), but that count is maintained from ERC-721 custody changes rather than the legal-owner ledger. In administered hosting, executeSecondaryTransfer mints the NFT to adminMultisig while recording the buyer as legal owner, so many distinct investors behind one custodian are counted as a single holder and the ICA holder cap can be exceeded.
Useful? React with 👍 / 👎.
2 participants
No description provided.