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

Added Send All #313

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

Added Send All #313

wants to merge 1 commit into from

Conversation

nixoid
Copy link

@nixoid nixoid commented Jul 29, 2015

This method should allow users to send all owned omni assets in 1 transaction.
More detailed description here
OmniLayer/omnicore#153

This method should allow users to send all owned omni assets with 1 transaction
@@ -404,6 +405,23 @@ Say you want to transfer 1 Mastercoin to another address. Only 16 bytes are need
|Currency identifier| [Currency identifier](#field-currency-identifier) |1 (Mastercoin)|
|Amount to transfer|[Number of Coins](#field-number-of-coins)|100,000,000 (1.0 coins) |

### Transfer All Coins (Send All)

Description: Transaction type 4 transfers all coins from the sending address to the reference address, defined in [Appendix A](#appendix-a-storing-omni-protocol-data-in-the-blockchain). This transaction can not be used to transfer bitcoins.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... transfers all coins in the available balances of all smart properties owned by the sending address ...

Items that need consideration:

  1. is this an all or nothing transaction - transfers of all available balances succeed or none of them succeed if at least 1 fails?
    • certain sends may be blocked in the future, e.g. a non-transferable SP (maybe that's just not in the available balance), address can reject receipt of specified SPs
      • for instance, an address may want to divest itself of one or more SPs and prevent any future ownership of those SPs
    • how are failure details reported?
  2. side effects of transfers, e.g. if the reference address has an open crowdsale for a transferred currency, the sending address would be credited with newly issued coins. This can be handled by the fact that this is not a txtype 0 Send, so the protocol can specify that Send All does not invoke the side effects of txtype 0 Send. That needs a decision.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WRT side effects, Send To Owners doesn't invoke crowdsale participation (as far as I know) so there's a precedent. The spec doesn't state that, so that's another clarification to be made.

@dexX7
Copy link
Member

dexX7 commented Jul 30, 2015

@marv-engine: is this an all or nothing transaction - transfers of all available balances succeed or none of them succeed if at least 1 fails?

Current behavior of OmniLayer/omnicore#155 is that the transaction is only valid, if at least one token is transferred. This is in line with send to owners transactions, which require at least one receiver to be valid.

certain sends may be blocked in the future, e.g. a non-transferable SP (maybe that's just not in the available balance)

I think we'd most likely create a new "tally" type, such as "reserved for sell offers", which is not part of the "available" balance. I don't really see the need for a refinement, but it should be explicitly stated, that only "available" balances are transferred.

side effects of transfers

The dilemma started with the "grant tokens" implementation (see mastercoin-MSC/mastercore#227), which simply calls the simple send logic. I'm not sure, if the passive effect (i.e. crowdsale participation) was intended, but now we have to live with it, I guess. It would be great, if this would be specified (of course not part of this PR).

I'd be fine, if the send all transaction does not trigger crowdsale participations, and thanks for pointing out that send to owners is without passiv effect. This serves as great justification, why send all may not trigger the participation either.

@dexX7
Copy link
Member

dexX7 commented Jul 30, 2015

In this context: if the transaction has no receiver, then we assume the sender is the receiver, and the transaction may be a valid "send all to self" transaction. This mirrors the behavior of "simple send".

@zathras-crypto
Copy link
Contributor

Send to owners is a great example, thanks!

In this context then it seems feasible to make crowdsale participation
limited to an explicit simple send only.

I'll take a look at whether any grants have ever triggered crowdsale
participation and if they have not, it may be a viable option to say that
as of X consensus-affecting feature activation that grants will no longer
have a secondary affect of participating in a crowdsale. As long as such
an action doesn't occur before then we could remove current logic in a
future release.
On Jul 31, 2015 2:07 AM, "dexX7" notifications@github.com wrote:

@marv-engine https://github.com/marv-engine: is this an all or nothing
transaction - transfers of all available balances succeed or none of them
succeed if at least 1 fails?

Current behavior of OmniLayer/omnicore#155
OmniLayer/omnicore#155 is that the transaction
is only valid, if at least one token is transferred. This is in line with
send to owners transactions, which require at least one receiver to be
valid.

certain sends may be blocked in the future, e.g. a non-transferable SP
(maybe that's just not in the available balance)

I think we'd most likely create a new "tally" type, such as "reserved for
sell offers", which is not part of the "available" balance.

side effects of transfers

The dilemma started with the "grant tokens" implementation (see
mastercoin-MSC/mastercore#227
mastercoin-MSC/mastercore#227), which simply
calls the simple send logic. I'm not sure, if the passive effect (i.e.
crowdsale participation) was intended, but now we have to live with it, I
guess. It would be great, if this would be specified (of course not part of
this PR).

I'd be fine, if the send all transaction does not trigger crowdsale
participations, and thanks for pointing out that send to owners is without
passiv effect. This serves as great justification, why send all may not
trigger the participation either.


Reply to this email directly or view it on GitHub
#313 (comment).

@zathras-crypto
Copy link
Contributor

I've run through a full parse with some extra code to look for crowdsale participation from a grant and I can't see any, so I'm fairly confident no grants have ever triggered crowdsale participation to date.

@dexX7
Copy link
Member

dexX7 commented Aug 3, 2015

In the context of an unrelated discussion about the mutability of transactions with test properties on mainnet:

The "send all" transaction currently violates that the main and test ecosystem should be strictly seperated. This has no direct effect on the global state, and the resulting main ecosystem balances [...] are exactly the same in any case, even if we assume the test ecosystem history may not be immutable.

However, if we assume the test ecosystem history is mutable, then it may have an impact on transaction validity. Right now a "send all" transaction is considered as valid, if at least one token was transferred, which may be in the test or main ecosystem.

To get to the point: I was wondering, if the "send all" transaction should come with an ecosystem parameter, to specify, whether test or main properties should be transferred.

@marvgmail
Copy link

I was wondering, if the "send all" transaction should come with an ecosystem parameter, to specify, whether test or main properties should be transferred.

@dexX7 Yes, all transactions should specify the ecosystem.

Also,

Right now a "send all" transaction is considered as valid, if at least one token was transferred

In this case, it's not necessarily one whole divisible token, but any fractional amount qualifies.

@dexX7
Copy link
Member

dexX7 commented Aug 3, 2015

@marvgmail: Yes, all transactions should specify the ecosystem.

Alright, so the new transaction format would be:

[version: 0] [type: 4] [ecosystem]

In this case, it's not necessarily one whole divisible token, but any fractional amount qualifies.

Yup, sure. I was referring to base units.

@zathras-crypto
Copy link
Contributor

What about MetaDEx cancels? Is that not the same kind of scenario? Where we allow either ecosystem, or both?

Q - why invalidate a 'send all' transaction that does not transfer any tokens. Other "no-ops" are not invalidated:

  • We don't invalidate a MetaDEx cancel that doesn't match any trades.
  • We don't invalidate sends to self that don't move any tokens.
    etc etc

@dexX7
Copy link
Member

dexX7 commented Aug 4, 2015

What about MetaDEx cancels? Is that not the same kind of scenario? Where we allow either ecosystem, or both?

It would probably best to allow only valid ecosystem values, i.e. no wildcard for both. It was initially done this way (with wildcard) due to the transaction message format (with property/property desired).

Q - why invalidate a 'send all' transaction that does not transfer any tokens.

Hmm.. I think we should get rid of those no-ops altogether. Send-to-self should be allowed, if the receiver is explicitly specified as such, but the current fallback to send-to-self, if no receiver was provided, just seems unnecessary.

I'd argue against no-ops with a counter question: do you think we should remove all checks, whether amounts to be transferred/created/... are not zero, and allow sends with zero tokens?

@dexX7
Copy link
Member

dexX7 commented Aug 4, 2015

A few words to my motivation in this context: the no-op question could become more relevant in the future. Let's assume we extend the methology of consensus hashes, and not only include balances [...], but also hash a list of valid transactions.

To ensure (historical) consensus hashes/checkpoints don't change, this requires that the validity of historical transactions is immutable.

Based on your earlier statement, that test property transactions may not be immutable, it follows that the validity of historical transactions may not be immutable either, if transactions without strict main/test ecosystem seperation are involved, thus introducing the chance of changing (historical) consensus hashes.

@zathras-crypto
Copy link
Contributor

It would probably best to allow only valid ecosystem values, i.e. no wildcard for both. It was initially done this way (with wildcard) due to the transaction message format (with property/property desired).

I agree here - I think we should have explicit separation of test and main ecosystems. If MetaDEx is the only place we allow a single payload to impact both test and main, then let's close that behaviour now before live.

I'd argue against no-ops with a counter question: do you think we should remove all checks, whether amounts to be transferred/created/... are not zero, and allow sends with zero tokens?

Nope. But there is a big difference here that needs to be considered, explicit versus implicit actions.

Explicit actions, eg "Send X from A to B" should be invalidated if that explicit action cannot be performed, either because X doesn't exist or is insufficient value.

But implicit actions, eg "Send {any} from A to B" should not be invalidated if {any} does not have any matches, because no matches is not the same as an invalid value. Just the same as "Cancel {any} trades from A" is not invalid if {any} does not have any matches.

In both cases A and B are explicit values and should invalidate if they are not acceptable values.

I hope I've explained that right hehe :)

Based on your earlier statement, that test property transactions may not be immutable, it follows that the validity of historical transactions may not be immutable either, if transactions without strict main/test ecosystem seperation are involved, thus introducing the chance of changing (historical) consensus hashes.

I agree here 100%, which is why I think there should be a "ban" if you will on any transactions that can cross ecosystems (ie single payload = single ecosystem only). EDIT: more accurately single transaction = single ecosystem only, in the context that we may think about multi-payload transactions in the future.

@dexX7
Copy link
Member

dexX7 commented Aug 4, 2015

But implicit actions, eg "Send {any} from A to B" should not be invalidated if {any} does not have any matches, because no matches is not the same as an invalid value. Just the same as "Cancel {any} trades from A" is not invalid if {any} does not have any matches.

This sounds reasonable. How about the other MetaDEx cancel operations then? The cancel operations are currently only considered as valid, if trades are really cancelled.

@zathras-crypto
Copy link
Contributor

Oh, my mistake - MetaDEx cancels shouldn't be invalid if they don't match
anything imo.
On Aug 4, 2015 12:37 PM, "dexX7" notifications@github.com wrote:

But implicit actions, eg "Send {any} from A to B" should not be
invalidated if {any} does not have any matches, because no matches is not
the same as an invalid value. Just the same as "Cancel {any} trades from A"
is not invalid if {any} does not have any matches.

This sounds reasonable. How about the other MetaDEx cancel operations
then? The cancel operations are currently only considered as valid, if
trades are really cancelled.


Reply to this email directly or view it on GitHub
#313 (comment).

@zathras-crypto
Copy link
Contributor

But implicit actions, eg "Send {any} from A to B" should not be invalidated if {any} does not have any matches, because no matches is not the same as an invalid value. Just the same as "Cancel {any} trades from A" is not invalid if {any} does not have any matches.

This sounds reasonable. How about the other MetaDEx cancel operations then? The cancel operations are currently only considered as valid, if trades are really cancelled.

I know this is not directly related to Send All, but I think both Send All and MetaDEx cancels should not be invalidated if they do not have any matches. Since MetaDEx isn't live yet I feel we have room to make that change without affecting consensus. Thoughts?

@dexX7
Copy link
Member

dexX7 commented Aug 21, 2015

Since MetaDEx isn't live yet I feel we have room to make that change without affecting consensus. Thoughts?

Supported.

@zathras-crypto
Copy link
Contributor

Supported

What do you think to an approach like zathras-crypto/omnicore@a751bef

@marvgmail
Copy link

It's about a month and a 1/2 since the last comment. Where do we stand on this PR?

@dexX7
Copy link
Member

dexX7 commented Oct 15, 2015

The "send-all" command is part of Omni Core 0.0.10.

The transaction format is as follows:

[version: 0] [type: 4] [ecosystem: 1 (for main) or 2 (for test)]

All available (i.e. not reserved) tokens in the given ecosystem are transferred from the sender to the receiver.

The transaction is considered as invalid, if no tokens were actually moved, i.e. the sender must have tokens in the given ecosystem.

See also the RPC documentation for "omni_sendall".

@dexX7
Copy link
Member

dexX7 commented Jul 2, 2019

The transaction "send all" is part of the current consensus rules, so we should build upon this pull request and eventually integrate it.

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

Successfully merging this pull request may close these issues.

5 participants