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

P8EMemorializeContract: Limit scope field changes. #336

Merged
merged 4 commits into from
May 26, 2021

Conversation

dwedul-figure
Copy link
Contributor

@dwedul-figure dwedul-figure commented May 26, 2021

…preserve its owners and value owners. Also only add to the data access list (instead of fully overwritting it).

Description

This PR makes the following changes to the processing done during P8EMemorializeContract:

  1. If there's already a Scope with the given scope id, don't update the Owners or ValueOwnerAddress fields.
  2. If there's already a scope with the given scope id, copy it's existing DataAccess values and add any new ones that might be missing (but don't remove any existing values).

Details:

Currently, during a P8EMemorializeContract call, the Scope.Owners, Scope.ValueOwnerAddress, and Scope.DataAccess values are populated using only info in the MsgP8EMemorializeContractRequest.

This has caused a problem in the following scenario:

  1. MsgP8EMemorializeContractRequest is called with a contract just for an ORIGINATOR.
  2. MsgP8EMemorializeContractRequest is called with a contract that has an ORIGINATOR and a SERVICING, that is to be added to the previously created scope.
  3. MsgP8EMemorializeContractRequest is called again for the contract with just for an ORIGINATOR.

Problem: Now, both the ORIGINATOR and SERVICING parties are owners on the scope, so both need to sign for step number 3 (which would then actually remove the SERVICING owner).

After some discussion, we decided that we don't want MsgP8EMemorializeContractRequest to update the Scope.Owners, or Scope.ValueOwnerAddress fields. We also want the Scope.DataAccess values to only be additive. E.g., during step number 2 (above), the SERVICING party would be added to the DataAccess list, but during step 3, they'd still be there (they wouldn't be removed).


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

…preserve its owners and value owners. Also only add to the data access list (instead of fully overwritting it).
@dwedul-figure dwedul-figure enabled auto-merge (squash) May 26, 2021 20:15
@codecov
Copy link

codecov bot commented May 26, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@70b5f3b). Click here to learn what that means.
The diff coverage is 61.53%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #336   +/-   ##
=======================================
  Coverage        ?   50.75%           
=======================================
  Files           ?      127           
  Lines           ?    11414           
  Branches        ?        0           
=======================================
  Hits            ?     5793           
  Misses          ?     5056           
  Partials        ?      565           
Impacted Files Coverage Δ
x/metadata/keeper/msg_server.go 0.30% <0.00%> (ø)
x/metadata/keeper/keeper.go 80.51% <100.00%> (ø)

x/metadata/keeper/keeper.go Outdated Show resolved Hide resolved
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.

Overall looks good... holding approve for a bit in light of automerge so the comment can be seen.

Copy link
Contributor

@channa-figure channa-figure left a comment

Choose a reason for hiding this comment

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

Nice work. LGTM

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.

Awesome.

@dwedul-figure dwedul-figure merged commit 74607b1 into main May 26, 2021
@dwedul-figure dwedul-figure deleted the dwedul/fix-p8e-scope-owner-update branch May 26, 2021 21:35
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.

3 participants