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: Add admin governance plugin client #124

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

josemarinas
Copy link
Contributor

@josemarinas josemarinas commented Dec 16, 2022

Description

Create a client for the admin governance plugin

Task: APP-1315

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran all tests with success and extended them if necessary.
  • I have updated the CHANGELOG.md file in the root folder of the package after the [UPCOMING] title and before the latest version.
  • I have tested my code on the test network.

const context = new Context(contextParams);
const contextPlugin: ContextPlugin = ContextPlugin.fromContext(context);

const client = new ClientAdmin(contextPlugin);
Copy link
Contributor

Choose a reason for hiding this comment

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

ClientAdmin or AdminClient? Probably the current is consistent with the rest but I see the upcoming MultisigClient, OvoteClient, TokenVotingClient... making more sense :-/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, we can change it here and make a task to change it everywhere

modules/client/examples.md Outdated Show resolved Hide resolved
modules/client/examples.md Outdated Show resolved Hide resolved
Comment on lines 2601 to 3098
const queryParams: IProposalQueryParams = {
skip: 0, // optional
limit: 10, // optional,
Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't we say that subgraph doesn't allow pagination?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well this is not a pagination, we cannot specify a page because we can't have the total amount of entries of a particular object, for example proposals. We can add a variable to count the number of proposals, the problem that we cannot have the total amount of proposals in a specific state, because the states mainly depend on the current time so from subgraph we cannot detect when a proposal passes from a pending state to a active state or when it passes from an active state to succeded or defeated, because this changes depend on the startDate and endDate of the proposal, and there is no way of tracking that in subgraph.
This was what the UI requested and what the task that we closed was about

/*
[
{
id: "0x12345...",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is inconsistent with the proposal ID's used elsewhere, now bigints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, but the subgraph is using pluginAdddress_proposalId as id because it has to be unique, the bigint proposal ID is in the field proposalId

};

type ProposalBase = {
id: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

id? or proposal id below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

id = pluginAddress_proposalId, has to be unique on subgraph

modules/client/src/admin/interfaces.ts Outdated Show resolved Hide resolved
plugin: {
address: string;
};
proposalId: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

const metadataString = await this.ipfs.fetchString(metadataCid);
const metadata = JSON.parse(metadataString) as ProposalMetadata;
return toAdminProposal(adminProposal, metadata);
// TODO: Parse and validate schema
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a follow up task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we merge this one first I'll add the validation on the validation branch, if we merge the validation branch first i'll add it on this branch

modules/client/test/integration/admin/methods.test.ts Outdated Show resolved Hide resolved
@josemarinas josemarinas changed the base branch from develop to merge_all December 22, 2022 10:52
@josemarinas josemarinas changed the base branch from merge_all to develop January 16, 2023 11:04
@josemarinas josemarinas force-pushed the f/APP-922-admin-governance-plugin branch from e90bfd7 to 0480d67 Compare January 16, 2023 14:51
@josemarinas josemarinas force-pushed the f/APP-922-admin-governance-plugin branch from 0480d67 to f031958 Compare January 23, 2023 11:03
@sonarcloud
Copy link

sonarcloud bot commented Jan 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
1.5% 1.5% Duplication

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