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

Dividend name #299

Merged
merged 9 commits into from
Oct 2, 2018
Merged

Dividend name #299

merged 9 commits into from
Oct 2, 2018

Conversation

maxsam4
Copy link
Contributor

@maxsam4 maxsam4 commented Oct 2, 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?

Enhancement

What is the current behavior?

Dividends are unnamed

What is the new behavior?

Dividend have a name

Does this PR introduce a breaking change?

(What changes might users need to make in their application due to this PR?)
Need to pass Dividend name when creating a Dividend.

Any Other information:

  1. Apparently Sublime added whitespace changes to the whole contracts :/. Add ?w=1 to the URL to see the diff with whitespace ignored (213b318?w=1).
  2. I made _createDividendWithCheckpointAndExclusions an internal function to save gas when being called from public functions. public -> public function calls require all the parameters to be copied into memory again while public -> internal will just pass on the reference.
  3. I am unable to run tests on my local machine for now so using travis for testing.

Fixes #295

@maxsam4 maxsam4 changed the title [WIP] Dividend name Dividend name Oct 2, 2018
@adamdossa
Copy link
Contributor

@maxsam4 Looks good to me - only comment is please could the function:
_CallERC20DividendDepositedEvent
be renamed to:
_emitERC20DividendDepositedEvent

maxsam4 and others added 2 commits October 2, 2018 18:21
Renamed `_CallERC20DividendDepositedEvent` to `_emitERC20DividendDepositedEvent`
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.

I am not a big fan of bytes32 but if we can replace the datatype to bytes32 for name then we can indexed it in the events that may useful for the dApp team.

@pabloruiz55 pabloruiz55 self-requested a review October 2, 2018 15:47
@pabloruiz55
Copy link
Contributor

pabloruiz55 commented Oct 2, 2018

looks good! Needs @satyamakgec 's approval

@satyamakgec satyamakgec merged commit deeb1fe into development-1.5.0 Oct 2, 2018
@maxsam4 maxsam4 deleted the dividend-name branch October 3, 2018 03:23
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.

4 participants