-
-
Notifications
You must be signed in to change notification settings - Fork 513
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
Globaldb migration: new tables and move assets #3400
Globaldb migration: new tables and move assets #3400
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.
Some initial comments.
@@ -622,7 +622,7 @@ def __post_init__( | |||
object.__setattr__(self, 'decimals', data.decimals) | |||
object.__setattr__(self, 'protocol', data.protocol) | |||
|
|||
underlying_tokens = GlobalDBHandler().fetch_underlying_tokens(data.ethereum_address) | |||
underlying_tokens = GlobalDBHandler().fetch_underlying_tokens(data.identifier) |
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.
Also above here: https://github.com/rotki/rotki/pull/3400/files#diff-d838b4670ae94dd2743ece1ac79b73a99865b761474821ed39d7d3498d830338R616
Shouldn't it be talking about an EVM token?
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.
Need to rename some things. Will do in the next PR
return {k.value: addr for k in CHAINID} | ||
|
||
|
||
def query_coingecko_for_addresses(identifier: str) -> Dict[str, str]: # noqa: E501 pylint: disable=unused-argument |
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.
As discussed. This CAN NOT be part of the actual DB migration. This can only happen after the migration is complete, the user has logged in and a long-running task can executed.
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.
Yes I agree. Won't use it right here but needed a function to query ids. Will move this function to a different place since I don't use it
|
||
for entry in query: | ||
try: | ||
ids = query_coingecko_for_addresses(entry[6]) |
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.
Querying any external service, especially in a for loop should not be part of a DB upgrade, ever. A DB upgrade is one of the most sensitive things we can do and if it goes wrong the user's installation may be fucked. So we need to keep it as simple as possible.
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. This logic has to move but it will be in another PR. They ids and information would come from the packaged globaldb but at this point I needed something to simulate this and test that the migration works. Also will be useful to move it into the script we will use to create the new globaldb
48f27f5
to
1bd404a
Compare
1bd404a
to
f966b58
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.
As discussed in standup, do the 1 change I told you either here or in the next PR. And we can merge this so you can continue work in the branch.
@@ -148,6 +160,49 @@ | |||
); | |||
""" | |||
|
|||
DB_V3_CREATE_COMMON_ASSETS_DETAILS = """ |
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.
And this won't stay right? We willl only have 1 version of creation command per table.
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 was thinking in someone comming from an old version but I believe it doesn't make sense? We can go ahead and remove the versions in the names
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.
it doesn't make sense. We never keep old version creation schemas around. That's what DB upgrades are for.
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.
As discussed yesterday ,will just merge this and later review the long lasting branch
Danger
This branch will modify your globaldb and break it. Make a copy of it before executing ANY code of it.
What has been done