From cd7a32dffb60da37962ce58675b00a7e75bc255c Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Thu, 22 Apr 2021 12:15:26 +0200 Subject: [PATCH 1/2] Test consistency and coverage of effect typenames (and cleanup unused effects) --- protocols/horizon/effects/main.go | 84 ++++++++++--------- services/horizon/internal/db2/history/main.go | 9 +- .../internal/resourceadapter/effects.go | 75 +++++++++-------- .../internal/resourceadapter/effects_test.go | 40 +++++++++ 4 files changed, 127 insertions(+), 81 deletions(-) diff --git a/protocols/horizon/effects/main.go b/protocols/horizon/effects/main.go index 28006e4fe7..b7530a9bca 100644 --- a/protocols/horizon/effects/main.go +++ b/protocols/horizon/effects/main.go @@ -43,9 +43,10 @@ const ( // account flags, either clearing or setting. EffectAccountFlagsUpdated EffectType = 6 // from set_options + // unused // EffectAccountInflationDestinationUpdated effects occur when an account changes its // inflation destination. - EffectAccountInflationDestinationUpdated EffectType = 7 // from set_options + // EffectAccountInflationDestinationUpdated EffectType = 7 // from set_options // signer effects @@ -92,14 +93,13 @@ const ( // trading effects + // unused // EffectOfferCreated occurs when an account offers to trade an asset - EffectOfferCreated EffectType = 30 // from manage_offer, creat_passive_offer - + // EffectOfferCreated EffectType = 30 // from manage_offer, creat_passive_offer // EffectOfferRemoved occurs when an account removes an offer - EffectOfferRemoved EffectType = 31 // from manage_offer, creat_passive_offer, path_payment - + // EffectOfferRemoved EffectType = 31 // from manage_offer, creat_passive_offer, path_payment // EffectOfferUpdated occurs when an offer is updated by the offering account. - EffectOfferUpdated EffectType = 32 // from manage_offer, creat_passive_offer, path_payment + // EffectOfferUpdated EffectType = 32 // from manage_offer, creat_passive_offer, path_payment // EffectTrade occurs when a trade is initiated because of a path payment or // offer operation. @@ -190,14 +190,15 @@ const ( // EffectTypeNames stores a map of effect type ID and names var EffectTypeNames = map[EffectType]string{ - EffectAccountCreated: "account_created", - EffectAccountRemoved: "account_removed", - EffectAccountCredited: "account_credited", - EffectAccountDebited: "account_debited", - EffectAccountThresholdsUpdated: "account_thresholds_updated", - EffectAccountHomeDomainUpdated: "account_home_domain_updated", - EffectAccountFlagsUpdated: "account_flags_updated", - EffectAccountInflationDestinationUpdated: "account_inflation_destination_updated", + EffectAccountCreated: "account_created", + EffectAccountRemoved: "account_removed", + EffectAccountCredited: "account_credited", + EffectAccountDebited: "account_debited", + EffectAccountThresholdsUpdated: "account_thresholds_updated", + EffectAccountHomeDomainUpdated: "account_home_domain_updated", + EffectAccountFlagsUpdated: "account_flags_updated", + // unused + // EffectAccountInflationDestinationUpdated: "account_inflation_destination_updated", EffectSignerCreated: "signer_created", EffectSignerRemoved: "signer_removed", EffectSignerUpdated: "signer_updated", @@ -208,33 +209,34 @@ var EffectTypeNames = map[EffectType]string{ EffectTrustlineAuthorizedToMaintainLiabilities: "trustline_authorized_to_maintain_liabilities", EffectTrustlineDeauthorized: "trustline_deauthorized", EffectTrustlineFlagsUpdated: "trustline_flags_updated", - EffectOfferCreated: "offer_created", - EffectOfferRemoved: "offer_removed", - EffectOfferUpdated: "offer_updated", - EffectTrade: "trade", - EffectDataCreated: "data_created", - EffectDataRemoved: "data_removed", - EffectDataUpdated: "data_updated", - EffectSequenceBumped: "sequence_bumped", - EffectClaimableBalanceCreated: "claimable_balance_created", - EffectClaimableBalanceClaimed: "claimable_balance_claimed", - EffectClaimableBalanceClaimantCreated: "claimable_balance_claimant_created", - EffectAccountSponsorshipCreated: "account_sponsorship_created", - EffectAccountSponsorshipUpdated: "account_sponsorship_updated", - EffectAccountSponsorshipRemoved: "account_sponsorship_removed", - EffectTrustlineSponsorshipCreated: "trustline_sponsorship_created", - EffectTrustlineSponsorshipUpdated: "trustline_sponsorship_updated", - EffectTrustlineSponsorshipRemoved: "trustline_sponsorship_removed", - EffectDataSponsorshipCreated: "data_sponsorship_created", - EffectDataSponsorshipUpdated: "data_sponsorship_updated", - EffectDataSponsorshipRemoved: "data_sponsorship_removed", - EffectClaimableBalanceSponsorshipCreated: "claimable_balance_sponsorship_created", - EffectClaimableBalanceSponsorshipUpdated: "claimable_balance_sponsorship_updated", - EffectClaimableBalanceSponsorshipRemoved: "claimable_balance_sponsorship_removed", - EffectSignerSponsorshipCreated: "signer_sponsorship_created", - EffectSignerSponsorshipUpdated: "signer_sponsorship_updated", - EffectSignerSponsorshipRemoved: "signer_sponsorship_removed", - EffectClaimableBalanceClawedBack: "claimable_balance_clawed_back", + // unused + // EffectOfferCreated: "offer_created", + // EffectOfferRemoved: "offer_removed", + // EffectOfferUpdated: "offer_updated", + EffectTrade: "trade", + EffectDataCreated: "data_created", + EffectDataRemoved: "data_removed", + EffectDataUpdated: "data_updated", + EffectSequenceBumped: "sequence_bumped", + EffectClaimableBalanceCreated: "claimable_balance_created", + EffectClaimableBalanceClaimed: "claimable_balance_claimed", + EffectClaimableBalanceClaimantCreated: "claimable_balance_claimant_created", + EffectAccountSponsorshipCreated: "account_sponsorship_created", + EffectAccountSponsorshipUpdated: "account_sponsorship_updated", + EffectAccountSponsorshipRemoved: "account_sponsorship_removed", + EffectTrustlineSponsorshipCreated: "trustline_sponsorship_created", + EffectTrustlineSponsorshipUpdated: "trustline_sponsorship_updated", + EffectTrustlineSponsorshipRemoved: "trustline_sponsorship_removed", + EffectDataSponsorshipCreated: "data_sponsorship_created", + EffectDataSponsorshipUpdated: "data_sponsorship_updated", + EffectDataSponsorshipRemoved: "data_sponsorship_removed", + EffectClaimableBalanceSponsorshipCreated: "claimable_balance_sponsorship_created", + EffectClaimableBalanceSponsorshipUpdated: "claimable_balance_sponsorship_updated", + EffectClaimableBalanceSponsorshipRemoved: "claimable_balance_sponsorship_removed", + EffectSignerSponsorshipCreated: "signer_sponsorship_created", + EffectSignerSponsorshipUpdated: "signer_sponsorship_updated", + EffectSignerSponsorshipRemoved: "signer_sponsorship_removed", + EffectClaimableBalanceClawedBack: "claimable_balance_clawed_back", } // Base provides the common structure for any effect resource effect. diff --git a/services/horizon/internal/db2/history/main.go b/services/horizon/internal/db2/history/main.go index e5ac0903e5..4da6945006 100644 --- a/services/horizon/internal/db2/history/main.go +++ b/services/horizon/internal/db2/history/main.go @@ -97,14 +97,13 @@ const ( // trading effects + // unused // EffectOfferCreated occurs when an account offers to trade an asset - EffectOfferCreated EffectType = 30 // from manage_offer, creat_passive_offer - + // EffectOfferCreated EffectType = 30 // from manage_offer, creat_passive_offer // EffectOfferRemoved occurs when an account removes an offer - EffectOfferRemoved EffectType = 31 // from manage_offer, create_passive_offer, path_payment - + // EffectOfferRemoved EffectType = 31 // from manage_offer, create_passive_offer, path_payment // EffectOfferUpdated occurs when an offer is updated by the offering account. - EffectOfferUpdated EffectType = 32 // from manage_offer, creat_passive_offer, path_payment + // EffectOfferUpdated EffectType = 32 // from manage_offer, creat_passive_offer, path_payment // EffectTrade occurs when a trade is initiated because of a path payment or // offer operation. diff --git a/services/horizon/internal/resourceadapter/effects.go b/services/horizon/internal/resourceadapter/effects.go index 9a8121ff07..1f6cd27fb4 100644 --- a/services/horizon/internal/resourceadapter/effects.go +++ b/services/horizon/internal/resourceadapter/effects.go @@ -11,14 +11,15 @@ import ( ) var EffectTypeNames = map[history.EffectType]string{ - history.EffectAccountCreated: "account_created", - history.EffectAccountRemoved: "account_removed", - history.EffectAccountCredited: "account_credited", - history.EffectAccountDebited: "account_debited", - history.EffectAccountThresholdsUpdated: "account_thresholds_updated", - history.EffectAccountHomeDomainUpdated: "account_home_domain_updated", - history.EffectAccountFlagsUpdated: "account_flags_updated", - history.EffectAccountInflationDestinationUpdated: "account_inflation_destination_updated", + history.EffectAccountCreated: "account_created", + history.EffectAccountRemoved: "account_removed", + history.EffectAccountCredited: "account_credited", + history.EffectAccountDebited: "account_debited", + history.EffectAccountThresholdsUpdated: "account_thresholds_updated", + history.EffectAccountHomeDomainUpdated: "account_home_domain_updated", + history.EffectAccountFlagsUpdated: "account_flags_updated", + // unused + // history.EffectAccountInflationDestinationUpdated: "account_inflation_destination_updated", history.EffectSignerCreated: "signer_created", history.EffectSignerRemoved: "signer_removed", history.EffectSignerUpdated: "signer_updated", @@ -29,33 +30,34 @@ var EffectTypeNames = map[history.EffectType]string{ history.EffectTrustlineAuthorizedToMaintainLiabilities: "trustline_authorized_to_maintain_liabilities", history.EffectTrustlineDeauthorized: "trustline_deauthorized", history.EffectTrustlineFlagsUpdated: "trustline_flags_updated", - history.EffectOfferCreated: "offer_created", - history.EffectOfferRemoved: "offer_removed", - history.EffectOfferUpdated: "offer_updated", - history.EffectTrade: "trade", - history.EffectDataCreated: "data_created", - history.EffectDataRemoved: "data_removed", - history.EffectDataUpdated: "data_updated", - history.EffectSequenceBumped: "sequence_bumped", - history.EffectClaimableBalanceCreated: "claimable_balance_created", - history.EffectClaimableBalanceClaimantCreated: "claimable_balance_claimant_created", - history.EffectClaimableBalanceClaimed: "claimable_balance_claimed", - history.EffectAccountSponsorshipCreated: "account_sponsorship_created", - history.EffectAccountSponsorshipUpdated: "account_sponsorship_updated", - history.EffectAccountSponsorshipRemoved: "account_sponsorship_removed", - history.EffectTrustlineSponsorshipCreated: "trustline_sponsorship_created", - history.EffectTrustlineSponsorshipUpdated: "trustline_sponsorship_updated", - history.EffectTrustlineSponsorshipRemoved: "trustline_sponsorship_removed", - history.EffectDataSponsorshipCreated: "data_sponsorship_created", - history.EffectDataSponsorshipUpdated: "data_sponsorship_updated", - history.EffectDataSponsorshipRemoved: "data_sponsorship_removed", - history.EffectClaimableBalanceSponsorshipCreated: "claimable_balance_sponsorship_created", - history.EffectClaimableBalanceSponsorshipUpdated: "claimable_balance_sponsorship_updated", - history.EffectClaimableBalanceSponsorshipRemoved: "claimable_balance_sponsorship_removed", - history.EffectSignerSponsorshipCreated: "signer_sponsorship_created", - history.EffectSignerSponsorshipUpdated: "signer_sponsorship_updated", - history.EffectSignerSponsorshipRemoved: "signer_sponsorship_removed", - history.EffectClaimableBalanceClawedBack: "claimable_balance_clawed_back", + // unused + // history.EffectOfferCreated: "offer_created", + // history.EffectOfferRemoved: "offer_removed", + // history.EffectOfferUpdated: "offer_updated", + history.EffectTrade: "trade", + history.EffectDataCreated: "data_created", + history.EffectDataRemoved: "data_removed", + history.EffectDataUpdated: "data_updated", + history.EffectSequenceBumped: "sequence_bumped", + history.EffectClaimableBalanceCreated: "claimable_balance_created", + history.EffectClaimableBalanceClaimantCreated: "claimable_balance_claimant_created", + history.EffectClaimableBalanceClaimed: "claimable_balance_claimed", + history.EffectAccountSponsorshipCreated: "account_sponsorship_created", + history.EffectAccountSponsorshipUpdated: "account_sponsorship_updated", + history.EffectAccountSponsorshipRemoved: "account_sponsorship_removed", + history.EffectTrustlineSponsorshipCreated: "trustline_sponsorship_created", + history.EffectTrustlineSponsorshipUpdated: "trustline_sponsorship_updated", + history.EffectTrustlineSponsorshipRemoved: "trustline_sponsorship_removed", + history.EffectDataSponsorshipCreated: "data_sponsorship_created", + history.EffectDataSponsorshipUpdated: "data_sponsorship_updated", + history.EffectDataSponsorshipRemoved: "data_sponsorship_removed", + history.EffectClaimableBalanceSponsorshipCreated: "claimable_balance_sponsorship_created", + history.EffectClaimableBalanceSponsorshipUpdated: "claimable_balance_sponsorship_updated", + history.EffectClaimableBalanceSponsorshipRemoved: "claimable_balance_sponsorship_removed", + history.EffectSignerSponsorshipCreated: "signer_sponsorship_created", + history.EffectSignerSponsorshipUpdated: "signer_sponsorship_updated", + history.EffectSignerSponsorshipRemoved: "signer_sponsorship_removed", + history.EffectClaimableBalanceClawedBack: "claimable_balance_clawed_back", } // NewEffect creates a new effect resource from the provided database representation @@ -247,6 +249,9 @@ func NewEffect( e := effects.ClaimableBalanceClawedBack{Base: basev} err = row.UnmarshalDetails(&e) result = e + case history.EffectAccountRemoved: + // there is no explicit data structure for account removed + fallthrough default: result = basev } diff --git a/services/horizon/internal/resourceadapter/effects_test.go b/services/horizon/internal/resourceadapter/effects_test.go index 794dba41be..1e642b1623 100644 --- a/services/horizon/internal/resourceadapter/effects_test.go +++ b/services/horizon/internal/resourceadapter/effects_test.go @@ -1,6 +1,7 @@ package resourceadapter import ( + "context" "encoding/json" "testing" @@ -12,6 +13,45 @@ import ( "github.com/stretchr/testify/assert" ) +func TestNewEffectAllEffectsCovered(t *testing.T) { + for typ, s := range EffectTypeNames { + if typ == history.EffectAccountRemoved { + // account removed uses the base effect + continue + } + e := history.Effect{ + Type: typ, + } + result, err := NewEffect(context.TODO(), e, history.Ledger{}) + assert.NoError(t, err, s) + // it shouldn't be a base type + _, ok := result.(effects.Base) + assert.False(t, ok, s) + } + + // verify that the check works for an unknown effect + e := history.Effect{ + Type: 20000, + } + result, err := NewEffect(context.TODO(), e, history.Ledger{}) + assert.NoError(t, err) + _, ok := result.(effects.Base) + assert.True(t, ok) +} + +func TestEffectTypeNamesAreConsistentWithAdapterTypeNames(t *testing.T) { + for typ, s := range EffectTypeNames { + s2, ok := effects.EffectTypeNames[effects.EffectType(typ)] + assert.True(t, ok, s) + assert.Equal(t, s, s2) + } + for typ, s := range effects.EffectTypeNames { + s2, ok := EffectTypeNames[history.EffectType(typ)] + assert.True(t, ok, s) + assert.Equal(t, s, s2) + } +} + func TestNewEffect_EffectTrustlineAuthorizedToMaintainLiabilities(t *testing.T) { tt := assert.New(t) ctx, _ := test.ContextWithLogBuffer() From 483a6906df757f8c820b1f45a597534d8fa690c9 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Mon, 26 Apr 2021 19:30:53 +0200 Subject: [PATCH 2/2] Address review feedback --- protocols/horizon/effects/main.go | 19 +++++++++---------- .../internal/resourceadapter/effects.go | 17 ++++++++--------- .../internal/resourceadapter/effects_test.go | 4 ++-- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/protocols/horizon/effects/main.go b/protocols/horizon/effects/main.go index b7530a9bca..1e5cb54590 100644 --- a/protocols/horizon/effects/main.go +++ b/protocols/horizon/effects/main.go @@ -46,7 +46,7 @@ const ( // unused // EffectAccountInflationDestinationUpdated effects occur when an account changes its // inflation destination. - // EffectAccountInflationDestinationUpdated EffectType = 7 // from set_options + EffectAccountInflationDestinationUpdated EffectType = 7 // from set_options // signer effects @@ -190,15 +190,14 @@ const ( // EffectTypeNames stores a map of effect type ID and names var EffectTypeNames = map[EffectType]string{ - EffectAccountCreated: "account_created", - EffectAccountRemoved: "account_removed", - EffectAccountCredited: "account_credited", - EffectAccountDebited: "account_debited", - EffectAccountThresholdsUpdated: "account_thresholds_updated", - EffectAccountHomeDomainUpdated: "account_home_domain_updated", - EffectAccountFlagsUpdated: "account_flags_updated", - // unused - // EffectAccountInflationDestinationUpdated: "account_inflation_destination_updated", + EffectAccountCreated: "account_created", + EffectAccountRemoved: "account_removed", + EffectAccountCredited: "account_credited", + EffectAccountDebited: "account_debited", + EffectAccountThresholdsUpdated: "account_thresholds_updated", + EffectAccountHomeDomainUpdated: "account_home_domain_updated", + EffectAccountFlagsUpdated: "account_flags_updated", + EffectAccountInflationDestinationUpdated: "account_inflation_destination_updated", EffectSignerCreated: "signer_created", EffectSignerRemoved: "signer_removed", EffectSignerUpdated: "signer_updated", diff --git a/services/horizon/internal/resourceadapter/effects.go b/services/horizon/internal/resourceadapter/effects.go index 1f6cd27fb4..ab9fdc9867 100644 --- a/services/horizon/internal/resourceadapter/effects.go +++ b/services/horizon/internal/resourceadapter/effects.go @@ -11,15 +11,14 @@ import ( ) var EffectTypeNames = map[history.EffectType]string{ - history.EffectAccountCreated: "account_created", - history.EffectAccountRemoved: "account_removed", - history.EffectAccountCredited: "account_credited", - history.EffectAccountDebited: "account_debited", - history.EffectAccountThresholdsUpdated: "account_thresholds_updated", - history.EffectAccountHomeDomainUpdated: "account_home_domain_updated", - history.EffectAccountFlagsUpdated: "account_flags_updated", - // unused - // history.EffectAccountInflationDestinationUpdated: "account_inflation_destination_updated", + history.EffectAccountCreated: "account_created", + history.EffectAccountRemoved: "account_removed", + history.EffectAccountCredited: "account_credited", + history.EffectAccountDebited: "account_debited", + history.EffectAccountThresholdsUpdated: "account_thresholds_updated", + history.EffectAccountHomeDomainUpdated: "account_home_domain_updated", + history.EffectAccountFlagsUpdated: "account_flags_updated", + history.EffectAccountInflationDestinationUpdated: "account_inflation_destination_updated", history.EffectSignerCreated: "signer_created", history.EffectSignerRemoved: "signer_removed", history.EffectSignerUpdated: "signer_updated", diff --git a/services/horizon/internal/resourceadapter/effects_test.go b/services/horizon/internal/resourceadapter/effects_test.go index 1e642b1623..3e95a329bd 100644 --- a/services/horizon/internal/resourceadapter/effects_test.go +++ b/services/horizon/internal/resourceadapter/effects_test.go @@ -15,8 +15,8 @@ import ( func TestNewEffectAllEffectsCovered(t *testing.T) { for typ, s := range EffectTypeNames { - if typ == history.EffectAccountRemoved { - // account removed uses the base effect + if typ == history.EffectAccountRemoved || typ == history.EffectAccountInflationDestinationUpdated { + // these effects use the base representation continue } e := history.Effect{