-
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
Forced Transfer #253
Forced Transfer #253
Conversation
@thegostep I feel this could be done as a module that the issuer can add so we keep the same logic of permissions. The |
contracts/tokens/SecurityToken.sol
Outdated
* @param _value amount of tokens to transfer | ||
* @param _data data attached to the transfer by controller for event | ||
*/ | ||
function controllerTransfer(address _from, address _to, uint256 _value, bytes _data) public onlyController { |
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.
what's _data
for?
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.
a reason string explaining why the transfer occured which gets logged as an event
@pabloruiz55 I agree this makes sense. Should this be a new module or added to the general permission manager? |
Not sure how a module would adjust We could introduce a setter for @pabloruiz55 related to this, do you think forcedTransfer should respect the verifyTransfer rules? I am thinking it should do (this would mean that the controller can only force transfer where it is possible given the attached TM modules - they could of course use the MATM if needed as well). @thegostep how important do you think it is to be able to return a list of controller addresses on-chain (rather than by event scanning) - this does make the logic more complex as you have to keep the indexes etc. and clean up behind yourself. |
I’m the one probably missing something.
Couldn’t we make the new ‘controllerTransfer’ “onlyModule” so only another contract can call it? Being that other contract a specific module.
…Sent from my iPhone
On Sep 11, 2018, at 8:04 PM, Adam Dossa ***@***.***> wrote:
Not sure how a module would adjust balances though unless I am missing something? It would need to do it via the ST contract, which doesn't have a setter for this variable.
We could introduce a setter for balances (which is only callable by a module of a certain type) but not sure that is the best approach.
@pabloruiz55 related to this, do you think forcedTransfer should respect the verifyTransfer rules? I am thinking it should do (this would mean that the controller can only force transfer where it is possible given the attached TM modules - they could of course use the MATM if needed as well).
@thegostep how important do you think it is to be able to return a list of controller addresses on-chain (rather than by event scanning) - this does make the logic more complex as you have to keep the indexes etc. and clean up behind yourself.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@thegostep Hey Stephane - we discussed this on the call today. Some thoughts:
|
contracts/tokens/SecurityToken.sol
Outdated
* @param _value amount of tokens to transfer | ||
* @param _data data attached to the transfer by controller for event | ||
*/ | ||
function controllerTransfer(address _from, address _to, uint256 _value, bytes _data) public onlyController { |
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.
Wouldn't this function also need to be added to the constructor?
transferFunctions[bytes4(keccak256(...))] = true;
@adamdossa
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.
Only if it calls verifyTransfer()
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.
This pattern can be revisited using internal / private functions
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.
Yes - if it is going to call verifyTransfer
then it should do so that TMs know they should record any side-effects of the transfer.
One option here is to call verifyTransfer
but disregard the result (or just include the result in the ControlledTransfer event).
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 like the idea of adding the result to the event
* @param _value amount of tokens to transfer | ||
* @param _data data attached to the transfer by controller to emit in event | ||
*/ | ||
function controllerTransfer(address _from, address _to, uint256 _value, bytes _data) external returns(bool); |
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.
Let's rename it to forceTransfer as it is a bit more evident what this does.
The way I see this working is that the So, as long as isEnabled == true this function can be called by the set operator. And then the setOperator function can be greatly simplified so the issuer can set it to an address or 0x0 at any moment. |
@adamdossa looks ready for you to review. |
@pabloruiz55 Reviewed and modified function names. If you can approve I'll merge once tests run. |
Please check if the PR fulfills these requirements
The commit message follows our Submission guidelines
Docs have been added / updated (for bug fixes / features)
Tests for the changes have been added (for bug fixes / features)
What kind of change does this PR introduce?
(Bug fix, feature, docs update, ...)
What is the current behavior?
(You can also link to an open issue here)
What is the new behavior?
(Define and describe any new functionality. Clarify if this is a feature change)
Does this PR introduce a breaking change?
(What changes might users need to make in their application due to this PR?)
Any Other information: