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

Fix quorum formatting #1348

Merged
merged 6 commits into from
Feb 9, 2024
Merged

Fix quorum formatting #1348

merged 6 commits into from
Feb 9, 2024

Conversation

mudrila
Copy link
Contributor

@mudrila mudrila commented Feb 9, 2024

Description

Multiply quorum percentage by 10000 to get it into proper format during Safe deployment, adjust quorum displaying accordingly

Notes

Issue / Notion doc (if applicable)

Closes #1347

Testing

Screenshots (if applicable)

@mudrila mudrila requested review from adamgall and Da-Colon February 9, 2024 16:29
@mudrila mudrila self-assigned this Feb 9, 2024
Copy link

netlify bot commented Feb 9, 2024

Deploy Preview for fractal-framework-interface-dev ready!

Name Link
🔨 Latest commit 82e1662
🔍 Latest deploy log https://app.netlify.com/sites/fractal-framework-interface-dev/deploys/65c662525d03580008899820
😎 Deploy Preview https://deploy-preview-1348--fractal-framework-interface-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@adamgall adamgall left a comment

Choose a reason for hiding this comment

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

Approving, but would appreciate an answer to the comment I left in the "builder" flow.

src/models/AzoriusTxBuilder.ts Outdated Show resolved Hide resolved

const quorumPercentage = quorumNumerator.mul(100).div(quorumDenominator);
Copy link
Member

Choose a reason for hiding this comment

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

Yes!

Comment on lines +101 to +104
(this.daoData as AzoriusERC20DAO | AzoriusERC721DAO).votingStrategyType ===
VotingStrategyType.LINEAR_ERC20
) {
const azoriusDAOData = this.daoData as AzoriusERC20DAO;
Copy link
Member

Choose a reason for hiding this comment

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

@mudrila can you explain these few lines a bit more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the parentAllocationAmount exists only for AzoriusERC20DAO, only then we need to call setEncodedSetupTokenClaimData and setPredictedTokenClaimAddress - this const azoriusDAOData = this.daoData as AzoriusERC20DAO; performed solely for type constraints.
This is mostly copy-paste of lines removed from constructor but before it was daoData = daoData as AzoriusERC20DAO and we just don't need (and actually can't ahah) to reassign to this.daoData

And if you're talking about async init - that's to fetch QUORUM_DENOMINATOR on chain, we need to await for RPC request promise.

@adamgall
Copy link
Member

adamgall commented Feb 9, 2024

@mudrila you may merge at your convenience

@mudrila mudrila merged commit 8aba272 into develop Feb 9, 2024
4 of 5 checks passed
@mudrila mudrila deleted the fix/quorum-multiplication branch February 9, 2024 18:23
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.

Bug: Creating new DAO sets incorrect quorumNumerator onchain
2 participants