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

Cosigned assets: NopOp and COAUTHORIZED_FLAG #146

Closed
tomerweller opened this issue Aug 8, 2018 · 12 comments · Fixed by #244
Closed

Cosigned assets: NopOp and COAUTHORIZED_FLAG #146

tomerweller opened this issue Aug 8, 2018 · 12 comments · Fixed by #244
Labels
CAP Represents an issue that requires a CAP.

Comments

@tomerweller
Copy link
Contributor

(Posting on behalf of David Mazières)

Motivation

A number of Stellar asset issuers need greater control over assets
than simply authorizing particular accounts to hold the asset. As a
result, we are seeing people build solutions in which the asset issuer
is actually a co-signer on asset holders' accounts. This solution has
several drawbacks. In particular, the asset owner cannot unilaterally
withdraw XLM or other assets from his or her account, and it is very
hard to trade one such restricted asset for another (as both issuers
would need to be cosigners on the account, giving one issuer
permission to authorize transactions on the other's asset).

This proposal is a first cut at attempting to address the problem
through minimal stellar-core changes.

Co-signer authorized trust lines

To accommodate asset issuers with restrictions, we propose adding
another mode for trustlines of AUTH_REQUIRED assets, called
"cosigner authorized," that lies somewhere between being not
authorized and fully authorized. The idea is to allow issuers to
require asset holders to request co-signing of transactions involving
the asset without being cosigners on asset holders' accounts.

enum TrustLineFlags
{
    // issuer has authorized account to perform transactions with its credit
    AUTHORIZED_FLAG = 1,
    // issuer allows transactions that are co-signed by issuer low threshold
    COAUTHORIZED_FLAG = 2
};

There are now three modes for the trustline of an AUTH_REQUIRED
asset, based on TrustLineEntry::flags:

  • If (flags & 3) == 0, the source account is not authorized. Any
    PAYMENT, PATH_PAYMENT, MANAGE_OFFER, or
    CREATE_PASSIVE_OFFER, with the source account causes the whole
    transaction to fail, with only two exceptions:

    • A PATH_PAYMENT is allowed to contain the asset in path, so
      long as it does not apear in sendAsset or destAsset, and

    • A MANAGE_OFFER is allowed if it sets amount to 0 (to delete
      the offer).

    In addition, any operation with a different (even authorized) source
    account will fail if it attempts to send the asset to an account
    with (flags & 3) == 0. Finally, existing orders on the order book
    belonging to the account with (flags & 3) == 0 are immediately
    invalid and cannot be filled. This behavior should be close or
    identical to the status quo.

  • If (flags & 3) == COAUTHORIZED_FLAG for an account, then the
    account has certain additional permissions compared to when those
    bits are 0. Specifically:

    • The account's offers in the order book continue to be valid, and
      can be filled at any time.

    • The account can use MANAGE_OFFER to reduce the amount of an
      offer involving the asset (including canceling the offer with
      and amount of 0), but not to increase the offer.

    • The account can receive (but not send) payments and path
      payments in the asset, up to the limit of the trustline.

    However, any other operation on the asset requires that the
    transaction be co-signed by the asset issuer at low threshold.
    Moreover, if the account increases the limit on the trustline and
    the transaction is not cosigned by the issuer, then the
    COAUTHORIZED_FLAG is cleared so (flags & 3) goes back to 0.

  • If (flags & 3) == AUTHORIZED_FLAG, then the account can perform
    arbitrary transactions on the asset, including sending payments to
    accounts with COAUTHORIZED_FLAG.

Operation changes

Implementing cosigned assets requires changes to two operations.
Because an asset issuer's signature may be out of place on a
transaction requiring asset issuer coauthorization, we add a NopOp
operation that requires a signature from an arbitrary source account.
Such an operation will be useful in other contexts, too, as currently
pre-authorized transactions sometimes require ugly hacks like having
an account send one Stroup to itself. Second, we have to modify
ALLOW_TRUST to enable issuers to set the new COAUTHORIZED_FLAG.

NO_OPERATION (NopOp)

The NO_OPERATION operation does nothing, but requires a valid
signature from the operation's source account. Since the default
source account is the source of the transaction, this will always
succeed at low threshold if the operation's sourceAccount is
NULL. However, it can be used to require a low threshold signature
from an asset issuer.

enum NopOp {
    LOW_THRESHOLD = 0,
    MED_THRESHOLD = 1,
    HIGH_THRESHOLD = 2
};

enum NopResult {
    NOP_SUCCESS = 0,
    NOP_BAD_THRESHOLD = -1,  // the operation was an invalid value
};

enum OperationType
{
    ...
    BUMP_SEQUENCE = 11,
    NO_OPERATION = 12
};

struct Operation
{
    union switch (OperationType type)
    {
    case NO_OPERATION:
        NopOp nopOp;
    }
    body;
};

union OperationResult switch (OperationResultCode code)
{
case opINNER:
    union switch (OperationType type)
    {
        ...
    case NO_OPERATION:
        NopResult nopResult;
    }
    tr;
default:
    void;
};

NopOp is not strictly necessary, as a redundant ALLOW_TRUST can be
used instead, but having NopOp is cleaner and has other applications
to pre-authorized transactions.

ALLOW_TRUST flags

Only one small change is required to AllowTrustOp. We change the
authorize field from a bool to a uint32. This provides binary
compatibility with clients that do not know about the
COAUTHORIZED_FLAG (since AUTHORIZED_FLAG == TRUE in XDR), but
having a uint32 provides the additional option of setting it to
COAUTHORIZED_FLAG.

struct AllowTrustOp
{
    AccountID trustor;
    union switch (AssetType type)
    {
    // ASSET_TYPE_NATIVE is not allowed
    case ASSET_TYPE_CREDIT_ALPHANUM4:
        opaque assetCode4[4];

    case ASSET_TYPE_CREDIT_ALPHANUM12:
        opaque assetCode12[12];

        // add other asset types here in the future
    }
    asset;

    // 0, AUTHORIZED_FLAG, or COAUTHORIZED_FLAG
    uint32 authorize;
};
@tomerweller tomerweller added the CAP Represents an issue that requires a CAP. label Aug 8, 2018
@tomerweller tomerweller changed the title Cosigned assets: NopOp and COAUTHORIZED_FLAG Cosigned assets: NopOp and COAUTHORIZED_FLAG Aug 8, 2018
@bartekn
Copy link
Contributor

bartekn commented Aug 9, 2018

@stanford-scs

with the source account causes the whole
transaction to fail, with only two exceptions:

  • A PATH_PAYMENT is allowed to contain the asset in path, so
    long as it does not apear in sendAsset or destAsset, and

  • A MANAGE_OFFER is allowed if it sets amount to 0 (to delete
    the offer).

Maybe a good moment to allow issuers to send back assets that are no longer authorized? Without it, any stats that involve number of assets (shares issued, assets in circulation, etc.) may be inaccurate because holder doesn't want to send it back.

Finally, existing orders on the order book belonging to the account with (flags & 3) == 0 are immediately invalid and cannot be filled.

After stellar/stellar-core#1718 offers in accounts that are no longer authorized are deleted.

uint32 authorize;

Why not use TrustLineFlags authorize (or a new enum type) here? It also has the same representation as bool and will be more clear.

Looks good! Would be great to see an example of transactions flow from holder and issuer that authorize a coauthorized trustline and then send a simple payment with issuer signature.

@dgobaud
Copy link

dgobaud commented Aug 11, 2018

@bartekn "Maybe a good moment to allow issuers to send back assets that are no longer authorized?"

Do you mean allow issuers to remove assets from an account that is no longer authorized? If so that would be great. I think already accounts that hold assets that become unauthorized can send them back to the issuer to "burn" them right?

@bartekn
Copy link
Contributor

bartekn commented Aug 11, 2018

Do you mean allow issuers to remove assets from an account that is no longer authorized? If so that would be great.

Yes, exactly.

I think already accounts that hold assets that become unauthorized can send them back to the issuer to "burn" them right?

This was an issue in our documentation, fixed only a few months ago. The truth is neither unauthorized account nor issuer can move the asset. Source: https://github.com/stellar/stellar-core/blob/c5fc4b13fb52c4b437698271c1a02bbb76bb23d1/src/transactions/PathPaymentOpFrame.cpp#L323-L331

@johansten
Copy link
Contributor

johansten commented Aug 14, 2018

Didn't notice #135 until now.

Still think it's a bad idea. Keep assets like that on separate accounts. Multi-sig works as-is.

As a developer, how do I know if an asset is co-signed, and needs a no-op and being sent off to some arbitrary cosigning service, before I'm able to submit it to the network? How do I even know who the cosigner is?

(Is no-op really needed, when you can just use the cosigner as source account for the tx env?)

@JeremyRubin
Copy link
Contributor

Noting that BumpSeq 0 has NoOp semantics, so we may just reuse that presently.

Or we may relax BumpSeq authorization to no longer require signers if the BumpSeq is a no-op.

@tomerweller
Copy link
Contributor Author

tomerweller commented Aug 14, 2018

@johansten,

// update: removed references to deleted comments

Still think it's a bad idea. Keep assets like that on separate accounts. Multi-sig works as-is.

The Motivation section above describes problems with the multi-sig approach. Problems which you have not addressed in your constructive feedback.

As a developer, how do I know if an asset is co-signed, and needs a no-op and being sent off to some arbitrary cosigning service, before I'm able to submit it to the network? How do I even know who the cosigner is?

This is more relevant as feedback for #135, in which it's suggested to add this information to the TOML file of the issuer.

Is no-op really needed, when you can just use the cosigner as source account for the tx env?

There are ways around a no-op (for example bumpseq as suggest by @JeremyRubin above). Adding the issuer as the source account on an operation allows stellar core to take advantage of the current mechanism of signature collection.

As you point out, using the issuer as a source account for the entire transaction can also accomplish that. However, this can lead to concurrency issues as the issuer will also need to provide a correct sequence number.

@johansten
Copy link
Contributor

johansten commented Aug 14, 2018

@tomerweller

I didn't notice #135 until after I had written my first comments. I've seen multi-sig used extensively by ICOs trying to do trading restrictions/vesting, hence my comment.

Still thinks it's a bad idea.

Again, how to deal with it as a developer?

None of the flags are exposed in Horizon as of yet, even after years of complaints.
E.g., It's impossible to find out if an account has an authorized trustline for an asset.

Using no-op, or bumpseq, or tx env source account, we still need to know if/how to do it.
stellar.toml might be one way, but still, it adds a lot of friction and overhead to everything.

Added: wouldn't is make sense to put this as an account flag?

@tomerweller
Copy link
Contributor Author

@johansten,

Still thinks it's a bad idea.

You get points for consistency.

None of the flags are exposed in Horizon as of yet, even after years of complaints.

Right. Horizon is lagging behind and it needs to get fixed. My sources tell me that the SDF is working on it, and that they're open to pull requests ;)

Using no-op, or bumpseq, or tx env source account, we still need to know if/how to do it.
stellar.toml might be one way, but still, it adds a lot of friction and overhead to everything.

Right. Ideally it should be handled in the SDK level, similarly to federation.

These are all valid concerns. However, I suggest that we try to limit the discussion in this repository to the protocol itself.

@johansten
Copy link
Contributor

These are all valid concerns. However, I suggest that we try to limit the discussion in this repository to the protocol itself.

It's hard to look at these things in isolation. The protocol affects how you use it.

If it's an asset-wide setting it should go into the account setting flags, just like all the other asset-wide settings.

@johansten
Copy link
Contributor

johansten commented Aug 18, 2018

Since you guys are mum on specifics regarding the background, I'm just guessing here.

Looking at this from a security token perspective, a lot of it makes sense, and some things don't.

If you want to be able to do compliance checks per transaction, co-signing seems like the best way to get that done.

From that PoV, some things leave me wondering what you're targeting. It doesn't seem to be that.
I guess I'm asking how this fits together with #135, and how to make this orthogonal to it, since there seems to be a bit of an overlap.

Is this a per-asset thing, or a per-trustline thing?

Again, I have no clue why you're trying to do this.

Looking at it from security token PoV again, you'd probably want the whole asset to be co-signed, and not individual asset holders.

So we require an additional signature in co-signing mode. And we add that by including a new No-op op, being a low signing threshold type of operation.

Is this really needed though? Can't we just say, if a trust line is co-signed, the issuer has to co-sign the transaction, w/o any new operation?

@tomerweller
Copy link
Contributor Author

tomerweller commented Aug 20, 2018

I guess I'm asking how this fits together with #135, and how to make this orthogonal to it, since there seems to be a bit of an overlap.

These issues are complimentary, #135 addresses the high level, ecosystem support. Which, in theory, can work without this protocol change. The protocol change is there to address the idiosyncrasies of having issuers as co-signers on a holder's account.

Looking at it from security token PoV again, you'd probably want the whole asset to be co-signed, and not individual asset holders.

Yeah, that's true in most cases. The idea behind using a flag on the trustline and not the issuer account is that issuers can still offer unrestricted access to specific trusted accounts, for example, market makers. It's more generic but still encompasses the base case. Do you see any potential issues here?

Is this really needed though? Can't we just say, if a trust line is co-signed, the issuer has to co-sign the transaction, w/o any new operation?

That's a good point. I'll defer to David (@stanford-scs) but I think main motivation is to not add complexity to the existing signature weight aggregation system.

@stanford-scs
Copy link
Contributor

stanford-scs commented Sep 4, 2018

The relationship between this and #135 is that this proposal is a concrete implementation of what #135 calls approach 1.

The reason we can't just change the semantics of unauthorized assets is that offers for co-signed assets must remain valid in the order book, while they must be canceled/invalid for unauthorized assets.

@JeremyRubin BumpSeq 0 is a viable alternative, though it will make transactions less clear. As an example of confusion, I was confused because the XDR file has no comment saying the threshold was low. (This proposal requires a NOP with low threshold.) As it happens, the threshold is low in the implementation. The question is more generally whether you believe having a NOP is useful, and if so this would be a good point to insert it in the ecosystem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CAP Represents an issue that requires a CAP.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants