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

Add in effects that weren't being returned by Horizon #368

Closed

Conversation

martinxsliu
Copy link
Contributor

I was looking at the way Horizon returns effects and I noticed that there were some effects and fields that weren't being serialized properly and hence not getting returned in the API. This PR adds in the missing effects.

Changes:

  • Adds AuthImmutable flag for the AccountFlagsUpdated effect.
  • Adds InflationDestination for the AccountInflationDestinationUpdated effect.
  • Adds the DataCreated, DataRemoved, and DataUpdated effects.

For reference I've been using

func (is *Session) ingestEffects() {
as the authoritative source of effects and their fields.

@martinxsliu
Copy link
Contributor Author

This was also reported in #294

@martinxsliu
Copy link
Contributor Author

In the course of doing this, I also found that the offer effects: EffectOfferCreated, EffectOfferRemoved, and EffectOfferUpdated are not being created in the ingestEffects method I linked above, as their docstring might suggest. Are these effects an artifact from earlier times and should no longer be used?

Copy link
Contributor

@nullstyle nullstyle left a comment

Choose a reason for hiding this comment

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

In addition to the one comment I made about keeping the API unchanged, please implement some tests for this PR.

It's looking good; Thanks!

Trustor string `json:"trustor"`
AssetType string `json:"asset_type"`
AssetCode string `json:"asset_code,omitempty"`
base.Asset
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't a refactoring; It changes the API, please revert it.

If you'll notice, these effects don't have asset_issuer fields. By embedding the base.Asset you will be including that field.

@nullstyle
Copy link
Contributor

In the course of doing this, I also found that the offer effects: EffectOfferCreated, EffectOfferRemoved, and EffectOfferUpdated are not being created in the ingestEffects method I linked above, as their docstring might suggest. Are these effects an artifact from earlier times and should no longer be used?

We just need to implement them... the constants were simply defined before we got around to adding the effects into ingestion.

@bartekn
Copy link
Contributor

bartekn commented Mar 28, 2018

@martinxsliu would you mind adding support to offer related effects (so basically closing #166)?

@howardtw
Copy link
Contributor

@martinxsliu do you mind rebasing the PR? I believe InflationDestination for the AccountInflationDestinationUpdated effect has been added. I also have a PR adding offer related effects.

@bartekn
Copy link
Contributor

bartekn commented Sep 12, 2019

Closing due to age and ingestion system deprecation.

@bartekn bartekn closed this Sep 12, 2019
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.

6 participants