Add tests for AccountsDeployContract facet #213
Conversation
0xNeshi
force-pushed
the
acc-depl-cont-tests
branch
2 times, most recently
from
1352e2c
to
f96168f
Compare
0xNeshi
changed the title
| string veBondingName; | ||
| string veBondingSymbol; | ||
| uint256 veBondingDecimals; | ||
| address veBondingReserveDenom; | ||
| address veBondingReserveDenom; // should this be string? |
There was a problem hiding this comment.
Should veBondingReserveDenom this be string?
Name and type combo is really confusing.
There was a problem hiding this comment.
Honestly, DaoTokenData isn't used anywhere right now. @SovereignAndrey is the concept of SubDaos not fleshed out? Looks like we're missing some use cases of the data we're storing here.
There was a problem hiding this comment.
@stevieraykatz Yea, SubDaos are not even planned on for go-live a little while still, so we might better just disable the createSubDao endpoint in Accounts for now. There's prob some holes in there since:
A) no ones looked at those contracts yet for a re-work
B) we need better product requirements to ensure we're meeting all the needed items whilst we do the above (A)
There was a problem hiding this comment.
so we might better just disable the
createSubDaoendpoint in Accounts for now
@SovereignAndrey are you talking about AccountsDeployContract.createDaoContract? If so, we can just close this PR haha
There was a problem hiding this comment.
Yes, sorry was thinking about the cosmos naming I think. I think we can maybe keep this PR, but add a revert for the create endpoint. Ultimately, I think we'll need to pull this facet out into a "SubDaoFactory" as a separate contract since it's not really related to the Accounts, but only concerned in governing them and gatekeeping access to them. It'll need to be along with the removal/commenting out of the donation matching code too since that'll tie in with subDaos + for charities it will rely on HALO token being live too.
There was a problem hiding this comment.
The test and small renames look good. Leaving open so @SovereignAndrey can reply
| string veBondingName; | ||
| string veBondingSymbol; | ||
| uint256 veBondingDecimals; | ||
| address veBondingReserveDenom; | ||
| address veBondingReserveDenom; // should this be string? |
There was a problem hiding this comment.
Honestly, DaoTokenData isn't used anywhere right now. @SovereignAndrey is the concept of SubDaos not fleshed out? Looks like we're missing some use cases of the data we're storing here.
0xNeshi
force-pushed
the
acc-depl-cont-tests
branch
from
82d67fe
to
ac37585
Compare
0xNeshi
force-pushed
the
acc-depl-cont-tests
branch
from
19082b7
to
3fe17fd
Compare
Closes #159
Instructions on making this work
yarnoryarn installto install npm dependenciesyarn testto verify all tests still pass