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

Optional parties and party/signer/role validation overhaul #1453

Merged
merged 138 commits into from
Apr 13, 2023

Conversation

SpicyLemon
Copy link
Contributor

@SpicyLemon SpicyLemon commented Mar 30, 2023

Description

closes: #1381
closes: #1437
closes: #1438

Primarily, this PR:

  • Adds a require_party_rollup boolean field to the scope, and an optional boolean field to the Party message
  • When require_party_rollup = true, new signer validation is used (see spec docs for full details) and optional = true parties are allowed in the scope and it's sessions. Otherwise, optional = true parties are not allowed and existing signer logic is applied.
  • When a spec has repeated roles, an equal number of parties with that role and different addresses are required. This is independent of the require_party_rollup and optional change.
  • The address for parties with the role PROVENANCE MUST be for a smart contract account. And any parties that have a smart contract account address must MUST have the PROVENANCE role. In other words, smart contracts MUST use the PROVENANCE role, and only smart contracts can use the PROVENANCE role.
  • The AddScopeOwner endpoint no longer updates the role of an existing entry with the same address as the one provided. It now simply adds the provided owner.

Secondarily, this PR:

  • Speeds up make proto-format.
  • Fixes some typos in the protos.
  • Moves the cli argument parsing helpers into their own file and adds unit tests on them.
  • Moves some of the keeper.go and signatures.go stuff into signers.go and signers_utils.go and refactors some more of that stuff.
  • Switches the metadata keeper to use interfaces for the auth and authz keepers and creates mocks for those so that unit testing can be a little easier in there.
  • Adds a bunch of unit tests that were missing.
  • Uses auto-generated .String() funcs for the metadata Msgs.
  • Removes the no-longer-needed .MsgTypeURL() funcs from the Msgs.
  • Renames the keeper's Validate...Update functions to Validate<endpoint>, e.g. ValidateWriteScope instead of ValidateScopeUpdate.

Overview of new signer validation

New signer validation is used for scopes, sessions, and records where the scope has require_party_rollup = true. For such scopes, all parties in any of its sessions must also be included in the scope's owners.

  • Any scope owners with optional=false are required to sign for any scope updates including its sessions and records.
  • Any session parties with optional=false are required to sign for updates to that session as well as the records being written under that session.
  • A party can be optional=false in the scope and optional=true in a session, and vice versa.
  • If a party is in multiple sessions in the same scope, it only needs to be in the scope owners once. It can also have different optional values in each session.

Then, coordination is done with the specification, parties, and signers to possibly require additional signatures (beyond the optional=false ones). The roles defined in the specification dictate which parties need to sign. For each required role, there needs to be a unique party that is also a signer.

For example:

  • A record spec requires a SERVICER.
  • A session that will contain that record has three parties:
    • Address :A, Role:OWNER, Optional: false
    • Address :B, Role:SERVICER, Optional: true
    • Address :C, Role:SERVICER, Optional: true

In order to write that record, address A must be a signer because it is optional=false in the session.
Then, EITHER address B or C must be a signer because the specification requires a SERVICER (and address A is not one).

In that above example, if the record spec instead requires SERVICER, SERVICER, then both B and C would need to sign since they are the only SERVICER entries, and two different ones are required.

Then, if, there were a forth party in the session: Address :D, Role:SERVICER, Optional: true (and two SERVICERs are required), any two of B, C, and D must sign.

In all of this, consideration is still made for x/authz authorizations. It's possible for a single signer to fulfill all signing requirements if other parties have granted them authorization. They can even fulfill the signing requirements for multiple of the same role as long as there are enough different parties that have given them authorizations.

For example:

  • A contract spec requires VALIDATOR, VALIDATOR, OWNER.
  • The session has these parties:
    • Address :A, Role:OWNER, Optional: true
    • Address :B, Role:OWNER, Optional: true
    • Address :C, Role:VALIDATOR, Optional: true
    • Address :D, Role:VALIDATOR, Optional: true
    • Address :E, Role:VALIDATOR, Optional: true
  • There are also two addresses not directly involved in the session: Address F, and G.
  • The following authorizations have been granted for writing sessions:
    • Grant 1: Granter: C, Grantee: B
    • Grant 2: Granter: D, Grantee: B
    • Grant 3: Granter: A, Grantee: F
    • Grant 4: Granter: D, Grantee: F
    • Grant 5: Granter: E, Grantee: F
    • Grant 6: Granter: A, Grantee: C
    • Grant 7: Granter: A, Grantee: G
    • Grant 8: Granter: C, Grantee: G

The session can be written with any of these collections of signers (not an exhaustive list):

  • B - They fulfill the OWNER spec requirement themselves. Because of grants 1 and 2, their signature can fulfill two different VALIDATOR roles from C and D.
  • F - An OWNER and two different VALIDATORs have granted them the ability to sign on their behalf (grants 3, 4, and 5). So all three requirements are satisfied.
  • C and D - Both C and D can individually fulfill the two VALIDATOR requirements. Then because of grant 6, C is allowed to sign for A, and A is an OWNER. So C's signature also covers the OWNER requirement.
  • G and E - The signature from E covers one of the two VALIDATOR requirements. Then G, because of grants 7 and 8, can sign for C (another VALIDATOR), and A (an OWNER).

Authorizations are not transitive though. Grant 1 says that B can sign on behalf of C, and grant 6 says that C can sign on behalf of A. That does not mean that B can sign on behalf of A.


One problem all of this solves is when a scope owner wants to give someone (e.g. a SERVICER) permission to write sessions/records in a certain subset of scopes.

The owner would list themselves as optional=false in the scope, probably with the role OWNER (really, anything but SERVICER). Then, they'd list the servicer as optional=true in the applicable scopes. The contract specs and record specs would then need to require a SERVICER. Lastly, the owner would issue an authz grant allowing them to write sessions and records on their behalf.

Now, the servicer can write their sessions records to those scopes using only their signature. However, they wouldn't be allowed to write those sessions and records to other scopes because a SERVICER signature is required, and they are not listed as such in those scopes.


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

…terfaces, and implement them on all the entries and specs and msgs.
…the Msg Type URL, and update the call chains to it as well.
…ing, but output exactly what's needed for a copy/paste into msg.go.
…red anymore and not used. Do type checking on nil values instead of empty structs. Use the sdk for MustAccAddressFromBech32. Add or update a bunch of comments.
…rs is just converting a string to AccAddress which NewEventTxCompleted was then converting right back to a string.
…HasAccess except takes in a string since the comparison is done using strings anyway.
…n address just so they can be converted back for comparison.
…equest.parties field since it's already ignored anyway.
x/metadata/client/cli/helpers.go Show resolved Hide resolved
x/metadata/client/cli/cli_test.go Show resolved Hide resolved
x/metadata/client/cli/cli_test.go Outdated Show resolved Hide resolved
x/metadata/types/scope.go Outdated Show resolved Hide resolved
x/metadata/types/scope.go Show resolved Hide resolved
x/metadata/types/scope_test.go Show resolved Hide resolved
x/metadata/types/scope_test.go Show resolved Hide resolved
x/metadata/types/scope_test.go Show resolved Hide resolved
x/metadata/types/scope_test.go Show resolved Hide resolved
x/metadata/keeper/signers_utils.go Show resolved Hide resolved
x/metadata/keeper/signers_utils.go Outdated Show resolved Hide resolved
x/metadata/keeper/signers_utils.go Show resolved Hide resolved
x/metadata/keeper/signers_utils.go Show resolved Hide resolved
x/metadata/keeper/signers.go Show resolved Hide resolved
x/metadata/keeper/signers.go Show resolved Hide resolved
x/metadata/keeper/scope.go Show resolved Hide resolved
Taztingo
Taztingo previously approved these changes Apr 12, 2023
iramiller
iramiller previously approved these changes Apr 12, 2023
Copy link
Member

@iramiller iramiller left a comment

Choose a reason for hiding this comment

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

High level review of changes complete. Overall this looks good!

…me unit tests on some stuff that didn't have any.
@SpicyLemon SpicyLemon dismissed stale reviews from iramiller and Taztingo via 31b8266 April 12, 2023 17:41
iramiller
iramiller previously approved these changes Apr 12, 2023
Copy link
Member

@iramiller iramiller left a comment

Choose a reason for hiding this comment

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

With the recent updates all tests/checks are passing.

@SpicyLemon SpicyLemon enabled auto-merge (squash) April 12, 2023 19:40
@iramiller
Copy link
Member

Massive code coverage now... nice work.

@SpicyLemon SpicyLemon merged commit 4e4a7c2 into main Apr 13, 2023
@SpicyLemon SpicyLemon deleted the dwedul/1438-optional-parties branch April 13, 2023 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants