Skip to content

feat: add parachain_system.authorize_upgrade call hash to output #49

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.

Sign up for GitHub

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

Merged
chevdor merged 5 commits into from
Sep 26, 2022
Merged

feat: add parachain_system.authorize_upgrade call hash to output #49

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
16e5862
Add parachain authorize upgrade proposal hash to output
ntn-x2 Sep 12, 2022
1bfd1e6
Revert PROP renaming
ntn-x2 Sep 12, 2022
d16e7a9
Reduce whitespaces in the template
ntn-x2 Sep 12, 2022
a2449cd
Fix copy-paste issue
ntn-x2 Sep 12, 2022
bf1546f
Remove 'proposal' from template
ntn-x2 Sep 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions scripts/analyse.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ WASM=$1
WASM_FULLPATH=/build/$WASM

SZ=`du -sb $WASM_FULLPATH | awk '{print $1}'`
PROP=`subwasm -j info $WASM_FULLPATH | jq -r .proposal_hash`
SET_CODE_PROP=`subwasm -j info $Z_WASM | jq -r .proposal_hash`
AUTHORIZE_UPGRADE_PROP=`subwasm -j info $Z_WASM | jq -r .parachain_authorize_upgrade_hash`
MULTIHASH=`subwasm -j info $WASM_FULLPATH | jq -r .ipfs_hash`
SHA256=0x`shasum -a 256 $WASM_FULLPATH | awk '{print $1}'`
TMSP=$(date --utc +%FT%TZ -d @$(stat -c "%Y" $WASM_FULLPATH))
Expand All @@ -14,7 +15,8 @@ SUBWASM=`subwasm -j info $WASM_FULLPATH`
JSON=$( jq -n \
--arg tmsp "$TMSP" \
--arg size "$SZ" \
--arg prop "$PROP" \
--arg set_code_prop "$SET_CODE_PROP" \
--arg authorize_upgrade_prop "$AUTHORIZE_UPGRADE_PROP" \
--arg blake2_256 "$BLAKE2_256" \
--arg ipfs "$MULTIHASH" \
--arg sha256 "$SHA256" \
Expand All @@ -23,7 +25,8 @@ JSON=$( jq -n \
'{
tmsp: $tmsp,
size: $size,
prop: $prop,
set_code_prop: $set_code_prop,
Copy link
Contributor

@chevdor chevdor Sep 12, 2022

Choose a reason for hiding this comment

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

I would prefer to keep the legacy prop to avoid breaking all the CI depending on srtool and its output.
If you think the new set_code_prop (way clearer definitely) is a must, we could add it as well and drop a comment that prop is now deprecated and will be removed in the future.

Ideally we'd line up the PR for the removal of prop already now but merge only later and make sure it comes with a big BREAKING warning.

Copy link
Contributor Author

@ntn-x2 ntn-x2 Sep 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Ok I would say let's stick with having the new line in. I reverted the change -> 1bfd1e6
Also noticed there was a copy-paste issue between the analyse and the build scripts ^^ -> a2449cd

Copy link
Contributor

@chevdor chevdor Sep 12, 2022

Choose a reason for hiding this comment

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

We could keep your renaming of PROP into SET_CODE_PROP, this is nice IMO.
Same around all you introduced, the naming is nicer, I would then only need prop to be still availble for now.

Copy link
Contributor Author

@ntn-x2 ntn-x2 Sep 13, 2022

Choose a reason for hiding this comment

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

Mhh yeah, that's true. But there would be a repeated line which I am not sure is a nice thing to have. We, at KILT, do of course delete lines that are not relevant for new releases, but do you think it would still be good to have two different properties that mean the same thing?

Copy link
Contributor

@chevdor chevdor Sep 13, 2022

Choose a reason for hiding this comment

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

I don't like duplication but breaking compat. is not an option. If we do break compat. it would be good to wait for a change or a set of changes that are worth it.

I do like your renaming but I think it may not be worth the "troubles" in the end.

The duplication would not remain on the long run but the more I think about it and the more I prefer to keep the legacy name even if this is less accurate with your change.

Copy link
Contributor Author

@ntn-x2 ntn-x2 Sep 13, 2022

Choose a reason for hiding this comment

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

Yes, I agree on rather leaving the old name, and maybe make a ticket (or whatever you use to track things) to rename it in the next breaking change.

authorize_upgrade_prop: $authorize_upgrade_prop,
blake2_256: $blake2_256,
ipfs: $ipfs,
sha256: $sha256,
Expand Down
9 changes: 6 additions & 3 deletions scripts/build
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@ SZ=`du -sb $Z_WASM | awk '{print $1}'`
KB=$(expr $SZ / 1024)
TMSP=`date --utc +%FT%TZ`
SHA256=0x`shasum -a 256 $Z_WASM | awk '{print $1}'`
PROP=`subwasm -j info $Z_WASM | jq -r .proposal_hash`
SET_CODE_PROP=`subwasm -j info $Z_WASM | jq -r .proposal_hash`
AUTHORIZE_UPGRADE_PROP=`subwasm -j info $Z_WASM | jq -r .parachain_authorize_upgrade_hash`
MULTIHASH=`subwasm -j info $Z_WASM | jq -r .ipfs_hash`

# If we work on a snapshot, we will NOT get a .git folder to work on
Expand Down Expand Up @@ -178,7 +179,8 @@ JSON=$( jq -n \
--arg bytes "$SZ" \
--arg wasm "$WASM" \
--arg z_wasm "$Z_WASM" \
--arg prop "$PROP" \
--arg set_code_prop "$SET_CODE_PROP" \
--arg authorize_upgrade_prop "$AUTHORIZE_UPGRADE_PROP" \
--arg sha256 "$SHA256" \
--arg tmsp "$TMSP" \
--arg git_tag "$GIT_TAG" \
Expand All @@ -201,7 +203,8 @@ JSON=$( jq -n \
pkg: $pkg,
tmsp: $tmsp,
size: $bytes,
prop: $prop,
set_code_prop: $set_code_prop,
authorize_upgrade_prop: $authorize_upgrade_prop,
ipfs: $ipfs,
sha256: $sha256,
wasm: $z_wasm,
Expand Down
32 changes: 17 additions & 15 deletions templates/output.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,27 @@ Summary generated with {{ gen }} using the docker image {{ context.docker.image
Time : {{ tmsp }}

== Compact
Version : {{ runtimes.compact.subwasm.core_version }}
Metadata : V{{ runtimes.compact.subwasm.metadata_version }}
Size : {{ runtimes.compact.size | int | filesizeformat }} ({{ runtimes.compact.size }} bytes)
Proposal : {{ runtimes.compact.prop }}
IPFS : {{ runtimes.compact.ipfs }}
BLAKE2_256 : {{ runtimes.compact.blake2_256 }}
Wasm : {{ runtimes.compact.wasm }}
Version : {{ runtimes.compact.subwasm.core_version }}
Metadata : V{{ runtimes.compact.subwasm.metadata_version }}
Size : {{ runtimes.compact.size | int | filesizeformat }} ({{ runtimes.compact.size }} bytes)
system.setCode proposal : {{ runtimes.compact.set_code_prop }}
parachainSystem.authorizeUpgrade proposal : {{ runtimes.compact.authorize_upgrade_prop }}
Copy link
Contributor

@chevdor chevdor Sep 12, 2022

Choose a reason for hiding this comment

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

The extra length here will make usage uncomfortable in some places.
We could use only:

  • setCode (I don't think there is another than the one in system)
  • authorizeUpgrade (that is already longer than the rest)

Copy link
Contributor Author

@ntn-x2 ntn-x2 Sep 12, 2022

Choose a reason for hiding this comment

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

Would this work then? d16e7a9
Or did you mean to only make the authorizeUpgrade row longer (which would look bad, in my opinion).

Copy link
Contributor

@chevdor chevdor Sep 12, 2022

Choose a reason for hiding this comment

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

As you did in d16e7a9 just without the proposal word. Those are hashes and not proposals (they happen to be the identifiers of the proposals indeed). I also removed hashes previously to keep the summary table concise.

Copy link
Contributor Author

@ntn-x2 ntn-x2 Sep 13, 2022

Choose a reason for hiding this comment

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

Sounds good! bf1546f

IPFS : {{ runtimes.compact.ipfs }}
BLAKE2_256 : {{ runtimes.compact.blake2_256 }}
Wasm : {{ runtimes.compact.wasm }}

== Compressed
{%- if runtimes.compressed %}
{%- set ratio = 100 - (( runtimes.compressed.subwasm.compression.size_compressed / runtimes.compressed.subwasm.compression.size_decompressed) ) * 100 %}
Version : {{ runtimes.compressed.subwasm.core_version }}
Metadata : V{{ runtimes.compressed.subwasm.metadata_version }}
Size : {{ runtimes.compressed.size | int | filesizeformat }} ({{ runtimes.compressed.size }} bytes)
Compression : {{ ratio | round(method="ceil", precision=2) }}%
Proposal : {{ runtimes.compressed.prop }}
IPFS : {{ runtimes.compressed.ipfs }}
BLAKE2_256 : {{ runtimes.compressed.blake2_256 }}
Wasm : {{ runtimes.compressed.wasm }}
Version : {{ runtimes.compressed.subwasm.core_version }}
Metadata : V{{ runtimes.compressed.subwasm.metadata_version }}
Size : {{ runtimes.compressed.size | int | filesizeformat }} ({{ runtimes.compressed.size }} bytes)
Compression : {{ ratio | round(method="ceil", precision=2) }}%
system.setCode proposal : {{ runtimes.compressed.set_code_prop }}
parachainSystem.authorizeUpgrade proposal : {{ runtimes.compressed.authorize_upgrade_prop }}
IPFS : {{ runtimes.compressed.ipfs }}
BLAKE2_256 : {{ runtimes.compressed.blake2_256 }}
Wasm : {{ runtimes.compressed.wasm }}
{% else %}
No compressed runtime found
{% endif %}