search

paritytech / substrate

Substrate: The platform for blockchain innovators
Apache License 2.0
8.39k stars 2.65k forks source link

Contracts: Update Config::Debug #14789

Closed pgherveou closed 1 year ago

pgherveou commented 1 year ago

Update the Debug trait that was behind the unsafe-debug feature flag, and get rid of the feature flag

Most config should set it to () that will simply log contract calls when runtime::contracts log target is set to trace.

More advanced debugging tools can leverage this trait to introduce more sophisticated observability behaviors. See previous PR (#14678) for more context.

Cumulus companion: https://github.com/paritytech/cumulus/pull/3035

athei commented 1 year ago

I don't think this is a proper replacement for the Debug trait which was specifically designed to bundle all future debugging infrastructure without cluttering the Config trait. Hence the generic name and the sub typing.

pgherveou commented 1 year ago

I don't think this is a proper replacement for the Debug trait which was specifically designed to bundle all future debugging infrastructure without cluttering the Config trait. Hence the generic name and the sub typing.

@athei Is that just a name issue? I can rename it back to Debug if we think its more appropriate.

@pmikolajczyk41 my goal here was to provide the same features that was previously exposed, but provide an implementation that does not add any overhead to the code.

athei commented 1 year ago

@athei Is that just a name issue? I can rename it back to Debug if we think its more appropriate.

This was not a rename but a removal. The Debug trait before was just an empty trait to bundle together all the different debugging traits we might want to add. Just as a shortcut name we can mention on the Config.

@pmikolajczyk41 I can understand that you want to make the implementations of those traits as simple as possible since you are the one implementing it. But I think you can agree that shifting complexity from on-chain code to a dev tool is the sensible thing to do. Which is exactly what we do by asking the implementor to manage state instead of the pallet.

Regarding conditional compilation: In this case it is viable but as soon as the arguments of the functions require computation we will start causing overhead.

pgherveou commented 1 year ago

@athei moved back everything under the Debugging umbrella trait, and used back T::Debug associated trait name.

pgherveou commented 1 year ago

@oleg-plakida you mind giving us a +1 here for removing the unsafe-debug feature added in the previous PR #14678

pgherveou commented 1 year ago

@oleg-plakida you mind giving us a +1 here for removing the unsafe-debug feature added in the previous PR #14678

cc @rcny

pgherveou commented 1 year ago

bot merge

paritytech-processbot[bot] commented 1 year ago

Error: Statuses failed for 986fe1097b50967bae539ec4e1cc36819c1041ee

pgherveou commented 1 year ago

bot merge