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

Xtokens smart contract selector fix and test #898

Merged
merged 27 commits into from
Oct 15, 2021

Conversation

girazoki
Copy link
Collaborator

What does it do?

It fixes the selector and adds typescript tests with ether.js to ensure the encoding is correctly done

What important points reviewers should know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

@girazoki girazoki added the B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes label Oct 13, 2021
tests/tests/test-precompile-xtokens.ts Outdated Show resolved Hide resolved
tests/tests/test-precompile-xtokens.ts Outdated Show resolved Hide resolved
tests/tests/test-precompile-xtokens.ts Outdated Show resolved Hide resolved
tests/tests/test-precompile-xtokens.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@joelamouche joelamouche left a comment

Choose a reason for hiding this comment

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

Looks good! I need more information/descriptions about what is going on there

tests/tests/test-precompile-xtokens.ts Outdated Show resolved Hide resolved
tests/tests/test-precompile-xtokens.ts Show resolved Hide resolved
@girazoki
Copy link
Collaborator Author

The selectors were wrong because they were not using the struct type (u8, bytes[]). However we did not catch that because we were not testing this through a true solidity call. These tests fix that by compiling an instance of the precompiles contract and using the selectors gotten by ethers.js itself. If the selectors are wrong, the calls do not pass. If the selectors are correct, the call passes and the extrinsic is dispatched

Copy link
Contributor

@joelamouche joelamouche left a comment

Choose a reason for hiding this comment

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

Looks good :)

Base automatically changed from jeremy-precompile-util-offset to master October 15, 2021 14:13
@girazoki girazoki merged commit 705482b into master Oct 15, 2021
@girazoki girazoki deleted the xtokens_smart_contract_selector_fix_and_test branch October 15, 2021 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants