(DvA) Add DvA Transfer Manager #171

aliarbak · 2023-10-05T12:39:24Z

No description provided.

aliarbak · 2023-10-09T14:08:09Z

contracts/DVA/DVATransferManager.sol

+
+    // solhint-disable-next-line code-complexity
+    function _approveTransfer(bytes32 transferID, Transfer storage transfer) internal returns (bool allApproved) {
+        bool approved = false;


I'm not sure I could implement this logic in a simpler way. I'm open to any suggestions 😅

contracts/DVA/DVATransferManager.sol

Nakasar · 2023-10-11T09:52:01Z

We store the list of approvers in the _transfers mapping. This means if a user initiates a transfer, then the approval criteria are modified, then the transfer would still use the criteria at the date it was initiated and not the updated ones. Is this expected (I couldn't see any mention of this in the specs so maybe not)?

aliarbak · 2023-10-11T09:53:44Z

We store the list of approvers in the _transfers mapping. This means if a user initiates a transfer, then the approval criteria are modified, then the transfer would still use the criteria at the date it was initiated and not the updated ones. Is this expected (I couldn't see any mention of this in the specs so maybe not)?

Not sure, I'll ask 👍

…he transfer is initiated

aliarbak · 2023-10-17T09:27:55Z

We store the list of approvers in the _transfers mapping. This means if a user initiates a transfer, then the approval criteria are modified, then the transfer would still use the criteria at the date it was initiated and not the updated ones. Is this expected (I couldn't see any mention of this in the specs so maybe not)?

Hey @Nakasar,
The final business decision here: If approval criteria change after a transfer is initiated, we need to reset the transfer's approval state (and invalidate all previous approvers).

And when an approver approves a transfer after the criteria change, it needs to reset the approval state and return. S/he needs to call the approve function one more time to be able to approve. This is the expected behavior.
(I updated the contract btw)

…ions/T-REX into dva-transfer-manager

Joachim-Lebrun · 2023-10-25T08:04:03Z

CHANGELOG.md

@@ -16,6 +16,7 @@ All notable changes to this project will be documented in this file.
  constant variable to be declared.
 - Add `function isPlugAndPlay() external pure returns (bool)` to `IModule`. Compliance modules now require this function to be declared. It indicates whether the compliance can be bound without any presetting.
 - Add `function canComplianceBind(address _compliance) external view returns (bool)` to `IModule`. Compliance modules now require this function to be declared. If it returns false, it means some presets must be made before compliance can bind to the module.
+- DVATransferManager contract


would be good to describe the contract a bit here

Joachim-Lebrun · 2023-10-30T09:11:39Z

contracts/DVA/DVATransferManager.sol

+    }
+
+    struct ApprovalCriteria {
+        address tokenAddress;


Suggested change

address tokenAddress;

As the ApprovalCriteria is given by a mapping(address => ApprovalCriteria) with address being the token address, it feels redundant to have it in the struct

Joachim-Lebrun · 2023-10-30T09:25:25Z

contracts/DVA/DVATransferManager.sol

+     *  DVATransferManager must be an agent of the given token
+     *  emits an `ApprovalCriteriaModified` event
+     */
+    function modifyApprovalCriteria(


not sure about the naming here. modify implies that the ApprovalCriteria already exists, while in fact this function is also used to set initial ApprovalCriteria
In this case i guess the terminology setApprovalCriteria would be more relevant

Joachim-Lebrun · 2023-10-30T09:31:03Z

contracts/DVA/DVATransferManager.sol

+     *  @param includeAgentApprover determines whether the agent is included in the approver list
+     *  @param sequentialApproval determines whether approvals must be sequential
+     *  @param additionalApprovers are the addresses of additional approvers to be added to the approver list
+     *  Only an agent of a token can call this function


Agents on T-REX are in charge of operational tasks such as minting, burning, freezing, recovering lost tokens, etc. Here i believe the task is more related to the general transfer rules management for the token, which is under the owner scope in T-REX, in the same way that it is the token owner that specifies the compliance rules, or the eligibility rules, i believe it should be the Token owner specifying the approval criterias in DvA

Joachim-Lebrun · 2023-10-30T09:34:22Z

contracts/DVA/DVATransferManager.sol

+     *  @param amount is the transfer amount
+     *  Approval criteria must be preset for the given token address
+     *  Receiver must be verified for the given token address
+     *  emits a `TransferInitiated` event


could add that the sender should give allowance first to the DvA smart contract address on the token contract, and that this allowance needs to be at least amount

Joachim-Lebrun · 2023-10-30T09:40:00Z

contracts/DVA/DVATransferManager.sol

+
+        IToken token = IToken(tokenAddress);
+        if (!token.identityRegistry().isVerified(recipient)) {
+            revert RecipientIsNotVerified(tokenAddress, recipient);


maybe would make sense to also test canTransfer with address _from being the DvA address and _to the recipient for amount, to be sure that after approval, the transfer will be allowed to happen. Or maybe this is a bit overkill...

We already have an e2e test for that (checks the balances as well)
https://github.com/TokenySolutions/T-REX/pull/171/files#diff-5bfef1f607b3b2d5e5aa8f381f378a17e93375f7cce6bfd63dad06dc354c5b97R511

Joachim-Lebrun · 2023-10-30T09:42:18Z

contracts/DVA/DVATransferManager.sol

+
+        bool transferSent = token.transferFrom(msg.sender, address(this), amount);
+        if (!transferSent) {
+            revert TokenTransferFailed(tokenAddress, msg.sender, address(this), amount);


I don't think this code is reachable. When transfer fails it reverts, therefore i don't see how we could get the case where transferFrom returns false and reach this code. I would therefore remove this part and the custom error that goes with it

Joachim-Lebrun · 2023-10-30T09:48:41Z

contracts/DVA/DVATransferManager.sol

+    function _transferTokensTo(Transfer memory transfer, address to) internal {
+        bool transferSent = IToken(transfer.tokenAddress).transfer(to, transfer.amount);
+        if (!transferSent) {
+            revert TokenTransferFailed(transfer.tokenAddress, address(this), transfer.sender, transfer.amount);


same remark as for transferFrom above, this code is not reachable, transfer either returns true or reverts. therefore it is not possible to get access to this case in which the transfer returns false and reach this custom error

Joachim-Lebrun · 2023-10-30T11:39:30Z

@aliarbak we should also have an implementation contract for DvA

aliarbak · 2023-10-31T13:03:25Z

all done @Joachim-Lebrun

aliarbak added 2 commits October 5, 2023 15:39

(DvA) Add DvA Transfer Manager

37bd233

Disable lint issues

f7f23b5

aliarbak added new contracts test coverage labels Oct 6, 2023

Joachim-Lebrun requested review from Nakasar and Joachim-Lebrun October 9, 2023 09:24

Joachim-Lebrun assigned aliarbak Oct 9, 2023

Split functions to increase readability

4b5610e

aliarbak commented Oct 9, 2023

View reviewed changes

Nakasar reviewed Oct 11, 2023

View reviewed changes

contracts/DVA/DVATransferManager.sol Outdated Show resolved Hide resolved

Add anyTokenAgent flag to DvA transfer approver

2318287

Reset the transfer approval state if approval criteria change after t…

29a731d

…he transfer is initiated

aliarbak added 7 commits October 17, 2023 15:01

Add delegate approve transfers function

e21b6c1

Add name function to DvATransferManager

20e561b

Update CHANGELOG.md

d6ecc22

Update package version to 4.1.0-beta1

b837cbf

Merge branch 'develop' into dva-transfer-manager

2b31a82

Add anyTokenAgent flag to getNextApprover result

b90557a

Merge branch 'dva-transfer-manager' of https://github.com/TokenySolut…

92f1707

…ions/T-REX into dva-transfer-manager

Joachim-Lebrun reviewed Oct 25, 2023

View reviewed changes

Joachim-Lebrun reviewed Oct 30, 2023

View reviewed changes

aliarbak added 4 commits October 31, 2023 15:25

Add DVA interface and remove approvers list from events

911e6ed

Update package version

be60654

Update namings and changelog

a1bb6d9

Update index.ts

629a323

aliarbak requested a review from Joachim-Lebrun October 31, 2023 13:03

aliarbak added 2 commits October 31, 2023 16:18

Add tokenAddress back to ApprovalCriteriaSet event

2aaff00

Update documentation of delegateApproveTransfer

0dbaf8d

Joachim-Lebrun merged commit 8e81b5e into develop Nov 6, 2023
3 checks passed

Joachim-Lebrun deleted the dva-transfer-manager branch November 6, 2023 12:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(DvA) Add DvA Transfer Manager #171

(DvA) Add DvA Transfer Manager #171

aliarbak commented Oct 5, 2023

aliarbak Oct 9, 2023

Nakasar commented Oct 11, 2023

aliarbak commented Oct 11, 2023

aliarbak commented Oct 17, 2023 •

edited

Loading

Joachim-Lebrun Oct 25, 2023

Joachim-Lebrun Oct 30, 2023

Joachim-Lebrun Oct 30, 2023

Joachim-Lebrun Oct 30, 2023

Joachim-Lebrun Oct 30, 2023

Joachim-Lebrun Oct 30, 2023

aliarbak Oct 31, 2023 •

edited

Loading

Joachim-Lebrun Oct 30, 2023

Joachim-Lebrun Oct 30, 2023

Joachim-Lebrun commented Oct 30, 2023

aliarbak commented Oct 31, 2023

(DvA) Add DvA Transfer Manager #171

(DvA) Add DvA Transfer Manager #171

Conversation

aliarbak commented Oct 5, 2023

Choose a reason for hiding this comment

Nakasar commented Oct 11, 2023

aliarbak commented Oct 11, 2023

aliarbak commented Oct 17, 2023 • 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

aliarbak Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joachim-Lebrun commented Oct 30, 2023

aliarbak commented Oct 31, 2023

aliarbak commented Oct 17, 2023 •

edited

Loading

aliarbak Oct 31, 2023 •

edited

Loading