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

Feature/plmc 473 change edit metadata extrinsic to edit the whole metadata #207

Conversation

JuaniRios
Copy link
Contributor

What?

  • change the edit_metadata extrinsic to be able to change all of the fields of ProjectMetadata

Why?

  • Issuers are likely to make mistakes when first creating the project

How?

  • if the project hasn't started the evaluation, the issuer can call edit_metadata with a copy of ProjectMetadata that contains optional fields. Each some field constitutes a replacement on the stored metadata. This new struct is called OptionalProjectMetadata.

Testing?

edit_metadata()

JuaniRios and others added 30 commits March 18, 2024 11:46
# Conflicts:
#	integration-tests/src/tests/vest.rs
#	pallets/funding/src/instantiator.rs
#	pallets/funding/src/lib.rs
#	pallets/funding/src/types.rs
#	runtimes/politest/src/lib.rs
Co-authored-by: Leonardo Razovic <4128940+lrazovic@users.noreply.github.com>
…-limit-multipliers-based-on-credential-type

# Conflicts:
#	pallets/funding/src/functions.rs
#	pallets/funding/src/lib.rs
#	pallets/funding/src/tests.rs
#	pallets/funding/src/types.rs
…credential-type

# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	integration-tests/Cargo.toml
#	integration-tests/src/constants.rs
#	integration-tests/src/lib.rs
#	integration-tests/src/tests/basic_comms.rs
#	integration-tests/src/tests/credentials.rs
#	integration-tests/src/tests/ct_migration.rs
#	integration-tests/src/tests/defaults.rs
#	integration-tests/src/tests/e2e.rs
#	integration-tests/src/tests/governance.rs
#	integration-tests/src/tests/oracle.rs
#	integration-tests/src/tests/reserve_backed_transfers.rs
#	integration-tests/src/tests/vest.rs
#	justfile
#	nodes/parachain/Cargo.toml
#	nodes/parachain/src/chain_spec.rs
#	nodes/parachain/src/chain_spec/polimec.rs
#	nodes/parachain/src/chain_spec/politest.rs
#	nodes/parachain/src/command.rs
#	nodes/parachain/src/rpc.rs
#	nodes/parachain/src/service.rs
#	pallets/funding/src/tests.rs
#	runtimes/polimec/Cargo.toml
#	runtimes/politest/Cargo.toml
…type' into feature/plmc-473-change-edit_metadata-extrinsic-to-edit-the-whole-metadata
@JuaniRios JuaniRios marked this pull request as ready for review March 25, 2024 13:04
@JuaniRios JuaniRios changed the base branch from feature/plmc-468-limit-multipliers-based-on-credential-type to main March 25, 2024 14:41
…sic-to-edit-the-whole-metadata

# Conflicts:
#	pallets/funding/src/functions.rs
#	pallets/funding/src/mock.rs
#	pallets/funding/src/tests.rs
#	pallets/funding/src/types.rs
Copy link
Collaborator

@vstam1 vstam1 left a comment

Choose a reason for hiding this comment

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

Why have this completely new OptionalMetadata? Why not do the "initial metadata" struct over again for editing and just replace metadata instead of this difficult "updating". I think you are also missing the updates to project_details and bucket based on metadata, so create and edit could be quite similar. So:

  • Validating metadata and updating bucket based on metadata and inserting metadata in storage could be single function that is used by both create and edit.
  • Create details in create, update details in edit.
  • Create does all the transfers and deposits and updating project_id counter etc, while create only takes in Project_id

@JuaniRios
Copy link
Contributor Author

Why have this completely new OptionalMetadata? Why not do the "initial metadata" struct over again for editing and just replace metadata instead of this difficult "updating". I think you are also missing the updates to project_details and bucket based on metadata, so create and edit could be quite similar. So:

  • Validating metadata and updating bucket based on metadata and inserting metadata in storage could be single function that is used by both create and edit.
  • Create details in create, update details in edit.
  • Create does all the transfers and deposits and updating project_id counter etc, while create only takes in Project_id
  • I was thinking it from the PolkadotJS perspective where it would have been annoying to fill a whole project metadata with the same fields when you wanted to change just one, but I see your point. Probably the UI will do the heavy lifting for the user, so better just repeat the struct.

  • Good point, I'll update projectDetails.

  • I didn't get the last point

@vstam1
Copy link
Collaborator

vstam1 commented Mar 25, 2024

For the last point, a lot of the logic between do_create and edit_metadata will be similar if you just use ProjectMetadataOf as input. So you can probably move a lot of the logic to a seperate function call that function for both create and edit

@JuaniRios JuaniRios requested a review from vstam1 March 25, 2024 16:47
Copy link
Collaborator

@vstam1 vstam1 left a comment

Choose a reason for hiding this comment

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

Lgtm, just single comment

pallets/funding/src/benchmarking.rs Show resolved Hide resolved
@JuaniRios JuaniRios merged commit 35df638 into main Mar 25, 2024
@JuaniRios JuaniRios deleted the feature/plmc-473-change-edit_metadata-extrinsic-to-edit-the-whole-metadata branch March 26, 2024 08:17
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.

2 participants