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

feat: remove todo #38

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

feat: remove todo #38

wants to merge 5 commits into from

Conversation

ashitakah
Copy link
Contributor

🤖 Linear

Closes GRT-182

  • Set for params.
  • Arbitrator in HorizonAccountingExtension
  • Remove TODOs and improve some style

Copy link

linear bot commented Sep 30, 2024

GRT-182 Review all TODOs

Comment on lines 55 to 52
) external onlyArbitratorModule returns (bytes memory /* _data */ ) {
) external onlyArbitratorModule returns (bytes memory _data) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I support keeping solhint's no-unused-vars and commenting unused parameter names. 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

src/contracts/HorizonAccountingExtension.sol Outdated Show resolved Hide resolved
test/unit/HorizonAccountingExtension.t.sol Outdated Show resolved Hide resolved
test/unit/HorizonAccountingExtension.t.sol Outdated Show resolved Hide resolved
Comment on lines 116 to 117
*/
event MaxUsersToCheckSetted(uint256 _maxUsersToCheck);
Copy link
Member

Choose a reason for hiding this comment

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

In this context, the past participle of set is set, not setted.

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, usually we use setted to nuance the meaning, bit I can change it

Comment on lines -232 to +235
_balance += _slash(_disputeId, 1, MAX_USERS_TO_CHECK, _result, _status);
_balance += _slash(_disputeId, 1, maxUsersToCheck, _result, _status);
Copy link
Member

Choose a reason for hiding this comment

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

I'd pass a constant (MAX_USERS_TO_SLASH) as argument instead of the magic number.

Copy link
Member

Choose a reason for hiding this comment

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

I'm good to have it as a constant too (1)

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.

3 participants