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

Add Multisig #433

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Add Multisig #433

wants to merge 10 commits into from

Conversation

immrsd
Copy link
Contributor

@immrsd immrsd commented Jan 28, 2025

No description provided.

@immrsd immrsd self-assigned this Jan 28, 2025
@immrsd
Copy link
Contributor Author

immrsd commented Jan 28, 2025

Test project (including only Multisig contracts) compiled successfully with 2.9.1:

image

Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

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

The PR looks good to me @immrsd , but I would like to also check the Deploy Preview when available @ericglau.

Copy link
Member

@ericglau ericglau left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

}

export interface MultisigOptions extends CommonOptions {
name: string;
Copy link
Member

Choose a reason for hiding this comment

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

The name is configurable from the API but not from the UI. Would there be any benefit to allow customizing the name in the UI?

(I realize this is also the case for VestingWallet)

Copy link
Contributor

@andrew-fleming andrew-fleming left a comment

Choose a reason for hiding this comment

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

LGTM! I left a small comment

Comment on lines 105 to 106

export const numberPattern = /^(?!$)(\d*)(?:\.(\d+))?(?:e(\d+))?$/;
Copy link
Contributor

Choose a reason for hiding this comment

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

Small thing: we could move this to somewhere in utils/ to avoid repetition since we're also using it in governor

Copy link
Member

Choose a reason for hiding this comment

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

This regex looks like it should support decimals and e notation, although those give an error in the UI. I'm not sure why that is the case.

Regardless, perhaps the regex should be updated to only look for natural numbers.

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.

4 participants