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

add contract migration check on all calls #1547

Merged
merged 47 commits into from
May 16, 2024

Conversation

ianthpun
Copy link
Contributor

@ianthpun ianthpun commented Apr 26, 2024

Closes #1543

Description

confirmed working here with a manual test

image


For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@ianthpun ianthpun changed the base branch from master to release/stable-cadence April 26, 2024 20:43
@ianthpun ianthpun changed the base branch from release/stable-cadence to feature/stable-cadence April 26, 2024 20:43
Copy link
Member

@chasefleming chasefleming left a comment

Choose a reason for hiding this comment

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

A couple minor things

internal/migrate/is_validated.go Outdated Show resolved Hide resolved
internal/command/command.go Outdated Show resolved Hide resolved
var missingContractErr error
if !slices.Contains(foundAddresses, addr) {
builder := strings.Builder{}
builder.WriteString("some contracts do not appear to have been a part of any emulated migrations yet, please ensure that it has been staged & wait for the next emulated migration (last migration report was at ")
Copy link
Member

Choose a reason for hiding this comment

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

Capitalize "Some"

@codecov-commenter
Copy link

codecov-commenter commented May 7, 2024

Codecov Report

Attention: Patch coverage is 24.24242% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 42.13%. Comparing base (2f95230) to head (67810c3).

Files Patch % Lines
internal/command/command.go 0.00% 18 Missing ⚠️
internal/migrate/is_validated.go 0.00% 6 Missing ⚠️
internal/migrate/stage.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           feature/stable-cadence    #1547      +/-   ##
==========================================================
- Coverage                   43.54%   42.13%   -1.41%     
==========================================================
  Files                          60       60              
  Lines                        4304     4184     -120     
==========================================================
- Hits                         1874     1763     -111     
- Misses                       2181     2188       +7     
+ Partials                      249      233      -16     
Flag Coverage Δ
unittests 42.13% <24.24%> (-1.41%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@chasefleming
Copy link
Member

@ianthpun has this been manually tested? The screenshot shows the code error output? That seems large and unnecessary. Can we keep everything but the error output and maybe say something like "run blah command" to see the errors?

@chasefleming
Copy link
Member

Looks like @jribbink already left a similar comment as I just did so plus one.

" - Account: %s\n"+
" - Contract: %s\n"+
" - Error: %s\n",
contract.ContractName,
Copy link
Member

Choose a reason for hiding this comment

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

Also, this is a lot to show. Is it better just to say like "3 contracts with errors found since last migration," and then a way to see more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya i agree i think it's quite a bit. Maybe we can list the contracts and then just like to the migration data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated now

image

I/ll update the original PR screenshot as well

Copy link
Member

@chasefleming chasefleming May 15, 2024

Choose a reason for hiding this comment

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

What does it look like if there are multiple that fail? Is there any sort of separation before they repeat?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

i'll update the initial PR to show that as well if anyones curious

@ianthpun ianthpun requested review from chasefleming and jribbink May 14, 2024 23:36
@chasefleming chasefleming added the Feature A new user feature or a new package API label May 15, 2024
@ianthpun ianthpun requested a review from jribbink May 16, 2024 17:45
@ianthpun ianthpun merged commit 8987859 into feature/stable-cadence May 16, 2024
6 checks passed
@ianthpun ianthpun deleted the ianthpun/validation-check branch May 16, 2024 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature A new user feature or a new package API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants