allora-network / allora-chain

Node software to run the Allora Network
https://www.allora.network/
Apache License 2.0
78 stars 72 forks source link

Reintroduce whitelists on global and topic levels #663

Open kpeluso opened 6 days ago

kpeluso commented 6 days ago

Purpose of Changes and their Description

Whitelists reintroduced so protocol vanguards can better nurture our budding network.

This file is the best entrypoint into understanding everything I did at the most raw level. Every other change is a consequence of the functions I created here:

x/emissions/keeper/whitelist.go

Roles (permissions) of existing whitelist has been expanded and new whitelists and related abstractions have been introduced:

Link(s) to Ticket(s) or Issue(s) resolved by this PR

https://linear.app/alloralabs/issue/PROTO-2678/reintroduce-whitelists

Are these changes tested and documented?

Still Left Todo

Done.

github-actions[bot] commented 6 days ago

The latest Buf updates on your PR. Results from workflow Buf Linter / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed⏩ skippedNov 2, 2024, 7:50 AM
kpeluso commented 3 days ago

Thank you for the review @xmariachi . Some responses to your overall comments:

Migration: This is a new minor version, v0.7.0, shouldn't the changes be in v6? Keeping them in v5 (which has already been migrated to) could be confusing going fw.

My thinking was that these were strictly additions and we only technically have to bother with a migration (more complexity) whenever we touch existing protos and properties.

Just a comment on an alternative way, maybe more canonical/universal, to impl roles/permissions: via defining a unified permissions list per address and (address, topic), defining roles separately, so each entry can have a set of roles. However because it is not expected this set of roles to grow (right?), the current impl is simpler and should also be slightly more performant at the expense of a bit more of storage space.

Exactly. It is simpler, the storage implied by these lists are bounded by protocol vanguard activity, and no, we don't expect this set of roles to grow -- this already covers everything except delegation and not-unique-to-allora actions e.g. sending ALLO.

relyt29 commented 2 days ago

My thinking was that these were strictly additions and we only technically have to bother with a migration (more complexity) whenever we touch existing protos and properties.

I think we should do this as a full on migration. It's adding a non-trivial amount of changes to protos and errors and quite a few other things. I think it's more appropriate to do non-x/upgrade migrations for hotfixes and small changes, this is not, changes the security model quite a bit

xmariachi commented 2 days ago

My thinking was that these were strictly additions and we only technically have to bother with a migration (more complexity) whenever we touch existing protos and properties.

I think we should do this as a full on migration. It's adding a non-trivial amount of changes to protos and errors and quite a few other things. I think it's more appropriate to do non-x/upgrade migrations for hotfixes and small changes, this is not, changes the security model quite a bit

I agree with @relyt29 . It also adds clarity on knowing what is being added on each migration / compat-breaking version Also, modifying this migration would make a migration from 0.5.0 to 0.6.0 inconsistent (only 0.5.0 to 0.7.0 would be consistent) so there would be no way to get 0.6.0 state by migrations. imho no need to lose this traceability.

kpeluso commented 1 day ago

@xmariachi @relyt29 I don't agree. This is testnet and we should expect to wipe previous migrations for mainnet, as well as module versions.

Our modules are not in sync already as x/emissions is v5, x/mint is less, and the chain is v0.6.0, and that's fine.

Traceability isn't a concern -- git history isn't going anywhere, and that's used for traceability more than testnet migrations that we'll probably wipe anyway. I think, however, you mean traceability in a way too closely resembling how databases migrate -- it's a clear analogy but at the same time, we'll never downgrade, so we don't need "reversabilitiy."

Strict additions to protos that clients don't touch need not be viewed as breaking, as it's breaking nothing about existing functionality for clients the chain cares to support atm. In fact, if we bump x/emissions, that would break all API clients regardless if they care about strictly typed global parameters or whitelists as they'd need to bump v5->v6 in their urls (which is yet another reason to use versioned protos instead of versioned modules imo).