Removes custom established VPs #2561

grarco · 2024-02-08T11:58:16Z

Describe your changes

Closes #2554.

Modifies tx_init_account and tx_update account to not write the vp key. Updates of it are not supported for now and when initializing a new established account (also for validators) the protocol automatically assigns it the vp_user.

The host function update_validity_predicate has been removed.

The accounts' vps have been removed from the genesis files.

Indicate on which release or other PRs this topic is based on

v0.31.8

Checklist before merging to `draft`

I have added a changelog
Git history is in acceptable state

cwgoes · 2024-02-08T13:19:32Z

wasm/wasm_source/src/tx_update_account.rs

-            &vp_code_sec.tag,
-        )?;
-    }
+    // TODO: commented out for now cause we don't want to support vp changes


Remove the code, don't comment it out. This is what Git is for.

cwgoes · 2024-02-08T13:19:36Z

wasm/wasm_source/src/vp_user.rs

-                let has_post: bool = ctx.has_key_post(key)?;
                if owner == &addr {
-                    has_post && *valid_sig
+                    // TODO: disabled cause we don't want to support vp changes


grarco · 2024-02-12T18:52:26Z

wasm/wasm_source/src/vp_user.rs

-                    true
-                }
-            }
+            KeyType::Vp(owner) => owner != &addr,


@cwgoes a note on this. This will prevent any vp update even if we wanted to allow it later. To do so we'd need to hard fork and modify this line so that vp updates are enabled. We's also need to modify all the accounts ? storage key to point to a different vp hash. At the same time we can't allow vp changes in here (even though we removed them from the tx) cause it will be possible to to change VP via a sequence of governance proposals

Fraccaman · 2024-02-12T19:03:21Z

@cwgoes do we really want to remove the possibility to update vps via governance? can't we just remove the possibility for usersd to update their acccou t vp and have just the default one? vps are whitelisted anyway and if we just remove the possibility from CLI and wasm transaction to update the validity predicate we can retain the possibility to create multiple of them via governance.

cwgoes · 2024-02-13T07:12:59Z

@cwgoes do we really want to remove the possibility to update vps via governance? can't we just remove the possibility for usersd to update their acccou t vp and have just the default one? vps are whitelisted anyway and if we just remove the possibility from CLI and wasm transaction to update the validity predicate we can retain the possibility to create multiple of them via governance.

Yeah - I think we can retain the possibility to update VPs via governance, I just want to remove the user-submitted txs.

codecov-commenter · 2024-02-13T17:16:32Z

Codecov Report

Attention: Patch coverage is 69.23077% with 8 lines in your changes missing coverage. Please review.

Project coverage is 54.00%. Comparing base (2535c9c) to head (3c54841).
Report is 3211 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/sdk/src/signing.rs	0.00%	4 Missing ⚠️
crates/apps/src/lib/cli.rs	0.00%	2 Missing ⚠️
crates/sdk/src/tx.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2561      +/-   ##
==========================================
+ Coverage   53.95%   54.00%   +0.05%     
==========================================
  Files         308      308              
  Lines      100018    99759     -259     
==========================================
- Hits        53967    53879      -88     
+ Misses      46051    45880     -171

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…client args and types.

grarco · 2024-03-05T17:52:46Z

After #2770 we probably need to rethink this a bit since we might need to let user update their vps

grarco · 2024-04-23T10:58:44Z

About my previous comment: given the way we chose to remove vps from the allowlist, I believe we need to revert the tx_update_account tx to allow the update of the vp in case it was removed from the allowlist. We can still prevent custom vps in genesis instead

brentstone · 2024-04-27T00:58:39Z

is this something we still want to include in the near future?

brentstone · 2024-11-29T19:27:44Z

Is this PR still relevant? @grarco

grarco · 2024-11-30T12:08:18Z

Is this PR still relevant? @grarco

I don't think so. I guess we can close this and also the related issue

cwgoes requested changes Feb 8, 2024

View reviewed changes

grarco force-pushed the grarco/remove-update-vp branch from 62330ac to 1fe7b54 Compare February 8, 2024 16:41

grarco added a commit that referenced this pull request Feb 9, 2024

Changelog #2561

af471eb

grarco changed the title ~~Removes vp updates~~ Removes custom established VPs Feb 9, 2024

grarco added a commit that referenced this pull request Feb 10, 2024

Changelog #2561

4b41ada

grarco force-pushed the grarco/remove-update-vp branch from af471eb to 4b41ada Compare February 10, 2024 00:03

grarco marked this pull request as ready for review February 10, 2024 00:40

grarco requested review from cwgoes and tzemanovic February 10, 2024 00:40

tzemanovic previously approved these changes Feb 12, 2024

View reviewed changes

grarco added the ready for draft label Feb 12, 2024

grarco mentioned this pull request Feb 12, 2024

Draft [breaking] #2587

Closed

grarco commented Feb 12, 2024

View reviewed changes

grarco added a commit that referenced this pull request Feb 13, 2024

Changelog #2561

ab1e89c

grarco dismissed tzemanovic’s stale review via a5d1e91 February 13, 2024 12:04

grarco force-pushed the grarco/remove-update-vp branch from 4b41ada to a5d1e91 Compare February 13, 2024 12:04

grarco added a commit that referenced this pull request Feb 13, 2024

Changelog #2561

2108c6d

grarco force-pushed the grarco/remove-update-vp branch 2 times, most recently from e18ae94 to 6a4d5d1 Compare February 13, 2024 16:51

grarco removed the ready for draft label Feb 13, 2024

grarco requested a review from tzemanovic February 13, 2024 16:52

grarco added ready for draft breaking-change labels Feb 14, 2024

grarco added 4 commits February 23, 2024 10:40

Removes vp updates

e0a4a3e

Removes vp from UpdateAccount

51074a6

Removes host fn update_validity_predicate and updates wasm tests

ff562c0

Removes VP from InitAccount and init validator in genesis. Reworks …

e4a7ddf

…client args and types.

grarco added 9 commits February 23, 2024 10:50

Rebuilds localnet genesis files

ab7ab42

Adjusts vp user name in tx_init_account

681ba17

Removes init-chain vp path arg from tests

4e3804f

Removes outdated test

1d15777

Fixes tests

18b64cb

Rebuilds masp proofs

62aaf6c

Changelog #2561

3a878f2

Allow vp updates in vp_user

723e09a

Adds vp update check in vp_user

497cbe1

grarco force-pushed the grarco/remove-update-vp branch from e1d53c9 to 497cbe1 Compare February 23, 2024 10:39

tzemanovic added the breaking:genesis Genesis configuration breaking change label Feb 23, 2024

tzemanovic previously approved these changes Feb 23, 2024

View reviewed changes

Merge branch 'base' into grarco/remove-update-vp-rebase

41fb713

grarco dismissed tzemanovic’s stale review via 41fb713 February 28, 2024 16:18

grarco added 2 commits February 28, 2024 17:36

Fixes merge errors

3430906

Merge branch 'base' into grarco/remove-update-vp-rebase

3c54841

Fraccaman mentioned this pull request Apr 15, 2024

Draft #3079

Closed

grarco removed the ready for draft label Apr 23, 2024

brentstone mentioned this pull request Apr 27, 2024

Draft - released 0.35.0 #3147

Closed

tzemanovic mentioned this pull request May 9, 2024

Draft - released as 0.36.0 #3213

Closed

tzemanovic mentioned this pull request May 21, 2024

Draft - released as 0.37.0 #3283

Closed

This was referenced May 24, 2024

Draft - released 0.38.0 #3304

Closed

Draft #3337

Closed

brentstone mentioned this pull request Jun 6, 2024

Draft #3377

Closed

grarco closed this Dec 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes custom established VPs #2561

Removes custom established VPs #2561

Uh oh!

grarco commented Feb 8, 2024 •

edited

Loading

Uh oh!

cwgoes Feb 8, 2024

Uh oh!

cwgoes Feb 8, 2024

Uh oh!

grarco Feb 12, 2024

Uh oh!

Fraccaman commented Feb 12, 2024

Uh oh!

cwgoes commented Feb 13, 2024

Uh oh!

codecov-commenter commented Feb 13, 2024 •

edited by codecov bot

Loading

Uh oh!

grarco commented Mar 5, 2024

Uh oh!

grarco commented Apr 23, 2024

Uh oh!

brentstone commented Apr 27, 2024

Uh oh!

brentstone commented Nov 29, 2024

Uh oh!

grarco commented Nov 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Removes custom established VPs #2561

Removes custom established VPs #2561

Uh oh!

Conversation

grarco commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe your changes

Indicate on which release or other PRs this topic is based on

Checklist before merging to draft

Uh oh!

cwgoes Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

cwgoes Feb 8, 2024

Choose a reason for hiding this comment

Uh oh!

grarco Feb 12, 2024

Choose a reason for hiding this comment

Uh oh!

Fraccaman commented Feb 12, 2024

Uh oh!

cwgoes commented Feb 13, 2024

Uh oh!

codecov-commenter commented Feb 13, 2024 • edited by codecov bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

grarco commented Mar 5, 2024

Uh oh!

grarco commented Apr 23, 2024

Uh oh!

brentstone commented Apr 27, 2024

Uh oh!

brentstone commented Nov 29, 2024

Uh oh!

grarco commented Nov 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

grarco commented Feb 8, 2024 •

edited

Loading

Checklist before merging to `draft`

codecov-commenter commented Feb 13, 2024 •

edited by codecov bot

Loading