Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Migrate Account to component #783
Migrate Account to component #783
Changes from 6 commits
331844d
2e7758e
77cca2d
4fc9be5
ce91ed0
4a90358
7f74150
bff9179
4415191
100c483
b3e2106
0570e44
52df8b1
9705ce7
feeac44
0f64992
f9ce9eb
fcdcdf7
d12a610
66c3fd7
daa9d68
495ff94
472b04c
a83b810
cc8d2f9
296122f
2e16eaa
d53d1c9
265faa7
c89e8b6
4eeafa9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the
Account
preset name. I guess we can differentiate between the twoAccount
s withcomponent
andcontract
, but idk.AccountPreset
maybe or something better? Thoughts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, but i'd go with
BasicAccount
or something, to distinguish from futureEthereumAccount
, orMultisigAccount
presets. I'd never call "preset" a contract btw.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I rather keep the name clean as Account, because the separation exists in the directory (imports) structure, and you cannot use a component as a preset, or a preset as a component (compiler error), so the potential issues from this clash are minimal IMO.
For Account we can come up with a name, but what about ERC20? Which names would you use for the preset? Since we are probably removing presets in favor of Wizard anyway, I vote for keeping the name as just Account (and ERC20 when it comes the time).
Let me know if I still should update it, and to what name then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can keep it like this for the time being, we can reassess post wizard