[refactor] #3874: Removes `IsAssetDefinitionOwnerQuery` #3920

benhhack · 2023-09-25T10:33:19Z

Description

Removed all instances of IsAssetDefinitionOwner except for some:

In wasm/validator/src/permission.rs and `/default.rs

With the function

fn is_asset_definition_owner(
        asset_definition_id: &AssetDefinitionId,
        authority: &AccountId,
    ) -> Result<bool> {
        IsAssetDefinitionOwner::new(asset_definition_id.clone(), authority.clone()).execute()
    }

I am unsure what to replace this result with. I have tried replacing it with FindAssetDefinitionById (since this is said to do the exact same thing as is asset definition owner in #3874) but this creates build errors. Should I delete everything that uses the is_asset_definition_owner function as well?

Linked issue

Remove IsAssetDefinitionOwnerQuery

This query was introduced only for the purposes of validator. Since #3442 we can use FindAssetDefinitionById to do the exact same thing.

Closes #3442

Checklist

I've read CONTRIBUTING.md
I've used the standard signed-off commit format (or will squash just before merging)
All applicable CI checks pass (or I promised to make them pass later)
(optional) I've written unit tests for the code changes
I replied to all comments after code review, marking all implemented changes with thumbs up

6r1d · 2023-09-25T10:56:13Z

Hello, Ben.
I will notify the others about this commit, but in the meantime, could you please sign-off your commit with git commit --amend --signoff and force-push to your branch?
Thanks.

benhhack · 2023-09-25T10:57:41Z

Hello, Ben. I will notify the others about this commit, but in the meantime, could you please sign-off your commit with git commit --amend --signoff and force-push to your branch? Thanks.

Hi there, will do.

mversic · 2023-09-26T05:28:20Z

Should I delete everything that uses the is_asset_definition_owner function as well?

no

I am unsure what to replace this result with. I have tried replacing it with FindAssetDefinitionById

you execute FindAssetDefinitionById query to get AssetDefinition and then compare AssetDefinition::Owner with authority

mversic · 2023-09-26T13:07:40Z

you are missing signoff again
you need to run cargo run --bin kagami -- schema > docs/source/references/schema.json and push the changes

benhhack · 2023-09-26T13:08:24Z

Thanks for the help. I'm getting problems with the mod tests and unfortunately, I don't know enough about Rust or the system to try to fix them. I'm doing research but also thought maybe you could help. The error response is

fatal runtime error: failed to initiate panic, error 5
error: test failed, to rerun pass `-p iroha_client --test mod`

Caused by:
  process didn't exit successfully: `<my-path>/iroha/target/debug/deps/mod-215035c2a4074a62 --nocapture` (signal: 6, SIGABRT: process abort signal)

The error looks like its coming from snapshot creation, when I run the tests more verbosely the line

2023-09-26T12:56:30.185430Z ERROR iroha_core::snapshot: Failed to create snapshot for wsv. error=Failed reading/writing "./storage/snapshot.tmp" from disk shows up.

Please advise on next steps.

Let me know how to provide any other information that may be helpful.

mversic · 2023-09-26T13:16:32Z

Please advise on next steps.

run cargo test locally

benhhack · 2023-09-26T13:19:03Z

run cargo test locally

Yes I have run this locally and I receive the error above with the tests in client.

coveralls · 2023-09-26T13:19:06Z

Pull Request Test Coverage Report for Build 6405672609

17 of 58 (29.31%) changed or added relevant lines in 7 files are covered.
6335 unchanged lines in 108 files lost coverage.
Overall coverage decreased (-0.8%) to 58.633%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
data_model/derive/src/lib.rs	0	1	0.0%
ffi/derive/src/attr_parse/getset.rs	6	7	85.71%
ffi/derive/src/attr_parse/derive.rs	3	5	60.0%
data_model/derive/src/filter.rs	0	12	0.0%
ffi/derive/src/convert.rs	5	30	16.67%

Files with Coverage Reduction	New Missed Lines	%
cli/src/main.rs	1	0.0%
cli/src/samples.rs	1	0.0%
cli/src/torii/pagination.rs	1	98.9%
config/base/src/runtime_upgrades.rs	1	51.72%
config/src/torii.rs	1	96.67%
core/src/smartcontracts/isi/block.rs	1	87.5%
crypto/src/merkle.rs	1	96.23%
data_model/src/domain.rs	1	48.25%
logger/src/lib.rs	1	95.15%
config/src/lib.rs	2	0.0%

Totals
Change from base Build 5423219773:	-0.8%
Covered Lines:	21700
Relevant Lines:	37010

💛 - Coveralls

mversic · 2023-09-26T18:11:02Z

Yes I have run this locally and I receive the error above with the tests in client.

oh, you also have to run: cargo run --release --bin kagami -- validator > configs/peer/validator.wasm and push the changes

benhhack · 2023-09-27T12:04:26Z

cargo run --release --bin kagami -- validator > configs/peer/validator.wasm

Okay, will do. Is this in the developer docs somewhere?

benhhack · 2023-09-27T12:09:53Z

Yes I have run this locally and I receive the error above with the tests in client.

oh, you also have to run: cargo run --release --bin kagami -- validator > configs/peer/validator.wasm and push the changes

I have done this and am getting the following error

    Finished release [optimized] target(s) in 3m 08s
     Running `target/release/kagami validator`
error: unrecognized subcommand 'validator'

Is there any documentation I can look at for kagami/these commands? What is their purpose?

mversic · 2023-09-27T12:34:15Z

Is there any documentation I can look at for kagami/these commands? What is their purpose?

sorry, I forgot we changed API for this recently. This command rebuilds validator/executor smartcontract:
cargo run --bin iroha_wasm_builder_cli build --optimize --outfile configs/peer/validator.wasm default_validator

mversic · 2023-09-28T08:49:52Z

@benhhack I hope you will fix CI errors so we can merge this

benhhack · 2023-09-28T10:32:23Z

@benhhack I hope you will fix CI errors so we can merge this

Yes, am working my way through trying to figure out commit squashing/message amending since its not something I've done before.

mversic · 2023-09-29T08:42:33Z

@benhhack you still have a conflict to resolve :)

mversic · 2023-10-03T07:42:40Z

@benhhack hi, can we expect an update this week?

benhhack · 2023-10-03T17:48:46Z

@benhhack hi, can we expect an update this week?

Hi yes, will be working on this tomorrow

benhhack · 2023-10-04T08:21:37Z

Hi @mversic. I'm unsure how to solve this conflict since I can't view the wasm file. Do you have any tips? I'd love to learn more about what's going on to pointing to any resources would also be very helpful.

I should also point out that not all the tests are passing despite running the commands you mentioned. Do you have any ideas?

mversic · 2023-10-04T08:28:49Z

Hi @mversic. I'm unsure how to solve this conflict since I can't view the wasm file. Do you have any tips? I'd love to learn more about what's going on to pointing to any resources would also be very helpful.

I should also point out that not all the tests are passing despite running the commands you mentioned. Do you have any ideas?

yes, you cannot see contents of a binary file. You have to build new validator.wasm with the command I have given previously and replace current validator with that one. That will resolve conflicts

benhhack · 2023-10-04T08:42:39Z

Hi @mversic. I'm unsure how to solve this conflict since I can't view the wasm file. Do you have any tips? I'd love to learn more about what's going on to pointing to any resources would also be very helpful.
I should also point out that not all the tests are passing despite running the commands you mentioned. Do you have any ideas?

yes, you cannot see contents of a binary file. You have to build new validator.wasm with the command I have given previously and replace current validator with that one. That will resolve conflicts

I have just done that and I see that the conflicts are not resolved.

mversic · 2023-10-04T08:46:22Z

I have just done that and I see that the conflicts are not resolved.

have you fetched latest changes from iroha2-dev and rebased your PR? First do that, then rebuild validator.wasm

benhhack · 2023-10-04T08:49:38Z

I have just done that and I see that the conflicts are not resolved.

have you fetched latest changes from iroha2-dev and rebased your PR? First do that, then rebuild validator.wasm

Yeah I have but will try that again.

mversic · 2023-10-04T08:52:23Z

I have just done that and I see that the conflicts are not resolved.

have you fetched latest changes from iroha2-dev and rebased your PR? First do that, then rebuild validator.wasm

Yeah I have but will try that again.

you have not rebased to latest iroha2-dev. Check what is latest commit (that is not yours) on your branch and what is latest commit on iroha2-dev. You will see additional commits on iroha2-dev branch

benhhack · 2023-10-04T09:09:12Z

The DCO check is now failing because some of the commits I've rebased on top of seem not to have signoffs.

mversic · 2023-10-04T09:13:00Z

The DCO check is now failing because some of the commits I've rebased on top of seem not to have signoffs.

DCO is the least of your concerns. Somehow you have made a mess of your local git history. Github now shows that you have modified 99 files. You will have to fix this

benhhack · 2023-10-04T09:32:57Z

How is it looking now @mversic ?

mversic · 2023-10-04T11:38:57Z

@benhhack please fix failing CI jobs, all tests are passing but you have formatting issues

benhhack · 2023-10-04T11:47:40Z

@benhhack please fix failing CI jobs, all tests are passing but you have formatting issues

Apologies, have run nightly and pushed the changes.

mversic · 2023-10-04T12:04:14Z

@benhhack please squash your commits into 1 and I will approve your PR

benhhack · 2023-10-04T12:53:20Z

@mversic I'm struggling with the squashing because of the validator.wasm merge conflicts. A standard interactive rebase isn't working. Do you have any tips?

mversic · 2023-10-09T08:27:02Z

@mversic I'm struggling with the squashing because of the validator.wasm merge conflicts. A standard interactive rebase isn't working. Do you have any tips?

you can have a look here. Tell me if this helps, we would like to merge this PR soon

benhhack · 2023-10-09T08:31:10Z

@mversic I'm struggling with the squashing because of the validator.wasm merge conflicts. A standard interactive rebase isn't working. Do you have any tips?

you can have a look here. Tell me if this helps, we would like to merge this PR soon

Sure, this will be completed tomorrow. When I update the branch would you recommend I update with a rebase or a merge?

mversic · 2023-10-09T08:38:29Z

@mversic I'm struggling with the squashing because of the validator.wasm merge conflicts. A standard interactive rebase isn't working. Do you have any tips?

you can have a look here. Tell me if this helps, we would like to merge this PR soon

Sure, this will be completed tomorrow. When I update the branch would you recommend I update with a rebase or a merge?

We use rebase workflow. Also, there is some more conflicts now that you have to resolve

Signed-off-by: benhhack <[email protected]>

benhhack · 2023-10-10T08:49:40Z

@mversic all the updates have definitely confused things, one of the recent ones also seems to have overlapping work with this one here. Again apologies for taking so long with this. I appreciate the assistance and I'm learning a lot. I'm going to refork the repository in its most updated form and work from there so that I can have a clean commit history without all the merging.

github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Sep 25, 2023

mversic self-assigned this Sep 26, 2023

Arjentix assigned benhhack Sep 26, 2023

benhhack marked this pull request as ready for review September 28, 2023 14:10

benhhack requested review from s8sato, appetrosyan, mversic, Arjentix, SamHSmith, QuentinI, outoftardis, ilchu, pesterev, Erigara, 0x009922 and DCNick3 as code owners September 28, 2023 14:10

mversic assigned pesterev Oct 9, 2023

benhhack added 2 commits October 10, 2023 09:09

IsAssetDefinitonOwner removed

69aea73

Signed-off-by: benhhack <[email protected]>

Nightly formatting

18345dd

Signed-off-by: benhhack <[email protected]>

benhhack closed this Oct 10, 2023

benhhack mentioned this pull request Oct 10, 2023

[refactor] #3874: Remove IsAssetDefinitionOwner #3979

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[refactor] #3874: Removes `IsAssetDefinitionOwnerQuery` #3920

[refactor] #3874: Removes `IsAssetDefinitionOwnerQuery` #3920

benhhack commented Sep 25, 2023

6r1d commented Sep 25, 2023

benhhack commented Sep 25, 2023

mversic commented Sep 26, 2023

mversic commented Sep 26, 2023

benhhack commented Sep 26, 2023

mversic commented Sep 26, 2023

benhhack commented Sep 26, 2023

coveralls commented Sep 26, 2023 •

edited

Loading

mversic commented Sep 26, 2023

benhhack commented Sep 27, 2023

benhhack commented Sep 27, 2023

mversic commented Sep 27, 2023

mversic commented Sep 28, 2023

benhhack commented Sep 28, 2023

mversic commented Sep 29, 2023

mversic commented Oct 3, 2023

benhhack commented Oct 3, 2023

benhhack commented Oct 4, 2023

mversic commented Oct 4, 2023

benhhack commented Oct 4, 2023

mversic commented Oct 4, 2023

benhhack commented Oct 4, 2023

mversic commented Oct 4, 2023

benhhack commented Oct 4, 2023

mversic commented Oct 4, 2023

benhhack commented Oct 4, 2023

mversic commented Oct 4, 2023

benhhack commented Oct 4, 2023

mversic commented Oct 4, 2023

benhhack commented Oct 4, 2023 •

edited

Loading

mversic commented Oct 9, 2023

benhhack commented Oct 9, 2023

mversic commented Oct 9, 2023

benhhack commented Oct 10, 2023

[refactor] #3874: Removes IsAssetDefinitionOwnerQuery #3920

[refactor] #3874: Removes IsAssetDefinitionOwnerQuery #3920

Conversation

benhhack commented Sep 25, 2023

Description

Linked issue

Checklist

6r1d commented Sep 25, 2023

benhhack commented Sep 25, 2023

mversic commented Sep 26, 2023

mversic commented Sep 26, 2023

benhhack commented Sep 26, 2023

mversic commented Sep 26, 2023

benhhack commented Sep 26, 2023

coveralls commented Sep 26, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6405672609

💛 - Coveralls

mversic commented Sep 26, 2023

benhhack commented Sep 27, 2023

benhhack commented Sep 27, 2023

mversic commented Sep 27, 2023

mversic commented Sep 28, 2023

benhhack commented Sep 28, 2023

mversic commented Sep 29, 2023

mversic commented Oct 3, 2023

benhhack commented Oct 3, 2023

benhhack commented Oct 4, 2023

mversic commented Oct 4, 2023

benhhack commented Oct 4, 2023

mversic commented Oct 4, 2023

benhhack commented Oct 4, 2023

mversic commented Oct 4, 2023

benhhack commented Oct 4, 2023

mversic commented Oct 4, 2023

benhhack commented Oct 4, 2023

mversic commented Oct 4, 2023

benhhack commented Oct 4, 2023

mversic commented Oct 4, 2023

benhhack commented Oct 4, 2023 • edited Loading

mversic commented Oct 9, 2023

benhhack commented Oct 9, 2023

mversic commented Oct 9, 2023

benhhack commented Oct 10, 2023

[refactor] #3874: Removes `IsAssetDefinitionOwnerQuery` #3920

[refactor] #3874: Removes `IsAssetDefinitionOwnerQuery` #3920

coveralls commented Sep 26, 2023 •

edited

Loading

benhhack commented Oct 4, 2023 •

edited

Loading