Fix trackId=>Origin conversion, support submit for root track #2084
Conversation
4meta5
added
A0-pleasereview
precompiles/referenda/src/lib.rs
Outdated
| @@ -278,6 +275,21 @@ where | |||
| Ok(()) | |||
| } | |||
|
|
|||
| // Helper function for submit precompile functions | |||
| fn track_id_to_origin(track_id: u16) -> EvmResult<Box<OriginOf<Runtime>>> { | |||
| let origin: OriginOf<Runtime> = if track_id == 0 { | |||
There was a problem hiding this comment.
How do we know track_id 0 is root ? That seems a very dangerous shortcut.
There was a problem hiding this comment.
I have not seen an OpenGov config with trackId 0 not corresponding to root origin. Moreover because it is a special origin, there is no other way to express this mapping.
There was a problem hiding this comment.
I'll add an integrity test to the runtimes to ensure trackId 0 is root but this is the best we can do.
There was a problem hiding this comment.
I think there should be a way to do that properly. It is too prone to error if one day we change the tracks
There was a problem hiding this comment.
@girazoki or @librelois to suggest some ideas ?
There was a problem hiding this comment.
Root is not part of the OpenGov origins, so whatever you do you have to map it manually somewhere.
To be sure of the match at compile time, we can create a public constant ROOT_TRACK_ID in a common crate and use it in runtime/.../tracks.rs and in the precompiles
There was a problem hiding this comment.
Why not make this part of the From<GovOrigin> instead of hardcoding? I will take a closer look at whether we can do something more clever, but at least this should probably be part of the runtime impl
There was a problem hiding this comment.
Additionally we could add a check that the origin matches the track through track_for inside the precompile
There was a problem hiding this comment.
Additionally we could add a check that the origin matches the track through
track_forinside the precompile
I like this idea of using Tracks::tracks_for so I'm doing this now and will ping once ready for review.
There was a problem hiding this comment.
@girazoki @librelois I did a significant refactor to use Runtime::Tracks::tracks() in the precompile and runtime. This removes the TryFrom<u16> impl for CustomOrigin and adds a FromStr impl for CustomOrigin.
Also added an integration test to ensure all trackIds in TRACKS_DATA either correspond to the root origin or a custom origin.
4meta5
added
A3-inprogress
| 2 => Ok(Origin::GeneralAdmin), | ||
| 3 => Ok(Origin::ReferendumCanceller), | ||
| 4 => Ok(Origin::ReferendumKiller), | ||
| fn try_from(value: String) -> Result<Origin, ()> { |
There was a problem hiding this comment.
I suggest to use strum derive macro EnumString to autogenerate this implementation:
|
4meta5
added
A0-pleasereview
4meta5
changed the title
Fixes referenda::submit for root track. We were previously relying on the
TryFrom<TrackId = u16>impl for custom origins, but this did not support the root origin/track.We replace
TryFrom<TrackId = u16>with a derivedFromStrimpl and use this in conjunction with Runtime::Tracks::tracks() to get the TrackId => Origin mapping.If Runtime::Tracks::tracks() grows big enough, we will change it back to use a TryFrom impl.