substrate proposals #275
substrate proposals #275
Conversation
There was a problem hiding this comment.
Great work @salman01zp this is a good first go. There are some issues I've commented on but to recap.
We need to track proposal nonces for each resource being updated. For the token wrapper, the resources are the asset metadata (feeRecipient and rescueTokens). Therefore, I think we do still want a map that either maps from resource id or asset id to the nonce that tracks the updates incident on a particular asset. Let me know what you think.
In the Solidity we do have separate setups for each asset, and therefore separate feeRecipients and rescue tokens and proposal nonces for each different deployment.
Also don't forget to add tests of all extrinsics created here and update the benchmarks if possible.
| @@ -29,7 +29,7 @@ use frame_support::traits::Currency; | |||
| use frame_system::RawOrigin; | |||
| use sp_runtime::traits::Bounded; | |||
| use webb_primitives::types::DepositDetails; | |||
| fn assert_last_event<T: Config<I>, I: 'static>(generic_event: <T as Config<I>>::Event) { | |||
| fn assert_last_event<T: Config<I>, I: 'static>(generic_event: <T as Config<I>>::RuntimeEvent) { | |||
There was a problem hiding this comment.
We have this function written 5 or so times, in the future @salman01zp please break it out and de-duplicate the code. Similar to what we are doing with webb.js/test-utils we should be doing something similar here. Creating a test-utils package that eventually lives and gets used by various substrate/rust projects (here it would be Substrate specific for example).
Okay, so when we update FeeRecipient we update it for the particular Asset. |
|
Yep, I think that provides the greatest flexibility for asset applications
on top.
…On Tue, Nov 8, 2022 at 9:41 PM Salman Pathan ***@***.***> wrote:
Great work @salman01zp <https://github.com/salman01zp> this is a good
first go. There are some issues I've commented on but to recap.
We need to track proposal nonces for each resource being updated. For the
token wrapper, the resources are the asset metadata (feeRecipient and
rescueTokens). Therefore, I think we do still want a map that either maps
from resource id or asset id to the nonce that tracks the updates incident
on a particular asset. Let me know what you think.
In the Solidity we do have separate setups for each asset, and therefore
separate feeRecipients and rescue tokens and proposal nonces for each
different deployment.
Also don't forget to add tests of all extrinsics created here and update
the benchmarks if possible.
Okay, so when we update FeeRecipient we update it for the particular Asset.
We can create a separate mapping of (AssetId ,FeeRecipient) and will
revert back ProposalNonce to mapping
—
Reply to this email directly, view it on GitHub
<#275 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADELLF735YOPM7TSLS4TSQDWHMTO7ANCNFSM6AAAAAAR2AS3GE>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Codecov ReportBase: 50.42% // Head: 50.14% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #275 +/- ##
==========================================
- Coverage 50.42% 50.14% -0.28%
==========================================
Files 55 55
Lines 2013 2064 +51
==========================================
+ Hits 1015 1035 +20
- Misses 998 1029 +31
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Summary of changes
Reference issue to close (if applicable)