feat: add parachain_system.authorize_upgrade call hash to output#49
Conversation
scripts/analyse.sh
Outdated
| tmsp: $tmsp, | ||
| size: $size, | ||
| prop: $prop, | ||
| set_code_prop: $set_code_prop, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
templates/output.txt
Outdated
| system.setCode proposal : {{ runtimes.compact.set_code_prop }} | ||
| parachainSystem.authorizeUpgrade proposal : {{ runtimes.compact.authorize_upgrade_prop }} |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Would this work then? d16e7a9
Or did you mean to only make the authorizeUpgrade row longer (which would look bad, in my opinion).
There was a problem hiding this comment.
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.
|
@chevdor can I have the green light to merge? |
2 participants
As the title implies, adds the information about the call that parachains should execute for a runtime upgrade.