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

Cannot deploy the new OpenZeppelin account contract #76

Closed
xJonathanLEI opened this issue Mar 1, 2022 · 11 comments · Fixed by #79
Closed

Cannot deploy the new OpenZeppelin account contract #76

xJonathanLEI opened this issue Mar 1, 2022 · 11 comments · Fixed by #79
Assignees

Comments

@xJonathanLEI
Copy link
Owner

Tried using starknet-rs to deploy the latest account contract from OpenZeppelin.

The transaction is not working:

{
  "status": "REJECTED",
  "transaction_failure_reason": {
    "tx_id": 1256114,
    "code": "TRANSACTION_FAILED",
    "error_message": "Error at pc=0:45:\nGot an exception while executing a hint.\nCairo traceback (most recent call last):\nUnknown location (pc=0:509)\nUnknown location (pc=0:499)\nUnknown location (pc=0:280)\nUnknown location (pc=0:242)\nUnknown location (pc=0:219)\n\nTraceback (most recent call last):\n  File \"<hint6>\", line 5, in <module>\nAssertionError: normalize_address() cannot be used with the current constants."
  },
  "transaction": {
    "contract_address": "0x615161661cdbea1c112383ce703d33299bef0e796903b88ec5601010ee22091",
    "contract_address_salt": "0x6c8035ea0cc6d4b0d933969db1367d7380adcaa462184b814f2def2bd9adf5",
    "constructor_calldata": [
      "1"
    ],
    "transaction_hash": "0x232db7959b3ccc22b6e1321fc412025a7d9733b90db809c29e47601b5ef5e3f",
    "type": "DEPLOY"
  }
}
@xJonathanLEI xJonathanLEI self-assigned this Mar 1, 2022
@xJonathanLEI
Copy link
Owner Author

Did some testing. I started out trying to deploy the artifact generated by the generate_artifacts.sh script with the official StarkNet CLI. It fails with the same error.

Then I tried directly compiling the account contract with starknet-compile and deploying with the official CLI. Much to my surprise, it works.

After some more testing, it turns out piping the artifact through jq is what causes the issue... The generate_artifacts.sh script uses jq to turn each artifact into a single line (so that Git diffs for example aren't a complete mess).

I tried using prettier to format the artifact instead, and it can still be successfully deployed with the official CLI.

I'm still in the process of trying to figure out what exactly has jq broken.

@xJonathanLEI
Copy link
Owner Author

OK it looks like the issue is jq reformatting decimal values into scientific notation. There seems to be no way to tell jq to not do this in the latest release of jq (1.6).

@xJonathanLEI
Copy link
Owner Author

Note that even after removing the use of jq, starknet-rs still cannot deploy the contract properly. It looks like serde_json has trouble handling very long decimal numbers as well.

So this issue is really caused by two things:

  • jq turning long decimal numbers into scientific notation; and
  • serde_json not properly handling long decimal numbers.

@milancermak
Copy link
Collaborator

@xJonathanLEI
Copy link
Owner Author

Turns out serde_json has this arbitrary_precision feature that adds support for long decimal numbers. Nice.

@xJonathanLEI
Copy link
Owner Author

xJonathanLEI commented Mar 3, 2022

@xJonathanLEI
Copy link
Owner Author

xJonathanLEI commented Mar 4, 2022

Hmmm I skimmed through the code of serde_json, and it seems that the only way to serialize a string without quotation marks would be to use the Number type when arbitrary_precision is enabled. In that case, it uses an internal marker $serde_json::private::Number which in turns uses NumberStrEmitter to do the trick. What NumberStrEmitter uses for serialization is not exposed through the serde API, so we can't just make use of it by implementing serde::Serialize. (raw_value seems to also enable this, but it's subject the the exact same clash issue)

Basically I'm trying to make sure we're outputting the exact same string as the one we read from the artifact file, and the original file presents the number as a decimal representation without quotes.

Since we can't use arbitrary_precision due to the clash with tag and untagged, and that it's not possible to implement something like NumberStrEmitter outside of serde_json, it looks like we're stuck.

@xJonathanLEI
Copy link
Owner Author

xJonathanLEI commented Mar 4, 2022

I guess the best we can do here is to enable arbitrary_precision and manually implement tag and untagged without the serde buffering. This issue has turned into a much larger undertaking than what I expected, thanks to the serde bug.

IMO it's almost always a bad idea to use very large numbers in JSON without turning them into strings... Lots of bad things can happen...

@xJonathanLEI
Copy link
Owner Author

Created a PR for using arbitrary_precision. I tested and I can now deploy the new account contract successfully.

That said, we're still not outputting the exact same JSON representation as the input artifact due to some other reasons. We should probably track this with a follow-up issue.

@xJonathanLEI
Copy link
Owner Author

Just did a quick test. With the fix in place we're outputting the exact same JSON string as the input file up to:

  • whitespaces, of course; and
  • the missing attributes and debug_info fields; and
  • ordering of fields; and
  • zero hex prefixes.

The last item can be fixed by changing UfeHex from {:#064x} to {:#x}.

I think we should be good. Contract deployment should work perfectly after the fix.

@milancermak
Copy link
Collaborator

Well, this turned out to be an unfortunate situation. Great job on sorting it out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants