Skip to content
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

allow multisig for non value transfer #58

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CJentzsch
Copy link
Contributor

When using the multisig wallet for non value transfer purposes, it always requires only one signature since even with a daily limit of 0, a 0 value transfer transaction is "under the daily limit".
Using < instead of <= solves the problem.
The only downside is that the real daily limit which can be spend per day is m_dailyLimit - 1 wei.

@alexvandesande
Copy link

Wouldn't a better solution be to always require multiple signatures if the data field was not empty? Otherwise all one would need to approve would be to send a non transfer transaction with a few wei.

@CJentzsch
Copy link
Contributor Author

CJentzsch commented Apr 27, 2016

hm, but calling a fallback function of another contract might also be something someone wants to do.
It could even be "just signing" something. No data no value transfer, just to prove or show something.

@chriseth
Copy link
Collaborator

The daily limit feature is not really useful once we have currency abstraction. I agree that with a daily limit of 0, every call (including send) should require multisig.

@CJentzsch
Copy link
Contributor Author

another option would be to make m_dailyLimit an int, instead of an uint . This way one could set it to a negative number. By doing this m_dailyLimit could still be the real maximum daily limit you can actually spent, and you have the possibility to let any transaction be signed by multiple signatures by setting it to -1.
But then you have a max m_dailyLimit of 2**128 , which should be enough ;-)

@alexvandesande
Copy link

@CJentzsch that sounds like a hack in order to do something that would be better as a parameter.

I don't think the parameter should even exist, if you want to do something like signing a transaction to prove something then it's reasonable to require multiple signatures, since we can't really detect it.

Another way to do it would be to create a more complex function that would analyze calls and allow or disallow them in a case by case question, but at that point it's so much complexity that we should simply adopt a DAO and you could add a "auto-manager" as one of the members. The future of the contract wallet is being a dao anyway.

@CJentzsch
Copy link
Contributor Author

ok, but using < instead of <= is actually intuitive. When I set daily limit to 0, I want everything coming from the multisig wallet signed by m_required members of the multisig.

@frozeman
Copy link
Contributor

Yes we should merge that. though it means a daily limit of 100 is actually only 99.9999999999...

@frozeman
Copy link
Contributor

@tgerring @vbuterin

@bencxr
Copy link
Contributor

bencxr commented May 19, 2016

Daily limit of 100 but only allowing 99.99999 can be confusing for the user, who will want to send 100.

I am in favor with @alexvandesande to always require multiple signatures when the data field is non empty, since using the value to check is no longer "safe" (may be tokens of greater value). That way we also satisfy @CJentzsch's point that a daily limit of 0 should mean "everything coming from the wallet signed by m_required members".

I'll make this change and submit a pull request.

@alexvandesande
Copy link

Thanks @bencxr !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants