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

[FROZEN] Installation and documentation update #99

Closed
wants to merge 12 commits into from

Conversation

RonTuretzky
Copy link
Collaborator

README.md Outdated
$ forge test --fork-url "https://rpc.gnosis.gateway.fm" -vvvv --fuzz-runs 1
```

For a full run of tests run the following command, note that it may take a significant amount of time
Copy link
Contributor

Choose a reason for hiding this comment

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

Run on sentence. Split into two. Missing period. Maybe give a rough estimate of how long is a "significant amount of time".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed with a036006

The proxy admin address is configured to be the Breadchain multisig at address `0x918dEf5d593F46735f74F9E2B280Fe51AF3A99ad` and the Yield Distributor proxy address is `0xeE95A62b749d8a2520E0128D9b3aCa241269024b`


Copy link
Contributor

Choose a reason for hiding this comment

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

Remove one line break.

The proxy admin address is configured to be the Breadchain multisig at address `0x918dEf5d593F46735f74F9E2B280Fe51AF3A99ad` and the Yield Distributor proxy address is `0xeE95A62b749d8a2520E0128D9b3aCa241269024b`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove this line. First part describes contract state which may change. Second part is deployment info. If anything this should be split into two sections. The multisig address can be added to general info at the top of the README. Then add another section, maybe in table form, that lists production deployments of the contracts contained in the repo, like:

Contract Address
Bread 0xa555d5344f6FB6c65da19e403Cb4c1eC4a1a5Ee3
YieldDistributor 0xeE95A62b749d8a2520E0128D9b3aCa241269024b

README.md Outdated
@@ -16,6 +16,7 @@ Contributions to this repo are expected to adhere to the [Biconomy Solidity Styl
### Build

```shell
$ git clone [email protected]:BreadchainCoop/breadchain.git --recursive
Copy link
Contributor

@bagelface bagelface Sep 28, 2024

Choose a reason for hiding this comment

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

Probably an unnecessary line because we can assume a user knows how to use git. Otherwise, I'd move this to a separate section above this one called "Clone".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While onboarding multiple people, the submodules have proven to be a point of friction
Moved to another section with 3c620e5



The development of this project was enabled by a [grant](https://gov.powerpool.finance/t/approved-grant-for-breadchain-cooperative-integrations/2007) from [Powerpool](https://powerpool.finance/).
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is necessary either. It's sort of an ambiguous statement and could be misinterpreted. Does it mean the entire Breadchain project was funded by this grant?

@@ -236,7 +236,7 @@ contract YieldDistributor is IYieldDistributor, OwnableUpgradeable {
/**
* @notice Internal function for casting votes for a specified user
* @param _account Address of user to cast votes for
* @param _points Basis points for calculating the amount of votes cast
* @param _points Points for calculating the amount of votes cast
Copy link
Contributor

Choose a reason for hiding this comment

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

would this be better described as "distribution"?

@@ -195,7 +195,7 @@ contract YieldDistributor is IYieldDistributor, OwnableUpgradeable {
}

/**
* @notice Distribute $BREAD yield to projects based on cast votes
* @notice Distribute $BREAD yield to projects based on cast votes, may leave some dust
Copy link
Contributor

Choose a reason for hiding this comment

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

Add as a new @dev tag if anything.

@@ -0,0 +1,89 @@
## What is BREAD?
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems weird to have docs for this contract but to not include the contract. Will bring it up at next core meeting.

@@ -0,0 +1,89 @@
## What is BREAD?

BREAD is the community currency for the Breadchain ecosystem which exists on Gnosis Chain. All BREAD is created through the [Bread Crowdstaking Application](https://www.notion.so/Crowdstaking-Application-9f233bc2fb1e419ebeb58a2809b21658?pvs=21) which anyone with xDAI on Gnosis Chain is able to use to have some BREAD for themselves.
Copy link
Contributor

@bagelface bagelface Sep 28, 2024

Choose a reason for hiding this comment

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

So whenever I see a fully capitalized name like this, I assume it is referring to the symbol/ticker. I think we should either prepend the $ to any occurrence for consistency personally. Recommended change:

Bread ($BREAD) is the community currency for the Breadchain ecosystem, deployed to the Gnosis Chain network as an ERC20 token. All $BREAD is created through the Bread Crowdstaking Application. Anyone with xDAI on Gnosis Chain can bake $BREAD for themselves, which is referred to as "baking".


BREAD is the community currency for the Breadchain ecosystem which exists on Gnosis Chain. All BREAD is created through the [Bread Crowdstaking Application](https://www.notion.so/Crowdstaking-Application-9f233bc2fb1e419ebeb58a2809b21658?pvs=21) which anyone with xDAI on Gnosis Chain is able to use to have some BREAD for themselves.

```mermaid
Copy link
Contributor

@bagelface bagelface Sep 28, 2024

Choose a reason for hiding this comment

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

I know we love diagrams, but is this really providing value? I have two problems with it: (1) It's essentially recreating the structure of the contract is a second place, which means it now needs to be maintained in two places. If we change the contract and forget to change this (which in my experience happens A LOT which stuff like this) it actually detracts from peoples understanding of the system. (2) It's not obvious what all the random symbols in this diagram mean, so it makes for a diagram that is sort of impossible to fully understand. Especially without a "key" for the symbols.

The structure of the contract is self documented by the contract code itself.

On line 3 the native currency (xDai) of Gnosis Chain gets converted to an ERC20 representation ([wxDai](https://gnosisscan.io/address/0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d)). This is done because in order to lock the xDai.

[sDai](https://gnosisscan.io/address/0xaf204776c7245bF4147c2612BF6e5972Ee483701) is an automatic mechanism that allows a user to recieve yield from the [Dai Savings Rate](https://blog.makerdao.com/why-the-dai-savings-rate-is-a-game-changer-for-the-defi-ecosystem-and-beyond/) by depositing and locking wxDai. This is how BREAD generates yield.
On line 4 , the Bread contract allows the [sDai contract](https://gnosisscan.io/address/0xaf204776c7245bF4147c2612BF6e5972Ee483701) to take xDai from itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we suddenly adding spaces before the commas in this whole section?

```
On line 2 , the `bal` variable represents the sDai balance of the Bread contract. This represents how much xDai is locked and earning yield.

In line 3 use the `bal` variable to determine how much xDai the Bread contract is eligible for by burning sDai, and we store that in the `assets` variable. This represents the original xDai locked and any rewards earned.
Copy link
Contributor

Choose a reason for hiding this comment

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

No there are no commas at all.

Copy link
Contributor

@bagelface bagelface left a comment

Choose a reason for hiding this comment

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

This is not ready to merge IMO. I got a lot of issues with the style and content. Maybe I'll take a shot and cleaning it up.

Copy link
Collaborator

@secbajor secbajor left a comment

Choose a reason for hiding this comment

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

@RonTuretzky I left some suggestions for how clarity or context might be improved to help this all make more sense to someone with limited background knowledge of these concepts. Overall these docs helped me a lot to understand the technical workings!


The Crowdstaking Application is a smart contract on Gnosis Chain that accepts a user’s xDAI and turns it into sDAI. In exchange, stakers receive BREAD tokens, minted at a 1-to-1 ratio with the collateralized xDAI.

All of the interest earned on the sDAI is helps fund the collective and its various member projects based on a monthly vote from BREAD holders. The Crowdstaking Application functions as a fundraising engine for the Breadchain Cooperative, while the BREAD token acts as a local currency within the ecosystem, promoting financial sustainability.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: remove "is" in this line


All of the interest earned on the sDAI is helps fund the collective and its various member projects based on a monthly vote from BREAD holders. The Crowdstaking Application functions as a fundraising engine for the Breadchain Cooperative, while the BREAD token acts as a local currency within the ecosystem, promoting financial sustainability.

Additionally, BREAD holders are able to to vote on how the yield generated from the sDAI is distributed among the projects part of the Breadchain Network every month.
Copy link
Collaborator

Choose a reason for hiding this comment

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

some typos; suggested fix:
"...BREAD holders are able to vote on how the yield generated from the sDAI is distributed among the projects that are part of the..."

9 // ... delegation snippet
10 }
```
On line 3 the native currency (xDai) of Gnosis Chain gets converted to an ERC20 representation ([wxDai](https://gnosisscan.io/address/0xe91D153E0b41518A2Ce8Dd3D7944Fa863463a97d)). This is done because in order to lock the xDai.
Copy link
Collaborator

Choose a reason for hiding this comment

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

last sentence doesn't read correctly; suggested fix:
"This is done to lock the xDai."

It would be helpful for my brain if there was one sentence prefacing this breakdown to explain the whole process more succinctly, GPT suggested:

"The minting process involves converting xDai to wxDai, locking it in the sDai contract to generate yield through the Dai Savings Rate, and minting BREAD tokens as vouchers for the deposited xDai, which can later be redeemed."

```
On line 2 , the `bal` variable represents the sDai balance of the Bread contract. This represents how much xDai is locked and earning yield.

In line 3 use the `bal` variable to determine how much xDai the Bread contract is eligible for by burning sDai, and we store that in the `assets` variable. This represents the original xDai locked and any rewards earned.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: rewrite to read "Line 3 uses the bal variable to determine..."

Also wondering about "rewards earned" - can that be more clear? My understanding is that this is talking about interest earned by the sDai via the DSR


To understand how much yield the Bread contract has , we first ascertain that by burning all sDai we have enough xDai for BREAD redemptions. While the state is unreachable, this validation is present for safety reasons. Once we are certain of that , the `assets - supply` subtraction in line 5 represents the **total liquid xDai available for claim to the Bread contract** minus **the total xDai "owed" to BREAD holders**. Thus, what is left over is what can be allocated to the Breadchain projects.

Click [here](https://docs.soliditylang.org/en/v0.8.27/types.html#ternary-operator) for a reference on the ternary operator that is used here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

ternary operators seem pretty standard; this link out feels a bit out of place to me.

- The total number of points allocated across all projects can vary.
- The actual voting power distributed to each project is calculated proportionally based on the points allocated.
- This system allows users to express their preferences with fine-grained control, as they can allocate any number of points (up to `maxPoints`) to each project.
- For example, if there are two projects and `maxPoints` is 100, a user could vote [100, 50], [1, 2], or any other combination, providing a wide range of possible distributions within the precision of the points system.
Copy link
Collaborator

Choose a reason for hiding this comment

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

is maxPoints a cap of how many points a project can receive or a cap of how many points a user can distribute?

i dont actually understand the example provided nor what the arrays ([100, 50], [1, 2]) are demonstrating


Here's what this function does:

1. It checks if the number of points matches the number of projects.
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh, so _points might be an array like [50, 100, 20] if there are 3 projects. each item in the array is a numerical value assigned by the user as a vote towards one of the projects, is that right?


1. It checks if the number of points matches the number of projects.

2. It calculates the total points and ensures they don't exceed the maximum allowed per project.
Copy link
Collaborator

Choose a reason for hiding this comment

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

i find myself still wanting to know more about this maximum, what it is and why

- Check if the start time is before the end time.
- Ensure the end time is not after the current block.
2. Initial Checkpoint Check:
- Get the total number of checkpoints for the account.
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is a checkpoint? why would an account have checkpoints? i think this is dealing with the part that scales voting power according to how long the account has been holding the BREAD, but that wouldnt be clear to me from reading at this point

- Break the loop as we've covered the entire interval.
7. Return Total Voting Power:
- The function returns the accumulated voting power over the specified interval.
Key Points:
Copy link
Collaborator

Choose a reason for hiding this comment

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

my brain would be helped by having these key points precede the function breakdown so that i can go through the details with higher level context

@RonTuretzky RonTuretzky changed the title Installation and documentation update [FROZEN] Installation and documentation update Oct 11, 2024
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