-
Notifications
You must be signed in to change notification settings - Fork 853
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
refactor(experimental): add coercion utility functions #1619
Conversation
7d927b7
to
b3e8de6
Compare
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.
Nice! These are way nicer :)
Do you have any thoughts on whether we should still export the assertIs...
functions?
Good call. I would say no, we shouldn't actually. Want me to add to this PR? |
I think we might as well remove them here yep. I don't think there's any case where you'd need to use the assert specifically instead. Just have to make sure we export a similar conversion function for any new ones going forward but that's not a bad thing! |
Hm, now that I think about it, I think the With that in mind, I'd have to still export them out of their modules, but can export only the coercion and type from each module in |
Personally, I'd vote for the following consistent API for all types: type SomeType = ...
export function isSomeType(value): value is SomeType {...}
export function assertIsSomeType(value): asserts value is SomeType {...}
export function someType(value): SomeType {...} Perhaps with the exception of type Base58EncodedAddress = ...
export function isBase58EncodedAddress(value): value is Base58EncodedAddress {...}
export function assertIsBase58EncodedAddress(value): asserts value is Base58EncodedAddress {...}
export function address(value): Base58EncodedAddress {...} |
OK yeah I can dig that. I'll update this PR. Do we think just exporting all of each is OK? Each module is wildcard-exported rn |
I guess each one would look like this, since we want the export type UnixTimestamp = number & { readonly __brand: unique symbol };
export function isUnixTimestamp(putativeTimestamp: number): putativeTimestamp is UnixTimestamp {
// see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date#the_epoch_timestamps_and_invalid_date
if (putativeTimestamp > 8.64e15 || putativeTimestamp < -8.64e15) {
return false;
}
return true;
}
export function assertIsUnixTimestamp(putativeTimestamp: number): asserts putativeTimestamp is UnixTimestamp {
// see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date#the_epoch_timestamps_and_invalid_date
try {
if (putativeTimestamp > 8.64e15 || putativeTimestamp < -8.64e15) {
throw new Error('Expected input number to be in the range [-8.64e15, 8.64e15]');
}
} catch (e) {
throw new Error(`\`${putativeTimestamp}\` is not a timestamp`, {
cause: e,
});
}
}
export function unixTimestamp(putativeTimestamp: number): UnixTimestamp {
assertIsUnixTimestamp(putativeTimestamp);
return putativeTimestamp;
} |
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.
export function address<TAddress extends string = string>( | ||
putativeBase58EncodedAddress: TAddress | ||
): Base58EncodedAddress<TAddress> { | ||
assertIsBase58EncodedAddress(putativeBase58EncodedAddress); |
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 is obviously the right thing to do, semantically, I just wish that doing this didn't have a CPU cost.
// I get this for free.
const address = '555555555555555555555555` as Base58EncodedAddress<'555555555555555555555555'>;
// This calls `base58.serialize` to perform the assertion, ffs
const address = address('555555555555555555555555');
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.
Doesn't seem like we can avoid it, right? If someone uses the former, they get the CPU bonus but no assertion, and if they instead use the latter, it's the opposite.
Ideally the real slick devs could just make sure their string is a valid base58 address and go with the former
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.
Yeah. I'm just whining out loud.
b3e8de6
to
24ae866
Compare
Added @lorisleiva's API suggestions. Good to go! |
🎉 This PR is included in version 1.78.6 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up. |
This PR adds type cercion utility functions such as
lamports()
andaddress()
to our API.Note: I originally felt like these didn't belong in
@solana/rpc-core
, but I followed the pattern of placing these coercion functions in the same file as their assertions. It seems to make sense. Lmk any thoughts on where to place these.Closes #1600