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.

Sign up for GitHub

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

Jump to bottom

services/horizon: CAP0018 Support #2423

Merged
merged 12 commits into from
Mar 30, 2020
Merged

services/horizon: CAP0018 Support #2423

merged 12 commits into from
Mar 30, 2020

Conversation

abuiles
Copy link
Contributor

@abuiles abuiles commented Mar 27, 2020 •
edited
Loading

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

Add initial support for CAP0018.

Why

Horizon needs to support CAP0018. To do this we have to update ingestion and all the JSON related structs.

At the ingestion level the following changes will happen:

Operation Details

We now add both flags to operation details

Before
{
  "authorize":    true,
  "asset_code":   "COP",
  "asset_issuer": "GDRW375MAYR46ODGF2WGANQC2RRZL7O246DYHHCGWTV2RE7IHE2QUQLD",
  "asset_type":   "credit_alphanum4",
  "trustee":      "GDRW375MAYR46ODGF2WGANQC2RRZL7O246DYHHCGWTV2RE7IHE2QUQLD",
  "trustor":      "GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3"
}
After

{
  "authorize":  true,
  "authorize_to_maintain_liabilities": true,
  "asset_code":                        "COP",
  "asset_issuer":                      "GDRW375MAYR46ODGF2WGANQC2RRZL7O246DYHHCGWTV2RE7IHE2QUQLD",
  "asset_type":                        "credit_alphanum4",
  "trustee":                           "GDRW375MAYR46ODGF2WGANQC2RRZL7O246DYHHCGWTV2RE7IHE2QUQLD",
  "trustor":                           "GDQNY3PBOJOKYZSRMK2S7LHHGWZIUISD4QORETLMXEWXBI7KFZZMKTL3"
}

The following is how the JSON response for an operation will look like when the flag is 2:

{
  "id": "124042211741474817",
  "paging_token": "124042211741474817",
  "transaction_successful": true,
  "source_account": "GBSTRUSD7IRX73RQZBL3RQUH6KS3O4NYFY3QCALDLZD77XMZOPWAVTUK",
  "type": "allow_trust",
  "type_i": 7,
  "created_at": "2020-03-27T03:40:10Z",
  "transaction_hash": "a77d4ee5346d55fb8026cdcdad6e4b5e0c440c96b4627e3727f4ccfa6d199e94",
  "asset_type": "credit_alphanum4",
  "asset_code": "USD",
  "asset_issuer": "GBSTRUSD7IRX73RQZBL3RQUH6KS3O4NYFY3QCALDLZD77XMZOPWAVTUK",
  "trustee": "GBSTRUSD7IRX73RQZBL3RQUH6KS3O4NYFY3QCALDLZD77XMZOPWAVTUK",
  "trustor": "GA332TXN6BX2DYKGYB7FW5BWV2JLQKERNX4T7EUJT4MHWOW2TSGC2SPM",
  "authorize":                         false,
  "authorize_to_maintain_liabilities": true,
}

And the following when it is 1:

{
  "id": "124042211741474817",
  "paging_token": "124042211741474817",
  "transaction_successful": true,
  "source_account": "GBSTRUSD7IRX73RQZBL3RQUH6KS3O4NYFY3QCALDLZD77XMZOPWAVTUK",
  "type": "allow_trust",
  "type_i": 7,
  "created_at": "2020-03-27T03:40:10Z",
  "transaction_hash": "a77d4ee5346d55fb8026cdcdad6e4b5e0c440c96b4627e3727f4ccfa6d199e94",
  "asset_type": "credit_alphanum4",
  "asset_code": "USD",
  "asset_issuer": "GBSTRUSD7IRX73RQZBL3RQUH6KS3O4NYFY3QCALDLZD77XMZOPWAVTUK",
  "trustee": "GBSTRUSD7IRX73RQZBL3RQUH6KS3O4NYFY3QCALDLZD77XMZOPWAVTUK",
  "trustor": "GA332TXN6BX2DYKGYB7FW5BWV2JLQKERNX4T7EUJT4MHWOW2TSGC2SPM",
  "authorize":                         true,
  "authorize_to_maintain_liabilities": true,
}

Effect Details

We add a new effect history.EffectTrustlineAuthorizedToMaintainLiabilities.

The JSON representation is the same as history.EffectTrustlineAuthorized and history.EffectTrustlineDeauthorized - except for the effect type and type id.

Balance resource

Similar to the operation response, we'll show both flags.

Trustline with flag 0 OR 1

"balances": [
    {
      "is_authorized": false,
      "is_authorized_to_maintain_liabilities": false,
      "balance": "27.1374422",
      "limit": "922337203685.4775807",
      "buying_liabilities": "0.0000000",
      "selling_liabilities": "0.0000000",
      "last_modified_ledger": 28893780,
      "asset_type": "credit_alphanum4",
      "asset_code": "USD",
      "asset_issuer": "GBSTRUSD7IRX73RQZBL3RQUH6KS3O4NYFY3QCALDLZD77XMZOPWAVTUK"
    },
    {
      "balance": "1.5000000",
      "buying_liabilities": "0.0000000",
      "selling_liabilities": "0.0000000",
      "asset_type": "native"
    }
  ],

Trustline with flag 2

"balances": [
    {
      "is_authorized": false,
      "is_authorized_to_maintain_liabilities": true,
      "balance": "27.1374422",
      "limit": "922337203685.4775807",
      "buying_liabilities": "0.0000000",
      "selling_liabilities": "0.0000000",
      "last_modified_ledger": 28893780,
      "asset_type": "credit_alphanum4",
      "asset_code": "USD",
      "asset_issuer": "GBSTRUSD7IRX73RQZBL3RQUH6KS3O4NYFY3QCALDLZD77XMZOPWAVTUK"
    },
    {
      "balance": "1.5000000",
      "buying_liabilities": "0.0000000",
      "selling_liabilities": "0.0000000",
      "asset_type": "native"
    }
  ],

This is part of #2357

Known limitations

[TODO or N/A]

@cla-bot cla-bot bot added the cla: yes label Mar 27, 2020
@abuiles abuiles changed the title Extend operation processor with authorized to mainliabilities details. services/horizon: CAP0018 Support Mar 27, 2020
@abuiles abuiles marked this pull request as ready for review March 27, 2020 23:04
@abuiles abuiles requested review from bartekn, tamirms, 2opremio and ire-and-curses and removed request for bartekn March 27, 2020 23:04
@abuiles abuiles force-pushed the cap18-operation-ingestion branch from 6cfefc0 to 750cca0 Compare March 30, 2020 15:29
@abuiles abuiles force-pushed the cap18-operation-ingestion branch from f2e492d to 3f819f9 Compare March 30, 2020 17:49
@abuiles abuiles merged commit 39db025 into stellar:protocol-13 Mar 30, 2020
@abuiles abuiles deleted the cap18-operation-ingestion branch March 30, 2020 18:01
@abuiles abuiles mentioned this pull request Mar 30, 2020
Closed
6 tasks
tamirms pushed a commit that referenced this pull request Mar 31, 2020
@abuiles @tamirms
services/horizon: CAP0018 Support (#2423)
88f97da 
* Extend operation processor with authorized to mainliabilities details.

* Add EffectTrustlineAuthorizedToMaintainLiabilities.

* Add EffectTrustlineAuthorizedToMaintainLiabilities to protocol.

* Show both authorize and authorize_to_maintain_liabilities in in operation details.

* Add test for AllowTrust resource adapter.

* Update allow trust details.

* Add test for effect resource.

* Add is_authorized_to_maintain_liabilities to balance resource.

* Always show is_authorized_to_maintain_liabilities.

* Always show authorize_to_maintain_liabilities in operation allow trust.

* Include all flags in allow trust operation details.

* Update changelog.
@ire-and-curses ire-and-curses linked an issue Apr 7, 2020 that may be closed by this pull request
Support Fee Bump Transactions (CAP 15) #2360
Closed
10 tasks
bartekn
bartekn reviewed Apr 20, 2020
View reviewed changes
Copy link
Contributor

@bartekn bartekn left a comment

Choose a reason for hiding this comment

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

Sorry for post-merge review, catching up with all the changes.

I found one inconsistency that can be confusing. From CAP-18:

The combination AUTHORIZED_FLAG | AUTHORIZED_TO_MAINTAIN_LIABILITIES_FLAG is not valid because AUTHORIZED_FLAG implies AUTHORIZED_TO_MAINTAIN_LIABILITIES_FLAG

The code in operation processor seems to be correctly setting the authorize_to_maintain_liabilities value:

details["authorize_to_maintain_liabilities"] = xdr.TrustLineFlags(op.Authorize).IsAuthorized() || xdr.TrustLineFlags(op.Authorize).IsAuthorizedToMaintainLiabilitiesFlag()

However the code in db2.history.TrustLine.IsAuthorizedToMaintainLiabilities doesn't. If I set AUTHORIZED_FLAG on a trust line, the is_authorized_to_maintain_liabilities on the Balance object will be false however authorize_to_maintain_liabilities on the operation will be true.

I guess the question is about the level of abstraction in Horizon:

  • If we want to show raw flag values then we should change operation processor code.
  • If we want to show the actual interpretation of raw flag values (AUTHORIZED_FLAG = true then AUTHORIZED_TO_MAINTAIN_LIABILITIES_FLAG = true) we should change balance object.

@abuiles
Copy link
Contributor Author

abuiles commented Apr 21, 2020

@bartekn

However the code in db2.history.TrustLine.IsAuthorizedToMaintainLiabilities doesn't. If I set AUTHORIZED_FLAG on a trust line, the is_authorized_to_maintain_liabilities on the Balance object will be false however authorize_to_maintain_liabilities on the operation will be true.

This is handled correctly in the balance object, if the authorize flag is 1, we show is_authorized_to_maintain_liabilities as true in balances -- see https://github.com/stellar/go/pull/2423/files#diff-667f71e250631e23bb40996734b0c0a1R53

Btw, we went back and forth on how to store and show this, check out for more context #2423 (comment)

@bartekn
Copy link
Contributor

bartekn commented Apr 21, 2020

@abuiles thanks! I see now that it was actually updated here: da9f526#diff-fa18377d7302ceaa29836017d5030298R261.

This was referenced Apr 25, 2020
Closed
Open
Closed
Closed
Closed
Closed
Closed
Closed
Open
Open
Closed
Closed
Closed
@fracek fracek mentioned this pull request Apr 25, 2020
Closed
@abuiles abuiles mentioned this pull request May 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartekn bartekn bartekn left review comments

@tamirms tamirms tamirms approved these changes

@2opremio 2opremio Awaiting requested review from 2opremio

@ire-and-curses ire-and-curses Awaiting requested review from ire-and-curses

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Fee Bump Transactions (CAP 15)
3 participants
@abuiles @bartekn @tamirms