-
Notifications
You must be signed in to change notification settings - Fork 653
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
chore(primitives): drop BaseDecode
trait
#7079
Conversation
@matklad, wdyt about extracting the Stuff as fundamental as An alternative could be to collect stuff like that in a light weight |
I don’t think CryptoHash needs to be extracted —- it’s used pervasively, but it doesn’t seems like a good abstraction. In particular, I think good public API would make hashes typed, so that it’s clear which object this is a hash of. So, I also don’t think that extracting stuff along type boundaries makes sense at all, unless this is a very crisply defined type. It’s better to extract stuff along the logical domains. I predict that, once we extract It also seems to me that “near RPC api” and “logic of transaction processing in a near node” are distinct domains, with distinct requirements, and trying to share types between them would lead to false sharing. We don’t want to expose internals of nearcore to backwards compatibility weirdness, and vice verse. We already have this problem with account id, which has this extra nearcore-only flag. So, I would suggest to just blindly duplicate everything we need for RPC, to avoid unnecessary dependencies, as accidental dependencies are the problem we are facing with right now. If after extraction we’ll get the problem that there’s too much duplication, we should start thinking about solving that, but I don’t think it makes sense to worry about that prematurely: my prediction is that we won’t see a lot of duplication problem, because the stuff we are extracting is the stable RPC boundary anyway. There’s also a minor hope that RPC types would be much easier to shake into a good shape. For example, we have some awkward error types in from strings, and changing that in nearcore leads to non-local changes (sort of like when we were introducing account id). Without nearcore, we should be able to just write nice APIs as they should be. |
This PR has been automatically marked as stale because it has not had recent activity in the 2 weeks. |
@miraclx any reason for not just slapping auto-merge here? The change is very good, look forwrard to it ) |
Applying auto-merge label myself just to not let this already good improvement hang in limbo! |
Stumbled on this as part of #6850 The only implementation of `BaseDecode` is for `CryptoHash` whose logic is no different from the implementation of `FromStr`.
Stumbled on this as part of #6850
The only implementation of
BaseDecode
is forCryptoHash
whose logic is no different from the implementation ofFromStr
.