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

Remove profile duplicated validation #557

Merged
merged 16 commits into from
Aug 6, 2021

Conversation

leobragaz
Copy link
Contributor

Description

This PR remove the double validation made inside Profile Update() function leaving only that
inside the keeper Validate().
Closes #548

Checklist

  • Targeted PR against correct branch.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit tests.
  • Wrote integration tests (simulation & CLI).
  • Updated the documentation.
  • Added an entry to the CHANGELOG.md file.
  • Re-reviewed Files changed in the Github PR explorer.

@codecov
Copy link

codecov bot commented Jul 13, 2021

Codecov Report

Merging #557 (9d672d4) into master (fa26783) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
- Coverage   82.84%   82.83%   -0.01%     
==========================================
  Files          91       91              
  Lines        5480     5477       -3     
==========================================
- Hits         4540     4537       -3     
  Misses        737      737              
  Partials      203      203              
Impacted Files Coverage Δ
x/profiles/types/account.go 56.16% <ø> (-0.89%) ⬇️
x/profiles/keeper/msgs_server_profile.go 82.22% <0.00%> (ø)

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 fa26783...9d672d4. Read the comment docs.

Copy link
Contributor

@dadamu dadamu left a comment

Choose a reason for hiding this comment

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

LGTM

@RiccardoM
Copy link
Contributor

Re-checking this, to be honest I'm not sure whether we should have this removed inside the Update function or not. Ideally, that function can be used by third party developers as well, and those checks are very quick to perform. For this reason, I don't think there's the necessity to remove it at all. We could simply leave it here. Before jumping to the conclusions, though, is it possible to have some data? Like, could we measure the impact on performances that this double check has? I think this should be farily easy to do and it could give us a nice idea whether it is worth or not removing the check.

@leobragaz
Copy link
Contributor Author

leobragaz commented Jul 26, 2021

@RiccardoM I've performed a bench test on Update() and these are the results:

with validation:

goos: darwin
goarch: amd64
pkg: github.com/desmos-labs/desmos/x/profiles/types
BenchmarkUpdate

BenchmarkUpdate-1   	  296853	      3937 ns/op
BenchmarkUpdate-2   	  310422	      3490 ns/op
BenchmarkUpdate-3   	  308560	      3546 ns/op
BenchmarkUpdate-4   	  311792	      3612 ns/op
BenchmarkUpdate-5   	  312789	      3666 ns/op
BenchmarkUpdate-6   	  286815	      3789 ns/op
BenchmarkUpdate-7   	  293988	      4159 ns/op
BenchmarkUpdate-8   	  311395	      3671 ns/op
BenchmarkUpdate-9   	  313987	      3518 ns/op
BenchmarkUpdate-10        320106	      3681 ns/op

without validation:

goos: darwin
goarch: amd64
pkg: github.com/desmos-labs/desmos/x/profiles/types
BenchmarkUpdate

BenchmarkUpdate-1   	 3203461	       380 ns/op
BenchmarkUpdate-2   	 3068451	       378 ns/op
BenchmarkUpdate-3   	 3199720	       367 ns/op
BenchmarkUpdate-4   	 3229094	       381 ns/op
BenchmarkUpdate-5   	 3226158	       372 ns/op
BenchmarkUpdate-6   	 3105884	       386 ns/op
BenchmarkUpdate-7   	 3091993	       370 ns/op
BenchmarkUpdate-8   	 3018584	       386 ns/op
BenchmarkUpdate-9   	 3113236	       374 ns/op
BenchmarkUpdate-10       3172558	       376 ns/op

Thoughts?

@RiccardoM
Copy link
Contributor

@bragaz Can you please push the bechmark test code? It's hard to understand what those results mean without having a look at the code

@leobragaz
Copy link
Contributor Author

@RiccardoM yeah sure, I forgot to push it

@leobragaz leobragaz requested a review from RiccardoM July 27, 2021 09:31
@RiccardoM
Copy link
Contributor

@bragaz After watching at the benchmarks and the code, I think it's safe to delete he Validate from within the Update method. Could you please update the Update documentation telling that it does not validate the updated profile, and this should be done before storing the profile itself? Just as a precaution for external developers

@leobragaz
Copy link
Contributor Author

@RiccardoM done

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
@RiccardoM RiccardoM merged commit be23505 into master Aug 6, 2021
@RiccardoM RiccardoM deleted the leonardo/profile-double-validation branch August 6, 2021 08:39
RiccardoM pushed a commit that referenced this pull request Aug 6, 2021
See PR #557

(cherry picked from commit be23505)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
RiccardoM pushed a commit that referenced this pull request Aug 15, 2021
See PR #557

(cherry picked from commit be23505)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
RiccardoM added a commit that referenced this pull request Sep 2, 2021
* Version 0.17.3

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Update CHANGELOG.md

* Added the on-chain upgrade handler

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Updated Cosmos SDK to fix --dry-run

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Version 0.17.6
- Updated Cosmos to v0.42.8
- Added the upgrade handler for the upcoming on-chain upgrade

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Updated Cosmos SDK to fix the capability issue

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Updated CHANGELOG

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Removed useless replace from within go.mod

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Release workflow update
Fixed tags not being fetched correctly

* Improve pagination

See PR #544

(cherry picked from commit b066424)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Require chain name to be lowercase

See PR #153

(cherry picked from commit 66e0c98)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Improved profile validation performance

See PR #557

(cherry picked from commit be23505)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Added command to generate chain link JSON

See PR #583

(cherry picked from commit cddb501)

* Changed Ledger app name to Desmos

See PR #590

(cherry picked from commit 91b21f7)

* Updated CHANGELOG

* Removed app migration support

* Removed unused Proto file

* Version 1.0.1

* fix: flag indicator in profile save example

* add: flag indicator in example

* Added changeset entry

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
(cherry picked from commit b40144b)

* Version 1.0.2

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

Co-authored-by: Paul <p22626262@gmail.com>
Co-authored-by: Leonardo Bragagnolo <leo.braga95@gmail.com>
Co-authored-by: Wingman L <42913823+ryuash@users.noreply.github.com>
RiccardoM added a commit that referenced this pull request Sep 7, 2021
* Version 0.17.3

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Update CHANGELOG.md

* Added the on-chain upgrade handler

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Updated Cosmos SDK to fix --dry-run

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Version 0.17.6
- Updated Cosmos to v0.42.8
- Added the upgrade handler for the upcoming on-chain upgrade

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Updated Cosmos SDK to fix the capability issue

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Updated CHANGELOG

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Removed useless replace from within go.mod

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Release workflow update
Fixed tags not being fetched correctly

* Improve pagination

See PR #544

(cherry picked from commit b066424)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Require chain name to be lowercase

See PR #153

(cherry picked from commit 66e0c98)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Improved profile validation performance

See PR #557

(cherry picked from commit be23505)
Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* Added command to generate chain link JSON

See PR #583

(cherry picked from commit cddb501)

* Changed Ledger app name to Desmos

See PR #590

(cherry picked from commit 91b21f7)

* Updated CHANGELOG

* Removed app migration support

* Removed unused Proto file

* Version 1.0.1

* fix: flag indicator in profile save example

* add: flag indicator in example

* Added changeset entry

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

Co-authored-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>
(cherry picked from commit b40144b)

* Version 1.0.2

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

* fix: added missing \ in profile save command example (#602)

(cherry picked from commit 3a73c95)

* Version v1.0.3

Signed-off-by: Riccardo Montagnin <riccardo.montagnin@gmail.com>

Co-authored-by: Paul <p22626262@gmail.com>
Co-authored-by: Leonardo Bragagnolo <leo.braga95@gmail.com>
Co-authored-by: Wingman L <42913823+ryuash@users.noreply.github.com>
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.

Double validation when saving profile
3 participants