feat: add transaction_hash and block_number in the manifest file

[remybar]

Related to: #748

For the world and every deployed contracts, store the transaction_hash and the block_number of the transaction which deploys them.

Pending points:

• Currently, the deployment manifest file is created by merging the base, overlay and remote manifest files, replacing the old deployment manifest file.
  transaction_hash and block_number data are collected during migration, so they are stored in the deployment manifest only. I don't think they are easily retrievable from the remote manifest (i.e from the blockchain) so we should also merge the old deployment manifest file with the other manifests as a starting point of the migration to avoid losing transaction_hash and block_number of not updated contracts.
• stored the seed used to create the world. It looks like the seed is just the name of the world, and the name is already stored. So, did I miss something for this point ?

@glihm @tarrencev
what is your opinion about these 2 pending points ?

[codecov]

Codecov Report

Attention: Patch coverage is 45.88235% with 46 lines in your changes are missing coverage. Please review.

Project coverage is 69.53%. Comparing base (fdaf0ac) to head (5ab6300).
Report is 4 commits behind head on main.

Files	Patch %	Lines
bin/sozo/src/ops/migration/mod.rs	29.78%	33 Missing ⚠️
bin/sozo/src/commands/dev.rs	0.00%	6 Missing ⚠️
crates/dojo-world/src/manifest.rs	77.77%	4 Missing ⚠️
crates/dojo-world/src/migration/mod.rs	78.57%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1651      +/-   ##
==========================================
- Coverage   69.55%   69.53%   -0.02%     
==========================================
  Files         266      266              
  Lines       27213    27267      +54     
==========================================
+ Hits        18927    18960      +33     
- Misses       8286     8307      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lambda-0x]

Thank you for your contribution!

To answer few of your questions:

Currently, the deployment manifest file is created by merging the base, overlay and remote manifest files, replacing the old deployment manifest file.

we merge base with overlay to get the manifest that requires to be deployed. remote manifest is used calculate WorldDiff which represents what we actually need to deploy.

also merge the old deployment manifest file with the other manifests as a starting point of the migration to avoid losing transaction_hash and block_number of not updated contracts.

yeah i think that makes sense to me, wdyt @tarrencev

stored the seed used to create the world. It looks like the seed is just the name of the world, and the name is already stored. So, did I miss something for this point ?

what we used to called seed is now called name in sozo migrate subcommand, by default its the name of project in Scarb.toml but user can specify custom name (seed) as well

[lambda-0x]

@remybar also we need transaction_hash and block_number only for world contract so you can remove it for other contracts

[glihm]

@remybar very good points, confirmed by @lambda-0x.

The world is the one that is very relevant because Torii needs a starting block to initiate the indexing. Having the block_number and the transaction_hash for the world ensures that Torii can fetch any data to be configured for a particular world.

Having both is somehow redundant, but having the block number ready to be used has the benefit of avoiding additional queries to the network.

For the contracts, we still have all the event system indexed by Torii which covers most of the use cases for now. Due to that, let's reduce the scope to the world only, as mentioned by @lambda-0x.

[remybar]

Thank you for your contribution!

To answer few of your questions:

Currently, the deployment manifest file is created by merging the base, overlay and remote manifest files, replacing the old deployment manifest file.

we merge base with overlay to get the manifest that requires to be deployed. remote manifest is used calculate WorldDiff which represents what we actually need to deploy.

also merge the old deployment manifest file with the other manifests as a starting point of the migration to avoid losing transaction_hash and block_number of not updated contracts.

yeah i think that makes sense to me, wdyt @tarrencev

stored the seed used to create the world. It looks like the seed is just the name of the world, and the name is already stored. So, did I miss something for this point ?

what we used to called seed is now called name in sozo migrate subcommand, by default its the name of project in Scarb.toml but user can specify custom name (seed) as well

Thank you @lambda-0x and @glihm for these details !

I have modified this PR to handle transaction_hash and block_number for the world contract only.
I've also added a merge_from_previous method to DeployedManifest to be able to retrieve transaction_hash and block_number from an old deployment manifest.
I don't know if the name is good, but I wanted to avoid writing a global merge method that would have merged all the fields from DeployedManifest when we are only interested in 2 fields of the world contract at the moment.

Now, I have to deep more on the seed/name topic because I don't understand why the World contract is not migrated if we change its name (which is used as salt to generate its contract address).

[glihm]

Awesome work man.

Now, I have to deep more on the seed/name topic because I don't understand why the World contract is not migrated if we change its name (which is used as salt to generate its contract address).

Not sure I get you here. If you run sozo migrate --name world2 it seems the world is correctly migrated. And in your branch, the manifest is updated with new addresses. However the name is not updated in the deployment manifest (were you referring to that?).

[lambda-0x]

I think seed related changes require some discussion which might take some time, so we can merge this PR without those changes and tackle it in a different PR to unblock #1652. wdyt @glihm?

[glihm]

I think seed related changes require some discussion which might take some time, so we can merge this PR without those changes and tackle it in a different PR to unblock #1652. wdyt @glihm?

Yep sounds good, I'll open an issue to follow up on that and discuss around the seed.

@remybar sounds good to you or you were planning to add more before the seed stuff?

[glihm]

Thanks @remybar for the work done here, let's move to the next step, @lambda-0x stage is yours!