-
Notifications
You must be signed in to change notification settings - Fork 25
feat: support account link pallet update #557
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
Merged
Merged
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
57fcee3
chore: pull in changes from typegen branch
rflechtner e044463
chore: add augmented types
rflechtner 0b5d360
chore: simplify types
rflechtner c2090ca
docs: document makeJsonEnum
rflechtner c0472d0
fix: ethereum signing support
rflechtner 3c4b9e4
fix: ethereum signing and address decoding
rflechtner 78fcece
chore: skip ethereum linking tests for now
rflechtner be3d91b
chore: adapt to interface changes
rflechtner 4bb0fd9
Merge branch 'develop' into rf-account-link-update
rflechtner 45fdf28
Merge branch 'develop' into rf-account-link-update
rflechtner 16a573b
docs: add docstrings
rflechtner 18a3833
chore: improve some typings
rflechtner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
This looks just like
Record. What does it do differently and is it important enough to have a new type for it? Because it makes things more confusing.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.
ok, I played around with a bit and
Recordwould not work here.The only solution I could come up with is:
Which I like more, because it is more straightforward and understandable. Only downside is, that it allows empty objects, but I don't see a way around that. On the other side,
makeJsonEnumwould not be needed anymore, which currently uses type assertion withasand is needed, because it would not be possible to assign to the JsonEnum type without it.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.
AFAIK it's the only way to turn
'a' | 'b'into something like{a: any} | {b: any}, which is the sort of type that polkadot requires for the creation of enums. What you proposed gives you{a?: any, b?: any}which besides empty objects also allows{a: 1, b: 2}. And it is not assignable to the polkadot type, so I don't think that will work.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.
Another way is to list the types explicitly:
{a: any} | {b: any}. It is wordier and longer (a downside) but also explicit (an upside).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.
but what does that solve? if variable
ais typed as a union of string literals, typescript interprets{[a]: any}as{[key: string]: any}and will not accept it for a signature that requires{a: any} | {b: any}