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

Remove Asset Code Types 4 and 12 #296

Open
JeremyRubin opened this issue Apr 23, 2019 · 2 comments
Open

Remove Asset Code Types 4 and 12 #296

JeremyRubin opened this issue Apr 23, 2019 · 2 comments
Labels
CAP Represents an issue that requires a CAP. enhancement CAP that adds or changes functionality as opposed to a fix. needs draft This an issue that has no corresponding draft, and as such has not entered the CAP/SEP process.

Comments

@JeremyRubin
Copy link
Contributor

The two existing current non-native Asset Code types are currently guaranteed to be non-overlapping without the enum tag by length.

Given that there are no functional differences between asset code types, I propose deprecating the distinction between the types by allowing either tag to refer to up to 12 bytes of asset code.

This should simplify the handling of this both internally (and help ensure that treatment of asset types is uniform) and externally for consumers of the API.

This should be forwards compatible as old clients should produce XDR which is binary compatible with the new semantics, but may break backwards compatibility as new clients may produce XDR which is incompatible with old clients parsings. While breaking backwards compatibility is not great, it may be a worthwhile cleanup to remove this distinction.

The motivation for such a change is that stellar core's rules should not be enforcing UX-layer invariants.

@tomerweller
Copy link
Contributor

@JeremyRubin this would be easier to discuss with the actual revised schema

@JeremyRubin
Copy link
Contributor Author

Fair crit -- hence being an issue rather than a PR.

Here are a few options:

original:

// 1-4 alphanumeric characters right-padded with 0 bytes
typedef opaque AssetCode4[4];

// 5-12 alphanumeric characters right-padded with 0 bytes
typedef opaque AssetCode12[12];

enum AssetType
{
    ASSET_TYPE_NATIVE = 0,
    ASSET_TYPE_CREDIT_ALPHANUM4 = 1,
    ASSET_TYPE_CREDIT_ALPHANUM12 = 2
};

union Asset switch (AssetType type)
{
case ASSET_TYPE_NATIVE: // Not credit
    void;

case ASSET_TYPE_CREDIT_ALPHANUM4:
    struct
    {
        AssetCode4 assetCode;
        AccountID issuer;
    } alphaNum4;

case ASSET_TYPE_CREDIT_ALPHANUM12:
    struct
    {
        AssetCode12 assetCode;
        AccountID issuer;
    } alphaNum12;

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

Tiny ugly change:

// 1-4 alphanumeric characters right-padded with 0 bytes
typedef opaque AssetCode4[12];

Full change:

// 5-12 alphanumeric characters right-padded with 0 bytes
typedef opaque AssetCode12[12];

enum AssetType
{
    ASSET_TYPE_NATIVE = 0,
    ASSET_TYPE_CREDIT_ALPHANUM4_DEPRECATED = 1,
    ASSET_TYPE_CREDIT_ALPHANUM12 = 2
};

struct AlphaNumAssetCode12 {
        AssetCode12 assetCode;
        AccountID issuer;
}

union Asset switch (AssetType type)
{
case ASSET_TYPE_NATIVE: // Not credit
    void;
case ASSET_TYPE_CREDIT_ALPHANUM4_DEPRECATED:
    struct AlphaNumAssetCode12 alphaNum12;
case ASSET_TYPE_CREDIT_ALPHANUM12:
    struct AlpahNumAssetCode12 alphaNum12;

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

@theaeolianmachine theaeolianmachine added CAP Represents an issue that requires a CAP. enhancement CAP that adds or changes functionality as opposed to a fix. needs draft This an issue that has no corresponding draft, and as such has not entered the CAP/SEP process. labels Jun 14, 2019
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. enhancement CAP that adds or changes functionality as opposed to a fix. needs draft This an issue that has no corresponding draft, and as such has not entered the CAP/SEP process.
Projects
None yet
Development

No branches or pull requests

4 participants
@theaeolianmachine @tomerweller @JeremyRubin and others