Skip to content
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

Ks contract versioning #2951

Merged
merged 12 commits into from
Nov 27, 2018
Merged

Ks contract versioning #2951

merged 12 commits into from
Nov 27, 2018

Conversation

captnseagraves
Copy link
Contributor

Implemented contract versioning

@codecov
Copy link

codecov bot commented Nov 26, 2018

Codecov Report

Merging #2951 into grants will increase coverage by 13.44%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           grants   #2951       +/-   ##
==========================================
+ Coverage   17.05%   30.5%   +13.44%     
==========================================
  Files         175     181        +6     
  Lines       13864   13908       +44     
  Branches     1813    1815        +2     
==========================================
+ Hits         2365    4242     +1877     
+ Misses      11491    9533     -1958     
- Partials        8     133      +125
Impacted Files Coverage Δ
app/grants/views.py 19.59% <ø> (+19.59%) ⬆️
app/grants/models.py 92.68% <100%> (+1.32%) ⬆️
app/retail/templatetags/is_in_list.py 100% <0%> (ø)
app/kudos/templatetags/kudos_extras.py 70% <0%> (ø)
app/grants/templatetags/grants_extra.py 71.42% <0%> (ø)
app/retail/templatetags/matches.py 100% <0%> (ø)
app/dashboard/templatetags/add_url_schema.py 71.42% <0%> (ø)
app/legacy/urls.py 100% <0%> (ø)
app/kudos/models.py 54.4% <0%> (+1.03%) ⬆️
... and 66 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b41fd07...22fe0d7. Read the comment docs.

@owocki
Copy link
Contributor

owocki commented Nov 26, 2018

screen shot 2018-11-26 at 2 16 34 pm

tried leaving this comment then my computer froze bc the PR is so big

from django.db import migrations


class Migration(migrations.Migration):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SaptakS @thelostone-mc do these migrations look ok to you?

app/assets/v2/js/grants/shared.js Outdated Show resolved Hide resolved
app/assets/v2/js/grants/shared.js Outdated Show resolved Hide resolved
app/assets/v2/js/grants/shared.js Outdated Show resolved Hide resolved
@captnseagraves captnseagraves merged commit abc9c68 into grants Nov 27, 2018
@@ -112,6 +112,12 @@ class Meta:
default='0x0',
help_text=_('The contract address of the Grant.'),
)
contract_version = models.DecimalField(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@captnseagraves
cc @owocki

Should we include a process for handling the ABI based on contract version, so it outputs the appropriate static URL or a given contract version..?

Also, we are we using a decimalfield if decimal_places=0 with max_digits=3?

Can we switch this to a charfield? If we're going by semantic versioning, decimalfield won't make sense: 0.1.0 for example. If we're using integers like: 0 or 1, we should use positiveintegerfield here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we switch this to a charfield? If we're going by semantic versioning, decimalfield won't make sense: 0.1.0 for example. If we're using integers like: 0 or 1, we should use positiveintegerfield here.

this makes sense to me

Should we include a process for handling the ABI based on contract version, so it outputs the appropriate static URL or a given contract version..?

as does this

@thelostone-mc thelostone-mc deleted the ks_contract_versioning branch December 28, 2018 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gitcoin Grants Gitcoin Grants
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants