Conversation
nozim
requested review from
chasefleming,
devbugging,
janezpodhostnik and
nvdtf
as code owners
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
janezpodhostnik
left a comment
janezpodhostnik
left a comment
There was a problem hiding this comment.
The code lgtm, but what is the release strategy here? This will break a lot of code right?
|
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
| 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. |
7 participants
Closes: #467
Description
For contributor use:
masterbranchFiles changedin the Github PR explorer