-
Notifications
You must be signed in to change notification settings - Fork 160
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Signed Transfer Manager #533
Conversation
return Result.NA; | ||
|
||
bytes32 hash = keccak256(abi.encodePacked(targetAddress, nonce, expiry, _from, _to, _amount)); | ||
bytes32 prependedHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n32", hash)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Encoder Library could be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll refactor a bit to use this (and in other places if needed)
if (!_checkSigner(signer)) { | ||
return Result.NA; | ||
} else if(_isTransfer) { | ||
_invalidateSignature(signature); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of SignatureInvalidated
event, we could emit SignatureUsed
or something +ve event as transaction get succeed. For invalidateSignature()
function this event makes sense but for a successful transfer, it will create confusion.
*/ | ||
function getTags() public view returns(bytes32[] memory) { | ||
bytes32[] memory availableTags = new bytes32[](2); | ||
availableTags[0] = "General"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be "Signed"
function deploy(bytes calldata /* _data */) external returns(address) { | ||
address polyToken = _takeFee(); | ||
SignedTransferManager signedTransferManager = new SignedTransferManager(msg.sender, polyToken); | ||
emit GenerateModuleFromFactory(address(signedTransferManager), getName(), address(this), msg.sender, now); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it follow the proxy approach of deployment ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For "Experimental" modules we haven't been using the Proxy deployment approach.
function updateSigners(address[] calldata _signers, bool[] calldata _signersStats) external withPerm(ADMIN) { | ||
require(_signers.length == _signersStats.length, "Array length mismatch"); | ||
IDataStore dataStore = IDataStore(ISecurityToken(securityToken).dataStore()); | ||
for(uint256 i=0; i<_signers.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting to store Signer addresses in the data store. I guess this makes sense, esp. if "signers" are going to be used in other places and would make upgrading the module easier (via a remove and re-add).
This PR includes STM and proper test case with signed data.
The module uses datastore to store its data.