Skip to content
This repository has been archived by the owner on Mar 28, 2023. It is now read-only.

DepositLog refactor #537

Merged
merged 8 commits into from
Mar 27, 2020
Merged

DepositLog refactor #537

merged 8 commits into from
Mar 27, 2020

Conversation

NicholasDotSol
Copy link
Contributor

refs: #467

  • Fix TBTCDepositToken shadow by removing TBTCSystem's copy
    • Now TBTCSystem uses the one inherited from DepositLog.
  • Remove return value from logging functions.
    • Return values were not used.
  • Logging functions now revert if approvedToLog fails.
    • Returning true/false allowed for non-halting logging fail. Events would not be emitted but the Deposit contract would continue to function. It seems that this is not a real benefit. If approvedToLog fails, the logging function is not called by the Deposit contract and it makes sense to revert.

NicholasDotSol added 4 commits March 23, 2020 02:19
Remove outdated comment
8a9db61 
No need for the warning, funcion is no longer empty
Remove tbtcDepositToken from tbtcSystem
8ea92a6 
TBTCSystem and DepositLog hold an independent copy of  TBTCDepositToken

TbtcSystem extedns DepositLog and theref ore the variable is shodowed.

Instead of renaming and keeping both, using only the one from
DepositLog is preferable
Remove log return values
6837b39 
The return values are not used. On approvedToLog error
revert isntead of returning false.
Returning false offers the abhlity to not halt the
deposit if approvedToLog returns false.
This does not fel like a real perk. if approvedToLog returns
false the event loger is not the deposit contract, and should revert.
Update tests
7aad202
@tintinweb
Copy link

setTbtcToken should be removed #467

NicholasDotSol added 2 commits March 23, 2020 04:36
Make setTbtcDepositToken internal
f1810be 
setTbtcDepositToken should only be called internally
Merge branch 'log-refactor' of https://github.com/keep-network/tbtc i…
597cdfe 
…nto log-refactor
@NicholasDotSol
Copy link
Contributor Author

setTbtcToken should be removed #467

@tintinweb, Not sure I follow.

@Shadowfiend
Copy link
Contributor

@tintinweb I assume #467 (comment) is the specific bit you're referencing. I think we're not currently aiming for the bigger refactor and just looking to keep this internal---is that right @NicholasDotSol ?

@NicholasDotSol
Copy link
Contributor Author

I think we're not currently aiming for the bigger refactor and just looking to keep this internal---is that right @NicholasDotSol ?

yeah, I don't see a reason for a larger refactor now. We successfully de-duplicate by using DepositLog's copy of tbtcDepositToken in TBTCSystem.
The look and functionality seem clean to me.

Open to discussion of course

@tintinweb
Copy link

Sorry I missed that conversation!

The only point I had here is that setTbtcToken() is not really needed because tbtcDepositToken it can be set directly from TBTCSystem (with the appropriate checks). However, the current implementation of using an internal setter is even cleaner. 🙌 LGTM

Shadowfiend
Shadowfiend approved these changes Mar 27, 2020
View reviewed changes
Copy link
Contributor

@Shadowfiend Shadowfiend left a comment

Choose a reason for hiding this comment

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

This looks good.

@Shadowfiend Shadowfiend merged commit caa238b into master Mar 27, 2020
@Shadowfiend Shadowfiend deleted the log-refactor branch March 27, 2020 20:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Shadowfiend Shadowfiend Shadowfiend approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@NicholasDotSol @tintinweb @Shadowfiend