Remove contract hex encoding #480
Conversation
nozim
requested review from
janezpodhostnik,
sideninja,
chasefleming and
nvdtf
as code owners
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #480 +/- ##
==========================================
- Coverage 61.23% 61.11% -0.13%
==========================================
Files 26 26
Lines 3312 3312
==========================================
- Hits 2028 2024 -4
- Misses 1112 1116 +4
Partials 172 172
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
|
I could add alternative methods with usage of string instead to make a smooth transition? |
|
I think we can make this non-breaking change: onflow/sdks#18 (comment) |
templates/accounts_test.go
Outdated
| require.Less(t, argumentsSize, len(testContractBody)*2+500, | ||
| "The create account argument size should not grow over "+ | ||
| "2 times the contract code (converted to hex) + 500 bytes of extra data.") | ||
| }) |
There was a problem hiding this comment.
I think here should be fixed for non-hex case.
There was a problem hiding this comment.
it already checks utf8 contract length to be below the limit.
|
Nice! @janezpodhostnik Why would this be a breaking change? |
|
@janezpodhostnik the only potential breaking point I see for existing contracts already deployed in hex format. Kindly help me to understand if I miss anything here. |
|
Looking at this again... yeah sorry I thought this was a breaking change, but I see that we are only using fields already on the Contract object. |
chasefleming
dismissed
their stale review
I think the sdks dependency actually needs to be bumped for this
templates/accounts_test.go
Outdated
| @@ -50,7 +48,7 @@ func TestCreateAccount(t *testing.T) { | |||
| argumentsSize += len(argument) | |||
| } | |||
| require.Less(t, txSize, 1000, "The create account script should not grow over 1kB.") | |||
| require.Less(t, argumentsSize, contractLen*2+500, | |||
| require.Less(t, argumentsSize, len(testContractBody)+500, | |||
| "The create account argument size should not grow over "+ | |||
| "2 times the contract code (converted to hex) + 500 bytes of extra data.") | |||
There was a problem hiding this comment.
This message needs updating as well
updated, kindly check latest commits. |
|
I think this PR needs to be merged and release first before updating: onflow/sdks#18 @turbolent can you confirm? |
|
@chasefleming @turbolent updated from master. Kindly check 🙏 |
|
@nozim That PR I linked was not merged or released yet so updating from master would not actually address the issue. |
Closes: #467
Description
For contributor use:
masterbranchFiles changedin the Github PR explorer