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

TestTxEnv::spawn_accounts should ignore internal addresses #694

Merged
merged 4 commits into from
Nov 30, 2022

Conversation

james-chf
Copy link
Contributor

This test helper writes an empty wasm VP to the ? subkey for any passed in addresses. We shouldn't need to do this for internal (and maybe also implicit?) addresses, since they shouldn't have a VP in storage. The motivation for this PR is that in eth-bridge-integration, there will be an internal account (EthBridgePool) which uses a different Merkle tree to other accounts and cannot have arbitrary bytes written to its ? subkey.

@james-chf james-chf force-pushed the james/mainline/dont-spawn-internal-account-vps branch from b1f7ee1 to db9a764 Compare October 26, 2022 15:54
@james-chf james-chf marked this pull request as ready for review October 26, 2022 16:39
@james-chf
Copy link
Contributor Author

@tzemanovic does this change make sense in terms of accounts? Maybe this fn should ignore implicit accounts as well, in which case it could just accept EstablishedAddresses. All wasm tests are passing with this change.

@tzemanovic
Copy link
Member

tzemanovic commented Oct 27, 2022

does this change make sense in terms of accounts? Maybe this fn should ignore implicit accounts as well, in which case it could just accept EstablishedAddresses. All wasm tests are passing with this change.

Yeah, I think this is fine and we can also ignore implicit accounts. I prefer this over changing type to EstablishedAddress as it's simpler ergonomics on caller side for a test helper and I don't think there's much danger to it.

The reason we have this helper for wasm is fn check_address_existence (shared/src/vm/host_env.rs) which already ignores implicit and internal accounts. On Storage there's also a method fn exists which should do the same, but it only works with established address and it's not being used at the moment.

@james-chf
Copy link
Contributor Author

Great, have updated to ignore implicit accounts while still accepting Address

tzemanovic added a commit that referenced this pull request Nov 16, 2022
* james/mainline/dont-spawn-internal-account-vps:
  Remove comment
  Update spawn_accounts to ignore implicit addresses
  Add changelog
  TestTxEnv::spawn_accounts should ignore internal addresses
@tzemanovic tzemanovic mentioned this pull request Nov 16, 2022
tzemanovic added a commit that referenced this pull request Nov 17, 2022
…count-vps' (#694) and 'bat/native-vp-test-tooling' (#738)

* james/mainline/dont-spawn-internal-account-vps:
  Remove comment
  Update spawn_accounts to ignore implicit addresses
  Add changelog
  TestTxEnv::spawn_accounts should ignore internal addresses

* bat/native-vp-test-tooling:
  Update tests/src/vm_host_env/tx.rs
  [feat]: Add multitoken suppor to the TestTxEnv. Add ability to execute wasm blobs on a TestTxEnv in order to test them with native vps
juped pushed a commit that referenced this pull request Nov 18, 2022
…count-vps' (#694) and 'bat/native-vp-test-tooling' (#738)

* james/mainline/dont-spawn-internal-account-vps:
  Remove comment
  Update spawn_accounts to ignore implicit addresses
  Add changelog
  TestTxEnv::spawn_accounts should ignore internal addresses

* bat/native-vp-test-tooling:
  Update tests/src/vm_host_env/tx.rs
  [feat]: Add multitoken suppor to the TestTxEnv. Add ability to execute wasm blobs on a TestTxEnv in order to test them with native vps
@juped juped merged commit d469e14 into main Nov 30, 2022
@juped juped deleted the james/mainline/dont-spawn-internal-account-vps branch November 30, 2022 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants