Skip to content

Small fixes#304

Merged
bbenligiray merged 10 commits intomainfrom
small-fixes
Jul 5, 2021
Merged

Small fixes#304
bbenligiray merged 10 commits intomainfrom
small-fixes

Conversation

@bbenligiray
Copy link
Copy Markdown
Member

A bunch of small fixes:

  • In Api3Voting, EPOCH_LENGTH is already assigned to voteTime in initialize() and since EPOCH_LENGTH and voteTime don't change, we can use voteTime instead of fetching EPOCH_LENGTH from the pool contract. It will just be slightly cheaper.

  • Typecasts at checkpoint arrays were assert()ed but the ones at reward records weren't, so added it for the sake of completeness.

  • totalStake is added to the reward records. This is actually not used in anywhere in the contract, but the problem I wanted to solve is that we don't keep any history of total staked amount on-chain for someone to build an external contract that uses that somehow. It just makes reward payments a bit more expensive so felt like it's worth it.

  • Delegation and delegation update events being the same was confusing, so created another one.

  • Small formatting and comment stuff

@bbenligiray bbenligiray requested a review from acenolaza July 4, 2021 11:17
@bbenligiray bbenligiray self-assigned this Jul 4, 2021
Copy link
Copy Markdown
Contributor

@acenolaza acenolaza 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 added a few minor comments that might be useful.

Comment thread packages/api3-voting/contracts/Api3Voting.sol
Comment thread packages/convenience/contracts/Convenience.sol
Comment thread packages/pool/contracts/RewardUtils.sol
@bbenligiray bbenligiray requested a review from acenolaza July 5, 2021 11:35
@bbenligiray
Copy link
Copy Markdown
Member Author

I added totalStake to the RewardMinted event as you've mentioned. Then I also added totalShares to Staked and ScheduledUnstakeas this was something recommended by Team Omega informally. Then I updated the events to log all major state variables (that change) to make it easier to index these in the future. This doesn't affect how the current dashboard works at all.

Copy link
Copy Markdown
Contributor

@acenolaza acenolaza left a comment

Choose a reason for hiding this comment

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

LGTM again. I was just wondering, should we add "To" to variables like userSharesUpdate (userSharesToUpdate) for consistency with other local variables (i.e. delegatedToUpdate)?

// Should never return false because the API3 token uses the
// OpenZeppelin implementation
assert(api3Token.transferFrom(source, address(this), amount));
emit DepositedByTimelockManager(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we also add source to the event just for completeness and consistency since we are adding all inputs on other events?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

source will always be timelockManager (which is immutable) due to the awkward way TimelockManager.sol was implemented, so not really needed (could be done for consistency but meh)

@@ -118,7 +122,9 @@ abstract contract TimelockUtils is ClaimUtils, ITimelockUtils {
userAddress,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

same here in this event

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

source is also only timelockManager

@bbenligiray
Copy link
Copy Markdown
Member Author

LGTM again. I was just wondering, should we add "To" to variables like userSharesUpdate (userSharesToUpdate) for consistency with other local variables (i.e. delegatedToUpdate)?

delegatedToUpdate is called that because the value it updates is called delegatedTo, so this is consistent (it's just that delegatedTo is weird for a variable name)

@bbenligiray bbenligiray merged commit e05d4a3 into main Jul 5, 2021
@bbenligiray bbenligiray deleted the small-fixes branch July 5, 2021 13:43
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