-
Notifications
You must be signed in to change notification settings - Fork 41
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
Fix: calculate genesis id properly #1359
Conversation
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.
is there an option to render it after fetching from grpc? it obviously won't change at this point, but in general it would be more robust to trust the local node on this info
@@ -11,7 +12,8 @@ import { toPublicService } from './utils'; | |||
// | |||
|
|||
export const generateGenesisID = (genesisTime: string, extraData: string) => { | |||
return `${toHexString(hash(genesisTime + extraData)).substring(0, 40)}`; | |||
const time = parseISODate(genesisTime).getTime() / 1000; |
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.
parseISODate returns milliseconds?
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
Agree about fetching from grpc, but we're indexing networks by genesis id (auto-connect to them, store logs using it and etc) — so we need it earlier (and even if Node does not work).
But probably Smapp should show some alerts in case the calculated genesis id and the one returned from API are different.
Okay, I just realized that if we will merge and deploy it as is — we will break UX for all Users.
|
4b057fd
to
19da36f
Compare
It is ready for review. How it works:
In case you are unlocking such a wallet, while you already have -- |
@monikasmolarek This is another issue, reproducible in |
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.
LGTM
It closes #1377