feat: ADR-004: Application links expiration time #723

leobragaz · 2022-01-17T16:39:51Z

Description

Closes: #516
Superseeds: #562

This PR introduce a new field ExpirationTime in the AppLinks object.
It will allow to automatically delete app links when they expires.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

TODO: - fix migrations - tests

- added tests for appLinkParams - fixed migration errors

- missing benchmarks

codecov · 2022-01-18T15:26:28Z

Codecov Report

Merging #723 (cd41847) into master (bc97e72) will decrease coverage by 1.92%.
The diff coverage is 73.12%.

@@            Coverage Diff             @@
##           master     #723      +/-   ##
==========================================
- Coverage   82.43%   80.51%   -1.93%     
==========================================
  Files         168      172       +4     
  Lines       14290    14818     +528     
==========================================
+ Hits        11780    11930     +150     
- Misses       2023     2380     +357     
- Partials      487      508      +21

Impacted Files	Coverage Δ
x/profiles/module.go	`10.12% <12.50%> (ø)`
x/profiles/legacy/v4/store.go	`61.53% <66.66%> (-22.77%)`	⬇️
x/profiles/legacy/v6/store.go	`67.04% <67.04%> (ø)`
x/profiles/legacy/v4/keeper.go	`73.79% <71.42%> (-9.66%)`	⬇️
x/profiles/keeper/alias_functions.go	`89.79% <80.00%> (-3.07%)`	⬇️
x/profiles/types/models_params.go	`83.95% <82.60%> (-0.45%)`	⬇️
x/profiles/abci.go	`100.00% <100.00%> (ø)`
x/profiles/keeper/keeper_app_links.go	`84.70% <100.00%> (+2.76%)`	⬆️
x/profiles/keeper/relay_app_links.go	`86.88% <100.00%> (+0.07%)`	⬆️
x/profiles/legacy/v5/store.go	`79.66% <100.00%> (ø)`
... and 7 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 9cd5262...cd41847. Read the comment docs.

fixed some bugs in the expiration time calculation

expiration_time params - fixed sims tests

added bench test workflow

leobragaz · 2022-01-24T10:49:07Z

x/profiles/module.go

-func (am AppModule) BeginBlock(_ sdk.Context, _ abci.RequestBeginBlock) {
+func (am AppModule) BeginBlock(ctx sdk.Context, _ abci.RequestBeginBlock) {
+	// TODO should we emit an event for this?
+	am.keeper.DeleteExpiredApplicationLinks(ctx)


should we emit some events for this?

Yes, we should emit one event for each deleted link. I don't know whether it would be better to do this here or inside the DeleteExpiredApplicationLinks method though

I think we can follow how we did with other keeper's methods and put it inside the DeleteExpiredApplicationLinks method.

leobragaz · 2022-01-24T10:54:05Z

x/profiles/types/models_params.go

+	DefaultMinDTagLength          = sdk.NewInt(3)
+	DefaultMaxDTagLength          = sdk.NewInt(30)
+	DefaultMaxBioLength           = sdk.NewInt(1000)
+	DefaultAppLinksExpirationTime = time.Hour * 24 * 7 * 4 * 6 // This duration is equal to roughly 5.5 months time in minutes 241920


I took the DefaultUnbondingTime param as an example:
https://github.com/cosmos/cosmos-sdk/blob/6ea204994443edc7841df4f0dc86ee5b4edc8377/x/staking/types/params.go#L21.

The expiration time default param is a duration of time of roughly 6 months.
Why roughly? Because not all months last the same.
So basically:
DefaultExpirationTime = 241920 minutes -> 5.5 months -> 168 days.

It's not a definitive default param, I would like to discuss with you what should be a reasonable amount of time before a link expires. IMO, it should not be less than 3 months.

We can set it to be 4382.91 hours considering one mean year has 8765.82 hours (see here). This could give us the duration closes to 6 months

Since we can't sum decimals in time params calculation I've added an ExpirationTimeCorrectionFactor that I calculated as follow:

4382.91 hours are 182 days;

DefaultAppLinksExpirationTime were 168 days;

182 - 168 = 14 days = 20160 minutes;

DefaultAppLinksExpirationTime + 20160 minutes = 262080 = 5.9835 Months.

added on-chain-upgrade test specs to on-chain-upgrade.yml

leobragaz · 2022-01-24T16:10:13Z

.github/workflows/bench.yml

@@ -0,0 +1,42 @@
+name: Benchmarks


This workflow uses benchstat a common go tool for comparing different benchmarks results.
In our case, the workflow will compare the master branch benchmark's result with the ones of a new PR which is IMO very useful to let us understand if we have introduced a performance regression.
In order to make this properly working, bench tests should be written when we consider it's the case.

…ation-impl' into leonardo/application-links-expiration-impl

Signed-off-by: Riccardo Montagnin <[email protected]>

RiccardoM · 2022-07-08T09:38:56Z

I've updated all the code and how the migrations are handled to work best. I would like to ask @dadamu to review the code and also fix the now failing test (it's always the one related to the JSON within the profiles module). Once that's done I think we can merge this

…application-links-expiration-impl

@bragaz

## Description This PR adds a new workflow to run benchmarks tests during each PR/commit to the master branch. Thanks to @bragaz for the original work done inside #723. --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://docs.cosmos.network/v0.44/building-modules/intro.html) - [ ] included the necessary unit and integration [tests](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [x] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

Signed-off-by: Riccardo Montagnin <[email protected]>

…application-links-expiration-impl

Signed-off-by: Riccardo Montagnin <[email protected]>

dadamu · 2022-07-15T06:50:08Z

x/profiles/keeper/alias_functions.go

+
+	i := int64(0)
+	for ; iterator.Valid(); iterator.Next() {
+		trimmedPrefixKey := bytes.TrimPrefix(iterator.Key(), types.ExpiringAppLinkTimePrefix)


Can you add a comment to describe Key() and Value() of iterator representing here?
It would be helpful to understand this closure.

Sure, added!

Signed-off-by: Riccardo Montagnin <[email protected]>

dadamu · 2022-07-15T07:13:23Z

x/profiles/keeper/keeper_app_links_benchmarks_test.go

+	b.Run("iterate and delete expired links", func(b *testing.B) {
+		for i := 0; i < b.N; i++ {
+			b.ReportAllocs()
+			profiles.BeginBlocker(ctx, k)


This method is out of keeper range, it would be better to have a specified function like DeleteExpiredApplicationLinks.

I've moved it to be inside the profiles package as it should be (since it's testing the BeginBlocker method). I removed the DeleteExpiredApplicationLinks as in my opinion it was pointless to have that method only so that we could write benchmark tests when we could simply have done it this way

Signed-off-by: Riccardo Montagnin <[email protected]>

dadamu · 2022-07-15T07:26:05Z

x/profiles/keeper/migrations.go

 	return v5.MigrateStore(ctx, m.keeper.storeKey, m.keeper.cdc, m.keeper.legacyAmino)
 }
+
+// Migrate6to7 migrates from version 6 to 7.
+func (m Migrator) Migrate6to7(ctx sdk.Context) error {


I am confused the version, v6.MigrateStore is actually migrating to v6 from v5 although the method is called Migrate6to7. Is that consensus version different from legacy store version?

That was actually an error within comments themselves. I've now updated them to make it more clear. Generally, the Migrate<X>to<X+1> calls the v<X> package that handles the migration from X to X+1 properly. So in this case Migrate6to7 calls the v6 package that migrates from 6 to 7 (latest version).

The only thing wrong here was the comment on the v6.MigrateStore function. It migrates from 6 to 7 (and not 5 to 6 as it was written). I've now fixed it

@RiccardoM Thanks for the explanation. But it would be another problem that v6.MigrateStore actually migrates v5types to the current type v6 like this.

Yeah, that is because inside the migration from v5 to v6 the types didn't change. So now we have to migrate from v5 to v7 (current version). I've updated the comments to make this more clear

Signed-off-by: Riccardo Montagnin <[email protected]>

dadamu · 2022-07-15T07:56:20Z

x/profiles/types/codec.go

@@ -40,14 +40,14 @@ func RegisterInterfaces(registry types.InterfaceRegistry) {
 	registry.RegisterImplementations((*exported.VestingAccount)(nil), &Profile{})
 	registry.RegisterImplementations((*authtypes.GenesisAccount)(nil), &Profile{})
 	registry.RegisterInterface(
-		"desmos.profiles.v2.AddressData",
+		"desmos.profiles.v3.AddressData",


Another question related to version. Should we set the version here to consensus version? e.g., desmos.profiles.v7.AddressData.

As well as the query route. To be like /desmos/profiles/v7/profiles/{user}

No, these types should reflect the Protobuf versioning (which is different from the module consensus versioning). We should update them only when we change the Protobuf definitions, as they change less often than the module consensus version (see from v4 to v5 where we didn't change a single Protobuf version but we had to do a migration to add new keys)

Signed-off-by: Riccardo Montagnin <[email protected]>

x/profiles/legacy/v6/store_test.go

Co-authored-by: Paul <[email protected]>

dadamu

Ready to go to me.

leobragaz added 2 commits January 17, 2022 16:09

- implemented expiration time for app links

d53e53b

TODO: - fix migrations - tests

- completed appLinksParams implementation

01991f4

- added tests for appLinkParams - fixed migration errors

github-actions bot added the x/profiles Module that allows to create and manage decentralized social profiles label Jan 17, 2022

- added tests for expiring links

e4b68b7

- missing benchmarks

github-actions bot added the x/CLI label Jan 18, 2022

leobragaz changed the title ~~Application links expiration time implementation~~ feat: Application links expiration time implementation Jan 19, 2022

leobragaz added 8 commits January 19, 2022 10:20

fixed proto file lint error

5d4d03a

reviewed migration code

85bdf16

added migrations tests (need to fix some)

edb7793

fixed migration errors

dfe9eba

fixed some bugs in the expiration time calculation

fixed params name in order to fix lint

fd1a7df

- switched from time.Time to time.Duration for

1328c82

expiration_time params - fixed sims tests

- added benchmark test for expired links deletion

1ef5251

fixed params migration

39449c6

added bench test workflow

github-actions bot added kind/build Related to the build of the project kind/ci Improve the CI/CD labels Jan 24, 2022

revert keeper's StoreKey and Cdc exports

054e230

leobragaz commented Jan 24, 2022

View reviewed changes

revert unwanted changes in chain links file

450d091

leobragaz changed the title ~~feat: Application links expiration time implementation~~ feat: requires-upgrade: Application links expiration time implementation Jan 24, 2022

leobragaz changed the title ~~feat: requires-upgrade: Application links expiration time implementation~~ requires-upgrade: Application links expiration time implementation Jan 24, 2022

leobragaz changed the title ~~requires-upgrade: Application links expiration time implementation~~ feat(requires-upgrade): Application links expiration time implementation Jan 24, 2022

leobragaz added 2 commits January 24, 2022 15:45

fixed params migration test

7cd38b4

added on-chain-upgrade test specs to on-chain-upgrade.yml

added changeset entry

ece318d

leobragaz changed the title ~~feat(requires-upgrade): Application links expiration time implementation~~ feat: Application links expiration time implementation Jan 24, 2022

leobragaz added the requires-upgrade Test the on-chain upgrade for this PR label Jan 24, 2022

edited genesis state link for on-chain-upgrade test

f3f8ca3

leobragaz commented Jan 24, 2022

View reviewed changes

RiccardoM added 3 commits July 8, 2022 09:29

Merge remote-tracking branch 'origin/leonardo/application-links-expir…

84ac02e

…ation-impl' into leonardo/application-links-expiration-impl

fixed small formatting

1aba69b

Signed-off-by: Riccardo Montagnin <[email protected]>

updated the migrations

355f21d

Signed-off-by: Riccardo Montagnin <[email protected]>

github-actions bot added the x/relationships label Jul 8, 2022

RiccardoM approved these changes Jul 8, 2022

View reviewed changes

Merge branch 'master' of github.com:desmos-labs/desmos into leonardo/…

3dccdc2

…application-links-expiration-impl

RiccardoM added the automerge Automatically merge PR once all prerequisites pass label Jul 14, 2022

dadamu and others added 4 commits July 14, 2022 16:37

test: fixed test sample of chain link cmd

1cd567c

fixed unit tests

7e8cc9c

Signed-off-by: Riccardo Montagnin <[email protected]>

Merge branch 'master' of github.com:desmos-labs/desmos into leonardo/…

5048d56

…application-links-expiration-impl

updated ADR status

189cfe1

Signed-off-by: Riccardo Montagnin <[email protected]>

github-actions bot added the kind/adr An issue or PR relating to an architectural decision record label Jul 15, 2022

slightly changed ADR

f002354

Signed-off-by: Riccardo Montagnin <[email protected]>