docs: new, extensions and clarifications #3757
Conversation
MartinquaXD
left a comment
MartinquaXD
left a comment
There was a problem hiding this comment.
Sorry for all the nits. But if we add new documentation we need to make sure that it's carrying its weight. 😅
crates/model/src/order.rs
Outdated
| /// receiver is the same as the owner. | ||
| #[serde(default)] | ||
| pub receiver: Option<H160>, | ||
| /// The *maximum* amount of `sell_token`s that should be sold. |
There was a problem hiding this comment.
Unfortunately it's not as easy as that. What these values represent also depends on partially_fillable and kind.
There was a problem hiding this comment.
Then we need to update the DB readme as well, that was my reference 🤔
| /// The owner's signature of the order. | ||
| #[serde(flatten)] | ||
| pub signature: Signature, | ||
| /// The ID of the quote this order refers to. |
There was a problem hiding this comment.
I feel like some of these comments don't really add much TBH.
There was a problem hiding this comment.
I get you but having some of them added and others not makes things confusing.
Also, I get that they won't help on the 500th read, but for people picking up the code base it can same a couple of clicks. Spread that over the codebase and you get a decent timesave when people are getting into the code for the first time
| /// the Vault, this is done by having a specific ERC20 allowance for the | ||
| /// Vault and relayer approval for the GPv2VaultRelayer. |
There was a problem hiding this comment.
nit: You approve() an allowance so it seems confusing to use both words here.
There was a problem hiding this comment.
This is how it is explained in the official docs though
https://docs.cow.fi/cow-protocol/reference/contracts/core/vault-relayer#balancer-external-balances
Do you have any concrete ideas how to improve this?
jmg-duarte
force-pushed
the
jmgd/docs/varied
branch
from
5c9a908 to
a3fbe6e
Compare
| /// Creates a new app data [`Validator`] with the provided app data | ||
| /// `size_limit` (in bytes). |
There was a problem hiding this comment.
Do we really need this to be documented, while the code is self-descriptive? IMO, this doc is redundant.
There was a problem hiding this comment.
I understand but leaving a single one undocumented feels kind of weird. Plus, when someone is reading the code for the first time and hover over this, they can see what it does without going in.
I think that in the end, it's a matter of taste, but providing docs even for these basic functions can help when people are just perusing and hovering over methods.
jmg-duarte
force-pushed
the
jmgd/docs/varied
branch
from
f02b923 to
981c15b
Compare
squadgazzz
left a comment
squadgazzz
left a comment
There was a problem hiding this comment.
LGTM, just a few nits. Also, please collect at least one more approval from someone before merging.
MartinquaXD
left a comment
MartinquaXD
left a comment
There was a problem hiding this comment.
Approved assuming the links get swapped.
crates/model/src/order.rs
Outdated
| /// | ||
| /// Check the [CoW docs on Balancer Internal Balances](2) for more details. | ||
| /// | ||
| /// [2]: https://docs.cow.fi/cow-protocol/reference/contracts/core/vault-relayer#balancer-external-balances |
There was a problem hiding this comment.
Internal and External links are still swapped.
Co-authored-by: Marcin Szymczak <[email protected]>
Co-authored-by: ilya <[email protected]>
jmg-duarte
force-pushed
the
jmgd/docs/varied
branch
from
3f59c0c to
8a913e1
Compare
Description
Not a great title I know. Again, during my order validation investigation I wrote some docs that aim to help with hovers and so on.
Changes
How to test
NA