Use abci-tag proc_macro for event attributes #276

DaviRain-Su · 2022-12-01T16:04:06Z

Closes: #214

Description

This is my implementation of the abci_tag proco marco implementation of the feeling that there is still a toy Everyone see what other improvements, this implementation is to solve the problems raised in this issue.

PR author checklist:

Added changelog entry, using unclog.
Added tests.
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).
Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

codecov · 2022-12-07T03:20:20Z

Codecov Report

Base: 65.31% // Head: 65.09% // Decreases project coverage by -0.21% ⚠️

Coverage data is based on head (89822fe) compared to base (3464923).
Patch coverage: 54.54% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #276      +/-   ##
==========================================
- Coverage   65.31%   65.09%   -0.22%     
==========================================
  Files         125      127       +2     
  Lines       13210    13868     +658     
==========================================
+ Hits         8628     9028     +400     
- Misses       4582     4840     +258

Impacted Files	Coverage Δ
...s/ibc/src/clients/ics07_tendermint/host_helpers.rs	`0.00% <ø> (ø)`
crates/ibc/src/core/handler.rs	`0.00% <0.00%> (ø)`
...rc/core/ics04_channel/events/channel_attributes.rs	`40.00% <0.00%> (+22.85%)`	⬆️
...src/core/ics04_channel/events/packet_attributes.rs	`25.00% <0.00%> (+3.75%)`	⬆️
...rc/core/ics03_connection/handler/conn_open_init.rs	`57.84% <15.68%> (-42.16%)`	⬇️
crates/ibc/src/mock/context.rs	`70.41% <23.35%> (-5.52%)`	⬇️
crates/ibc/src/core/context.rs	`33.33% <54.54%> (+23.75%)`	⬆️
...core/ics03_connection/handler/conn_open_confirm.rs	`77.50% <65.64%> (-22.50%)`	⬇️
...src/core/ics03_connection/handler/conn_open_ack.rs	`78.05% <66.87%> (-14.51%)`	⬇️
...src/core/ics03_connection/handler/conn_open_try.rs	`80.99% <71.03%> (-14.93%)`	⬇️
... and 4 more

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

plafer

Thank you for this!

Could you also implement the change for

the structs found in core::ics02_client::events
ModuleEventAttribute in crate::events

plafer · 2022-12-09T20:56:39Z

crates/ibc/abci-tag/Cargo.toml

@@ -0,0 +1,14 @@
+[package]
+name = "abci-tag-proc-marco"


Suggested change

name = "abci-tag-proc-marco"

name = "abci-event-attr-key-proc-macro"

plafer · 2022-12-09T20:57:12Z

crates/ibc/abci-tag/src/lib.rs

+use syn::{parse_macro_input, DataStruct, DeriveInput, Ident as SynIdent};
+
+#[proc_macro_attribute]
+pub fn abci_tag(args: TokenStream, item: TokenStream) -> TokenStream {


Suggested change

pub fn abci_tag(args: TokenStream, item: TokenStream) -> TokenStream {

pub fn abci_event_attr_key(args: TokenStream, item: TokenStream) -> TokenStream {

Could please change the name? I find this more descriptive.

plafer · 2022-12-09T21:13:04Z

crates/ibc/abci-tag/src/lib.rs

+            EventAttribute {
+              key: #args.parse().unwrap(),
+              value: attr.#field_name.as_str().parse().unwrap(),
+              index: false,
+            }


This is the old way to do it. Can you have the return value be as it was, e.g.

impl From<PortIdAttribute> for abci::EventAttribute { fn from(attr: PortIdAttribute) -> Self { (PORT_ID_ATTRIBUTE_KEY, attr.port_id.as_str()).into() } }

romac · 2022-12-12T11:34:11Z

May I ask what problem this is solving and what's the scope for this? Is it only used to derive the From implementation? To me it sounds a bit overkill to introduce a whole new crate and a proc macro to save on 4 lines of code per event that won't change substantially in the future.

romac · 2022-12-12T11:38:54Z

How we really want to save on those 4 lines, how about using a declarative macro instead? eg.

event_attr!(PortIdAttribute, PORTID_ATTR_KEY, port_id);

plafer · 2022-12-12T12:37:19Z

I agree... I even prefer no declarative macro at all since it's just clearer what is being implemented for the struct.

Should we just close this PR and the issue? @romac @hu55a1n1 WDYT?

hu55a1n1 · 2022-12-12T12:44:55Z

What I liked about the proc v/s declarative macro approach was that you didn't have to specify anything other than the attr key const, but I agree this is overkill as there are more additions in the PR than there are deletions (+104 −87). 👍
I think this was a good experiment but I am also in favour of closing the PR and issue.

plafer · 2022-12-12T12:57:01Z

Thank you @DaviRain-Su for this. Unfortunately we'll close it for the reasons discussed.

DaviRain-Su · 2022-12-12T12:57:33Z

Thank you @DaviRain-Su for this. Unfortunately we'll close it for the reasons discussed.

Understand

DaviRain-Su added 2 commits December 2, 2022 00:03

Use abci-tag proc_macro for event attributes

03f70b3

Merge branch 'main' into 214-proc-marco-event-attr

adac1ee

Create 214-proc-macro-attr-for-event-attributes.md

89822fe

DaviRain-Su marked this pull request as ready for review December 8, 2022 01:49

plafer suggested changes Dec 9, 2022

View reviewed changes

plafer closed this Dec 12, 2022

plafer mentioned this pull request Dec 12, 2022

Proc macro attr for event attributes #214

Closed

5 tasks

DaviRain-Su deleted the 214-proc-marco-event-attr branch January 11, 2023 03:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use abci-tag proc_macro for event attributes #276

Use abci-tag proc_macro for event attributes #276

DaviRain-Su commented Dec 1, 2022 •

edited

codecov bot commented Dec 7, 2022 •

edited

plafer left a comment

plafer Dec 9, 2022

plafer Dec 9, 2022

plafer Dec 9, 2022

romac commented Dec 12, 2022

romac commented Dec 12, 2022

plafer commented Dec 12, 2022

hu55a1n1 commented Dec 12, 2022 •

edited

plafer commented Dec 12, 2022

DaviRain-Su commented Dec 12, 2022

	name = "abci-tag-proc-marco"
	name = "abci-event-attr-key-proc-macro"

	pub fn abci_tag(args: TokenStream, item: TokenStream) -> TokenStream {
	pub fn abci_event_attr_key(args: TokenStream, item: TokenStream) -> TokenStream {

Use abci-tag proc_macro for event attributes #276

Use abci-tag proc_macro for event attributes #276

Conversation

DaviRain-Su commented Dec 1, 2022 • edited

Description

PR author checklist:

Reviewer checklist:

codecov bot commented Dec 7, 2022 • edited

Codecov Report

plafer left a comment

Choose a reason for hiding this comment

plafer Dec 9, 2022

Choose a reason for hiding this comment

plafer Dec 9, 2022

Choose a reason for hiding this comment

plafer Dec 9, 2022

Choose a reason for hiding this comment

romac commented Dec 12, 2022

romac commented Dec 12, 2022

plafer commented Dec 12, 2022

hu55a1n1 commented Dec 12, 2022 • edited

plafer commented Dec 12, 2022

DaviRain-Su commented Dec 12, 2022

DaviRain-Su commented Dec 1, 2022 •

edited

codecov bot commented Dec 7, 2022 •

edited

hu55a1n1 commented Dec 12, 2022 •

edited