Problem: no well-tested implementation of subscriptions module (fix #295) #381

yihuang · 2021-02-23T07:39:16Z

Solution:
- implement basic calendar and crontab logic
- implement the module

👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻

PR Checklist:

Thank you for your code, it's appreciated! :)

This change is

yihuang · 2021-02-23T09:48:40Z

TODO:

add integration test
collect gas fees for create subscription
incentive for deleting plan to reclaim storage?
analyze overflow issue of arithmetic, consistent sign-ness of integers, constraints on user inputs.
cronspec shortcuts
Timezone

codecov-io · 2021-02-23T11:07:55Z

Codecov Report

Merging #381 (e68844c) into master (431f8d8) will increase coverage by 10.84%.
The diff coverage is 26.82%.

@@             Coverage Diff             @@
##           master     #381       +/-   ##
===========================================
+ Coverage   12.26%   23.10%   +10.84%     
===========================================
  Files          31       55       +24     
  Lines        4779     8742     +3963     
===========================================
+ Hits          586     2020     +1434     
- Misses       3935     6154     +2219     
- Partials      258      568      +310

Flag	Coverage Δ
integration_tests	`20.96% <22.06%> (+8.70%)`	⬆️
unit_tests	`11.38% <11.84%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
app/app.go	`3.67% <0.00%> (ø)`
app/genesis.go	`0.00% <0.00%> (ø)`
app/test_helpers.go	`0.00% <0.00%> (ø)`
x/subscription/types/codec.go	`0.00% <ø> (ø)`
x/subscription/types/genesis.go	`0.00% <0.00%> (ø)`
x/subscription/types/msgs.go	`12.31% <0.00%> (ø)`
x/subscription/types/params.go	`0.00% <ø> (ø)`
x/supply/types/query.pb.gw.go	`1.13% <ø> (+0.51%)`	⬆️
x/subscription/types/query.pb.gw.go	`0.38% <0.38%> (ø)`
x/subscription/types/genesis.pb.go	`0.69% <0.69%> (ø)`
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 431f8d8...e68844c. Read the comment docs.

third_party/proto/cosmos/base/abci/v1beta1/abci.proto

x/subscription/spec/03_messages.md

lgtm-com · 2021-02-24T03:15:54Z

This pull request introduces 4 alerts when merging b87e296 into 8764b74 - view on LGTM.com

new alerts:

4 for Incorrect conversion between integer types

lgtm-com · 2021-02-24T03:45:48Z

This pull request introduces 4 alerts when merging e62c7cc into 8764b74 - view on LGTM.com

new alerts:

4 for Incorrect conversion between integer types

lgtm-com · 2021-02-24T08:18:12Z

This pull request introduces 4 alerts when merging 2b8692f into 8764b74 - view on LGTM.com

new alerts:

4 for Incorrect conversion between integer types

proto/subscription/v1beta1/subscription.proto

x/subscription/client/cli/tx.go

lgtm-com · 2021-02-26T02:02:33Z

This pull request introduces 4 alerts when merging 1d075c0 into 91e5405 - view on LGTM.com

new alerts:

4 for Incorrect conversion between integer types

lgtm-com · 2021-02-26T02:38:22Z

This pull request introduces 4 alerts when merging b5dbefc into 91e5405 - view on LGTM.com

new alerts:

4 for Incorrect conversion between integer types

lgtm-com · 2021-02-26T03:00:50Z

This pull request introduces 4 alerts when merging 4ae9efd into 91e5405 - view on LGTM.com

new alerts:

4 for Incorrect conversion between integer types

lgtm-com · 2021-02-26T06:09:59Z

This pull request introduces 4 alerts when merging 0d42e76 into 66b3503 - view on LGTM.com

new alerts:

4 for Incorrect conversion between integer types

tomtau · 2021-03-01T03:55:32Z

there are a few conflicts with the latest master

yihuang · 2021-03-01T04:02:04Z

there are a few conflicts with the latest master

done

yihuang · 2021-03-06T18:43:05Z

Question for the KVStore in Keeper, I guess it uses the KVStore of the cacheMultiStore, so when the keeper sets the data with keys to the KVStore, when these data will be flushed into the disk?

it's here, the abci commit event happens once for each block, at the end of block processing.

yihuang · 2021-03-08T01:31:20Z

could you squash into one commit?

done

leejw51crypto · 2021-03-08T01:47:49Z

message MsgCreateSubscriptionResponse {}
how about returning subscription id? like plan_id when creating plan?
tzoffset as int32?
Plan, MsgCreatePlan,
uint32 tzoffset = 7; // in seconds
how about int32? ,
because it can be UTC plus or minus ?

tomtau · 2021-03-08T02:36:24Z

proto/subscription/v1/query.proto

+    option (google.api.http).get = "/chainmain/subscription/v1beta1/plan/{plan_id}";
+  }
+
+  rpc Plans(QueryPlansRequest) returns (QueryPlansResponse) {
+    option (google.api.http).get = "/chainmain/subscription/v1beta1/plans";
+  }
+
+  rpc Subscription(QuerySubscriptionRequest) returns (QuerySubscriptionResponse) {
+    option (google.api.http).get = "/chainmain/subscription/v1beta1/subscription/{subscription_id}";
+  }
+
+  rpc Subscriptions(QuerySubscriptionsRequest) returns (QuerySubscriptionsResponse) {
+    option (google.api.http).get = "/chainmain/subscription/v1beta1/subscriptions";
+  }
+
+  rpc Params(QueryParamsRequest) returns (QueryParamsResponse) {
+    option (google.api.http).get = "/chainmain/subscription/v1beta1/params";


v1 instead of v1beta1?

tomtau · 2021-03-08T03:32:54Z

x/subscription/client/cli/tx.go

+			if err != nil {
+				return err
+			}
+			// Get voting address


owner address instead of voting address?

tomtau · 2021-03-08T03:50:03Z

x/subscription/keeper/keeper.go

+
+	var gasPerCollection uint32
+	k.paramSpace.Get(ctx, types.KeyGasPerCollection, &gasPerCollection)
+	ctx.GasMeter().ConsumeGas(sdk.Gas(gasPerCollection), "create subscription")


in spec, it says:

ConsumeGas( count_period(create_time, expiration_time, plan.cron_spec, plan.cron_tz) * GasPerCollection )

but here, it'll just charge a fixed param gasPerCollection?

yes, forgot that, added now.

tomtau · 2021-03-08T03:58:28Z

x/subscription/keeper/keeper.go

+	ctx.EventManager().EmitEvents(sdk.Events{
+		sdk.NewEvent(
+			types.EventTypeCreatePlan,
+			sdk.NewAttribute(types.AttributeKeyPlanID, strconv.FormatInt(int64(plan.PlanId), 10)),


what happens in these event emitting if PlanId/SubscriptionId doesn't fit in int64?

There's actually a FormatUint, I just changed to that.
But still, I think when PlanId/SubscriptionId overflow, it'll wrap around, could possibly cause id conflict.
Maybe it's better to just stop creating new items when this happens?

EDIT: added id overflow detection, return error when happend.

tomtau · 2021-03-08T04:20:45Z

x/subscription/types/calendar.go

+
+	qCycles := remDays / daysPer4Y
+	if qCycles == 25 {
+		// TODO I think this path is never reached


maybe year 2074 or something like that?

I tried this one, it don't reach here too, I've tried lots of cases. 😄

yihuang · 2021-03-08T05:27:38Z

message MsgCreateSubscriptionResponse {}
how about returning subscription id? like plan_id when creating plan?

added

tzoffset as int32?
Plan, MsgCreatePlan,
uint32 tzoffset = 7; // in seconds
how about int32? ,
because it can be UTC plus or minus ?

good idea, added now.

JayT106 · 2021-03-08T14:53:34Z

Question for the KVStore in Keeper, I guess it uses the KVStore of the cacheMultiStore, so when the keeper sets the data with keys to the KVStore, when these data will be flushed into the disk?

it's here, the abci commit event happens once for each block, at the end of block processing.

Thanks!

leejw51crypto · 2021-03-09T04:13:07Z

abc.go, func BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock, k keeper.Keeper) {
..
these times are all utc?
could you comment about it?
BlockTime().Unix() unit is seconds?

blockTime := uint64(ctx.BlockTime().Unix())
if blockTime >= subscription.NextCollectionTime {

leejw51crypto · 2021-03-09T04:15:23Z

repeated cosmos.base.v1beta1.Coin

could you modify to cosmos.base.v1.Coin?

leejw51crypto

lgtm

yihuang · 2021-03-09T06:24:20Z

repeated cosmos.base.v1beta1.Coin
could you modify to cosmos.base.v1.Coin?

This namespace is defined in cosmos-sdk, I don't think we can change it ;D

yihuang · 2021-03-09T06:25:15Z

abc.go, func BeginBlocker(ctx sdk.Context, req abci.RequestBeginBlock, k keeper.Keeper) {
..
these times are all utc?
could you comment about it?
BlockTime().Unix() unit is seconds?
blockTime := uint64(ctx.BlockTime().Unix())
if blockTime >= subscription.NextCollectionTime {		

Yeah, these are all unix timestamps in seconds.

JayT106

lgtm

tomtau

we can probably merge this by Friday -- so may be good to resolve conflicts + changelog entry for unreleased 2.0.0?

tomtau · 2021-03-17T01:22:01Z

@yihuang one more test if it can be added -- init a Chain with https://github.com/crypto-org-chain/chain-main/releases/tag/v1.1.0 and then cosmovisor upgrade for v2 with subscriptions? probably some handler to init storage needs to be added?

yihuang · 2021-03-17T06:43:33Z

@yihuang one more test if it can be added -- init a Chain with https://github.com/crypto-org-chain/chain-main/releases/tag/v1.1.0 and then cosmovisor upgrade for v2 with subscriptions? probably some handler to init storage needs to be added?

Done, reused the test_cosmovisor test case, migration seems happen smoothly.

CHANGELOG.md

…rypto-org-chain#295) Solution: - implement basic calendar and crontab logic - implement the module add subscription integrationt test check SubscriptionEnabled parameter support timezone fix event emmision add README for implementation notes fix wording in comments and namings Change org name in proto package name Update app/app.go Co-authored-by: Tomas Tauber <2410580+tomtau@users.noreply.github.com> assert plan owner's balance add missing tzoffset to spec Add comments for storage Use more explicit ParseUint validate genesis state Custom bitset implementation lint issues improve coverage of calendar/cronspec/bitset improve coverage of keeper Add license/copyright of bitset v1beta -> v1 update sign-ness of types in spec quick test SecsToTM fix proto version name and comment change tzoffset to signed int32 return subscription_id in CreateSubscription api multiple CountPeriods when consume gas FormatInt -> FormatUint handle planID/subscriptionID overflow test count periods add v2.0.0 in CHANGELOG Extend cosmovior integration tests to test v2.0.0 migration

yihuang requested review from calvinaco and tomtau as code owners February 23, 2021 07:39

tomtau reviewed Feb 24, 2021

View reviewed changes

third_party/proto/cosmos/base/abci/v1beta1/abci.proto Outdated Show resolved Hide resolved

tomtau reviewed Feb 24, 2021

View reviewed changes

x/subscription/spec/03_messages.md Outdated Show resolved Hide resolved

yihuang force-pushed the subscription-impl branch 2 times, most recently from 1e01a29 to e62c7cc Compare February 24, 2021 03:30

yihuang force-pushed the subscription-impl branch from 7a9d1e2 to 2b8692f Compare February 24, 2021 08:02

tomtau requested review from devashishdxt and leejw51crypto February 24, 2021 10:18

devashishdxt reviewed Feb 25, 2021

View reviewed changes

proto/subscription/v1beta1/subscription.proto Outdated Show resolved Hide resolved

x/subscription/client/cli/tx.go Outdated Show resolved Hide resolved

yihuang mentioned this pull request Feb 25, 2021

Problem: subscription spec is not revised for implementation #389

Merged

13 tasks

yihuang force-pushed the subscription-impl branch from 2b8692f to 1d075c0 Compare February 26, 2021 01:40

yihuang force-pushed the subscription-impl branch 2 times, most recently from 49b3529 to b5dbefc Compare February 26, 2021 02:15

yihuang force-pushed the subscription-impl branch from b5dbefc to 4ae9efd Compare February 26, 2021 02:39

devashishdxt approved these changes Feb 26, 2021

View reviewed changes

tomtau requested a review from linfeng-crypto February 26, 2021 04:25

yihuang force-pushed the subscription-impl branch from 4ae9efd to 808c38f Compare February 26, 2021 05:36

tomtau requested a review from JayT106 March 1, 2021 03:54

yihuang force-pushed the subscription-impl branch from 9533c1e to 7b1f42b Compare March 6, 2021 18:53

tomtau reviewed Mar 8, 2021

View reviewed changes

leejw51crypto approved these changes Mar 9, 2021

View reviewed changes

JayT106 approved these changes Mar 9, 2021

View reviewed changes

linfeng-crypto approved these changes Mar 11, 2021

View reviewed changes

tomtau self-requested a review March 16, 2021 09:26

tomtau suggested changes Mar 16, 2021

View reviewed changes

yihuang force-pushed the subscription-impl branch from e9c1f20 to 70cb197 Compare March 17, 2021 06:43

tomtau reviewed Mar 17, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

yihuang force-pushed the subscription-impl branch from 0af05d1 to be24f1f Compare March 17, 2021 07:12

fix test case test_manual_export

47923ff

yihuang force-pushed the subscription-impl branch from 37f223f to 47923ff Compare March 17, 2021 12:56

Merge branch 'master' into subscription-impl

e68844c

tomtau merged commit 22f169d into crypto-org-chain:master Mar 19, 2021

Problem: no well-tested implementation of subscriptions module (fix #295) #381

Problem: no well-tested implementation of subscriptions module (fix #295) #381

Conversation

yihuang commented Feb 23, 2021 • edited by tomtau Loading

PR Checklist:

yihuang commented Feb 23, 2021 • edited Loading

codecov-io commented Feb 23, 2021 • edited by codecov bot Loading

Codecov Report

lgtm-com bot commented Feb 24, 2021

lgtm-com bot commented Feb 24, 2021

lgtm-com bot commented Feb 24, 2021

lgtm-com bot commented Feb 26, 2021

lgtm-com bot commented Feb 26, 2021

lgtm-com bot commented Feb 26, 2021

lgtm-com bot commented Feb 26, 2021

tomtau commented Mar 1, 2021

yihuang commented Mar 1, 2021

yihuang commented Mar 6, 2021

yihuang commented Mar 8, 2021

leejw51crypto commented Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Mar 8, 2021 • edited Loading

Choose a reason for hiding this comment

yihuang commented Mar 8, 2021

JayT106 commented Mar 8, 2021

leejw51crypto commented Mar 9, 2021 • edited Loading

leejw51crypto commented Mar 9, 2021 • edited Loading

leejw51crypto left a comment

Choose a reason for hiding this comment

yihuang commented Mar 9, 2021

yihuang commented Mar 9, 2021

JayT106 left a comment

Choose a reason for hiding this comment

tomtau left a comment

Choose a reason for hiding this comment

tomtau commented Mar 17, 2021

yihuang commented Mar 17, 2021 • edited Loading

yihuang commented Feb 23, 2021 •

edited by tomtau

Loading

yihuang commented Feb 23, 2021 •

edited

Loading

codecov-io commented Feb 23, 2021 •

edited by codecov bot

Loading

leejw51crypto commented Mar 8, 2021 •

edited

Loading

yihuang Mar 8, 2021 •

edited

Loading

yihuang Mar 8, 2021 •

edited

Loading

leejw51crypto commented Mar 9, 2021 •

edited

Loading

leejw51crypto commented Mar 9, 2021 •

edited

Loading

yihuang commented Mar 17, 2021 •

edited

Loading