feat: api events

[Daanvdplas]

Added events to contract library and pallet, including tests for the pallet.

Note: the weight for increase allowance has changed due to an extra storage read.

The events in the contract library will have to be demonstrated in an example contract. This will have to be done in a different PR.

[evilrobot-01]

Hmmm, not sure about the id removal from the events. See inline comments for more context.

[evilrobot-01]

Sadly I think this is problematic. I understand it was probably removed to conform to the standard, but does ommitting it effectively make it useless? The events are emitted from a pallet managing multiple assets, so without knowing which asset id had an approval, is there any point? A contract call might be able to correlate the resulting events based on the extrinsic events, but if contract makes two calls to the fungibles pallet to create an allowance, for two different assets, how can we tell from the events which value matches which asset?

[Daanvdplas]

But perhaps this brings us back to this comment.

In short, reason for adding events to the fungible pallet:

• standard compliance - the problem is that the events are not useful though (see frank's comment). In addition, when we do add the asset_id the events become the same as in pallet assets (e.g. Transferred)
• pallet usage - we have an indicator how often the pallet is used. More interesting is how often the API is used

Would it be oke to start without events in the pallet?

[peterwht]

Would it be oke to start without events in the pallet?

I would rather we start with something, even super simple events so we can be aware of if the API is being used on testnet.

[Daanvdplas]

Alright I will then add the asset id back in the Transfer and Approval event

[evilrobot-01]

A general heads up that all these PRs to the api branch do not seem to include any test coverage stats, meaning that there might still be some work to do before the branch is merged to main. I will take a look at that branch first to see why.

[evilrobot-01]

A general heads up that all these PRs to the api branch do not seem to include any test coverage stats, meaning that there might still be some work to do before the branch is merged to main. I will take a look at that branch first to see why.

FYI - the daan/api branch is missing a number of commits from main, with the ci one perhaps the most important. I suggest getting this addressed soon so that you can get a feel for how much code is untested before assuming that the initial api work is done.

[Daanvdplas]

FYI - the daan/api branch is missing a number of commits from main, with the ci one perhaps the most important. I suggest getting this addressed soon so that you can get a feel for how much code is untested before assuming that the initial api work is done.

On it right away

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 93.77778% with 14 lines in your changes missing coverage. Please review.

Project coverage is 33.62%. Comparing base (545bd33) to head (3b3ae9f).
Report is 1 commits behind head on daan/api.

Files	Patch %	Lines
pallets/api/src/fungibles/mod.rs	85.56%	1 Missing and 13 partials ⚠️

@@             Coverage Diff              @@
##           daan/api     #153      +/-   ##
============================================
+ Coverage     32.45%   33.62%   +1.16%     
============================================
  Files            33       34       +1     
  Lines          2915     2995      +80     
  Branches       2915     2995      +80     
============================================
+ Hits            946     1007      +61     
- Misses         1946     1956      +10     
- Partials         23       32       +9     

Files	Coverage Δ
pallets/api/src/fungibles/tests.rs	100.00% <100.00%> (ø)
pallets/api/src/mock.rs	100.00% <ø> (ø)
runtime/devnet/src/config/api.rs	0.00% <ø> (ø)
pallets/api/src/fungibles/mod.rs	82.46% <85.56%> (-0.19%)	⬇️

... and 7 files with indirect coverage changes

[peterwht]

LGTM! Left a small comment, good with whatever you decide.

[evilrobot-01]

Just one more optimisation and a variable rename for consistency. Peter has a good point in terms of emitting events to determine usage, so hopefully a quick add.

[peterwht]

[sc-1124]

[evilrobot-01]

One last minor refactor then good for me.