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

MATM changes #450

Merged
merged 34 commits into from
Jan 2, 2019
Merged

MATM changes #450

merged 34 commits into from
Jan 2, 2019

Conversation

comeonbuddy
Copy link
Contributor

@comeonbuddy comeonbuddy commented Dec 3, 2018

Please check if the PR fulfills these requirements

  • The commit message follows our Submission guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

  • Removed all the blocking related functions and events in MATM
  • Given a single address, anyone is able to see a list of all active approvals from/to that address
  • Improved MATM to allow allowances and expiryTimes to be changed for existing approvals.
  • As an issuer, you are able to add/revoke in batches (using a multi approach)
  • As an issuer, you are able to add a description to a manual approval.
  • Anyone is able to see a list of all active approvals

Does this PR introduce a breaking change?

Yes

Any Other information:

@comeonbuddy comeonbuddy changed the base branch from master to dev-2.1.0 December 3, 2018 11:06
@pabloruiz55 pabloruiz55 changed the title Remove blocking matm [WIP] Remove blocking matm Dec 3, 2018
@comeonbuddy comeonbuddy changed the title [WIP] Remove blocking matm Remove blocking matm Dec 4, 2018
@comeonbuddy comeonbuddy changed the title Remove blocking matm [WIP] Remove blocking matm Dec 4, 2018
Copy link
Contributor

@satyamakgec satyamakgec left a comment

Choose a reason for hiding this comment

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

Changes are looking fine to me.
@comeonbuddy you don't need to remove /*solium-disable-next-line security/no-block-members*/ this from the code it only used for linting :)

@VictorVicente VictorVicente changed the title [WIP] Remove blocking matm Remove blocking matm Dec 8, 2018
Copy link
Contributor

@maxsam4 maxsam4 left a comment

Choose a reason for hiding this comment

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

Still need to review logic. Only completed style review currently.

Copy link
Contributor

@maxsam4 maxsam4 left a comment

Choose a reason for hiding this comment

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

The code looks good, Just some minor things that can be optimized.
Test cases need to be fixed/added.

@VictorVicente VictorVicente changed the title Remove blocking matm [WIP] MATM changes Dec 11, 2018
@VictorVicente
Copy link
Contributor

Edited title and description to include all new MATM changes

@VictorVicente VictorVicente changed the title [WIP] MATM changes MATM changes Dec 14, 2018
@coveralls
Copy link

coveralls commented Dec 18, 2018

Coverage Status

Coverage decreased (-0.3%) to 97.746% when pulling ad29c1b on remove-blocking-MATM into f2bd23a on dev-2.1.0.

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.

None yet

8 participants