-
Notifications
You must be signed in to change notification settings - Fork 956
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
Removes custom established VPs #2561
base: main
Are you sure you want to change the base?
Conversation
&vp_code_sec.tag, | ||
)?; | ||
} | ||
// TODO: commented out for now cause we don't want to support vp changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the code, don't comment it out. This is what Git is for.
wasm/wasm_source/src/vp_user.rs
Outdated
if owner == &addr { | ||
has_post && *valid_sig | ||
// TODO: disabled cause we don't want to support vp changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
62330ac
to
1fe7b54
Compare
af471eb
to
4b41ada
Compare
wasm/wasm_source/src/vp_user.rs
Outdated
true | ||
} | ||
} | ||
KeyType::Vp(owner) => owner != &addr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
@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. |
4b41ada
to
a5d1e91
Compare
e18ae94
to
6a4d5d1
Compare
Codecov ReportAttention: Patch coverage is
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. |
e1d53c9
to
497cbe1
Compare
After #2770 we probably need to rethink this a bit since we might need to let user update their vps |
About my previous comment: given the way we chose to remove vps from the allowlist, I believe we need to revert the |
is this something we still want to include in the near future? |
Describe your changes
Closes #2554.
Modifies
tx_init_account
andtx_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 thevp_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