DRAFT - FM_PC_ExternalPriceSetter, Oracle, blacklist contracts and interfaces implemented

[vendrell46]

The scope of this PR is to review the proposed implementation, considering that we DO NOT consider it complete, but rather a work in progress.

We want to ensure we are going in the right direction by getting feedback from the team.

[Zitzak]

Thank you for the work. Overall looks great and right on track. I've left a comments which are mostly about style, formatting etc and some related to how the implementation might be done differently.

Running a bit out of time on my end to do a second pass but I think this will give you guys enough to keep developing.

[Zitzak]

Thank you for the work guys!

I have reviewed everything again until the sell function in the FM. I will need to come back to finish the review tomorrow

[Zitzak]

Another review. Think we are almost there except for some work on the sell function. We should plan a quick sync around this maybe tomorrow

[FHieser]

All my comments up until this point have been addressed

[Zitzak]

I like the solution you guys found to add the emit to the _sellOrder function. However I think we need to adapt it just a bit, as well as the fee collection.
Also I believe the question around the decimals for price setting is still open.

Other than that it looks great, and only added a few small comments

[Zitzak]

I leave the last few comments and will be requesting a review of @marvinkruse today

[Zitzak]

@vendrell46 additionally I noticed that there are several natspec comments in the FM which could be moved to the interface as well

[marvinkruse]

Hey, leaving some comments as I pause my review for today before I continue tomorrow morning (CET time)! :) Really great work overall, really appreciate this! 👏 Definitely on the right track, just a few small things to iron out, mainly on the Inverter Standard side. It's not easy, even for us, to fully follow it yet, as it's quite a lot of rules and some of them are quite cryptic, so please don't see my comments or the amount of comments as a bad thing, I like your work! 🙏

Some general comments:

• We end each natspec comment with a dot, like
  • @notice Maximum number of addresses that can be blacklisted in a batch.
• Section delimiters should have a whitespace between the // and the --- part
• We prefer to press tab after each NatSpec keyword. I left comments for this here and there, so you probably see what I mean?
• There are cases of @inheritdoc tags with just two // in front instead of three ///, found these in FM_PC_ExternalPrice_Redeeming_v1.sol specifically.
• Some lines are >80 characters (also including whitespaces at the front, etc.), which need a few line breaks here and there, as we do not want to go above that limit of 80.

Thank you in advance 🙏 Let me know if you have any question or need clarifications or feedback on anything.

[marvinkruse]

We don't use any header to signal the "imports" section, so this can just be removed. Otherwise the imports look fine 👍`suggestion

Suggested change

// Imports

[vendrell46]

could we double-check with @Zitzak ? It was suggested by him

[Zitzak]

We can go with what @marvinkruse suggests, as he is up to date with the Inverter standard.

Apologies for letting you add it in @vendrell46 🙇‍♂️

[vendrell46]

removed Imports header

[marvinkruse]

This sentence is a bit confusing, i.e. the blacklist operations (like adding someone to the blacklist) can not be performed by the contract owner in the code, but by someone who has the backlist manager role. The management of the blacklist managers is on the contract owner, though.

This sentence in line 31 doesn't really capture this (i.e. says the opposite) and may need a bit of an update.

[vendrell46]

clarified

[marvinkruse]

I believe there is a space too much, after each @keyword we just press tab once, which on my end adds 2 spaces, while this seems like it has 3?

[vendrell46]

true, eagle eye. Fixed

[marvinkruse]

I believe there is a space too much, after each @keyword we just press tab once, which on my end adds 1 space, while this seems like it has 2?

[scab24]

I understand, so which format would we go with, the one you suggested in the previous comment or the current one?
Should we leave 1 or 2 spaces?

[vendrell46]

Indeed, using the tabs adds just one space. Fixed

[marvinkruse]

What we prefer for mappings is to directly name the involved parameters so it's easy to see what is being mapped, like so for example:

Suggested change

	mapping(address => bool) private _blacklist;
	mapping(address account => bool isBlacklisted) private _blacklist;

[vendrell46]

done

[marvinkruse]

The comment is missing the part that this is just about the fee, not in general (as it's just checking for there to be enough for the fee).

[vendrell46]

done

[marvinkruse]

The line is too long, needs a break at 80 characters max.

[vendrell46]

fixed

[marvinkruse]

It has not been fixed yet I think?

[marvinkruse]

Still not fixed?

[vendrell46]

now

[marvinkruse]

The line is too long, needs a break at 80 characters max.

[vendrell46]

fixed

[marvinkruse]

The line is too long, needs a break at 80 characters max.

[vendrell46]

fixed

[marvinkruse]

The line is too long, needs a break at 80 characters max.

[vendrell46]

fixed

[marvinkruse]

Adding another comment as my last one got lost I believe:

The line is too long, needs a break at 80 characters max.

[vendrell46]

done

[marvinkruse]

Natspec section needs to be copied from the contract now that that one was changed. also no dot at the end of a title 🙏

[vendrell46]

there isn't a OraclePrice_v1 implementation, so what do you suggest, @marvinkruse?

This interface is extended in interface ILM_ManualExternalPriceSetter_v1 is IOraclePrice_v1

[vendrell46]

dot removed

[marvinkruse]

Good job on getting the changes in, looks great 🙏 Just a simple line being too long, otherwise I couldn't spot anything obvious.

[Zitzak]

Longest PR I've ever seen :D Great work @vendrell46 @scab24