-
Notifications
You must be signed in to change notification settings - Fork 14
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
Track colony versions in the database #163
Track colony versions in the database #163
Conversation
@@ -39,6 +39,8 @@ const ExtensionActionButton = ({ extensionData }: Props) => { | |||
); | |||
}; | |||
|
|||
const isSupportedColonyVersion = colony.version >= 5; |
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.
There used to be a ColonyVersion
enum exported from colony-js
but it's not there anymore. Open to suggestions on replacing the hardcoded value
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'd say have a constant
const MIN_SUPPORTED_COLONY_VERSION = 5
const isSupportedColonyVersion = colony.version >= MIN_SUPPORTED_COLONY_VERSION;
or similar
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.
Looks good 👍
@@ -39,6 +39,8 @@ const ExtensionActionButton = ({ extensionData }: Props) => { | |||
); | |||
}; | |||
|
|||
const isSupportedColonyVersion = colony.version >= 5; |
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'd say have a constant
const MIN_SUPPORTED_COLONY_VERSION = 5
const isSupportedColonyVersion = colony.version >= MIN_SUPPORTED_COLONY_VERSION;
or similar
|
||
const extensionCompatible = isExtensionCompatible( | ||
Extension[extensionData.extensionId], | ||
extensionData.availableVersion as ExtensionVersion, | ||
10, | ||
(extensionData.availableVersion + 1) as ExtensionVersion, |
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.
How come we're checking if availableVersion + 1 is compatible? Isn't availableVersion the latest version in the db (i.e. latest + 1 doesn't exist)?
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.
Good spot, my bad!
@@ -230,6 +230,7 @@ const createMetacolony = async (singerOrWallet) => { | |||
name: 'meta', | |||
profile: { displayName: 'Metacolony' }, | |||
type: 'METACOLONY', | |||
version: BigNumber.from(metacolonyVersion).toNumber() |
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.
BigNumber.from...toNumber()
could live as a little utility e.g.
numbers.ts
const toNumber = (bigNumber) => BigNumber.from(bigNumber).toNumber()
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.
Makes sense 👍
@@ -304,6 +305,7 @@ function* colonyCreate({ | |||
name: givenColonyName, | |||
colonyNativeTokenId: tokenAddress, | |||
profile: { displayName }, | |||
version: BigNumber.from(currentColonyVersion).toNumber(), |
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.
Now that we're storing currentColonyVersion
in the db, we should probably also retreive it from there too instead of from the chain? On the basis that it is presumably faster to query the db.
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'm thinking that this would be covered by #179
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.
Yep, this looks very clean. Can't really find anything I'd want to change, but my review here is late, after you've addressed Will's comments.
Anyway, nice one 💯
Description
This PR adds support for tracking colony version in the block-ingestor. See block-ingestor#4 for details and testing.
Summary of CDapp changes:
version
field on Colony modelitem
field of CurrentVersion model tokey
to avoid confusion withitems
version
field to all existing create colony mutationsisSupportedColonyVersion
checks to extensionsResolves #102