cronos
cronos copied to clipboard
Problem: Missing changes to ADR007 for generic event arguments
Solution: Revise the ADR with the new proposal
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
- [ ] Have you read the CONTRIBUTING.md?
- [ ] Does your PR follow the C4 patch requirements?
- [ ] Have you rebased your work on top of the latest master?
- [ ] Have you checked your code compiles? (
make) - [ ] Have you included tests for any non-trivial functionality?
- [ ] Have you checked your code passes the unit tests? (
make test) - [ ] Have you checked your code formatting is correct? (
go fmt) - [ ] Have you checked your basic code style is fine? (
golangci-lint run) - [ ] If you added any dependencies, have you checked they do not contain any known vulnerabilities? (
go list -json -m all | nancy sleuth) - [ ] If your changes affect the client infrastructure, have you run the integration test?
- [ ] If your changes affect public APIs, does your PR follow the C4 evolution of public contracts?
- [ ] If your code changes public APIs, have you incremented the crate version numbers and documented your changes in the CHANGELOG.md?
- [ ] If you are contributing for the first time, please read the agreement in CONTRIBUTING.md now and add a comment to this pull request stating that your PR is in accordance with the Developer's Certificate of Origin.
Thank you for your code, it's appreciated! :)
other that that, lgtm
However one thing which is not specified is how to serialized the args to byte[] assuming args will be a struct or object which contains multiple parameters.
if I understand it correctly, I think that's purposefully left open, i.e. left for dapps to converge on something that makes sense in their future context.
if I understand it correctly, I think that's purposefully left open, i.e. left for dapps to converge on something that makes sense in their future context.
Technically it cannot be left open for dapps to decide on their standard because the data needs to be deserialize on-chain, depending on our implementation, dapps would need to follow the same serialization rule
some checks are failing
some checks are failing
It's from another file. Fixing it here: https://github.com/crypto-org-chain/cronos/pull/679 and we can merge this PR I believe.
@calvinaco should we clarify that the extraData should be encoded outside of the evm call and passed into the contract call as the input, otherwise if the contract encoded the extraData inside contract and we upgraded the chain logic about extraData encoding/decoding, contract devs still need to upgrade the contract.
@calvinaco should we clarify that the
extraDatashould be encoded outside of the evm call and passed into the contract call as the input, otherwise if the contract encoded theextraDatainside contract and we upgraded the chain logic aboutextraDataencoding/decoding, contract devs still need to upgrade the contract.
Yes, I think this can be added as a guideline but not enforced. The contract developer may still want to do some tricks by constructing the argument by themselves, as long as their contract is upgradable this can be managed.
@adu-crypto @yihuang If there's no other concerns I think we can merge this.