-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce a burn address for cdk_erigon
#463
Conversation
@LindaGuiga If you merge latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mostly good with it, but left some comments to discuss / address.
I'm good with this once rebased + revert the changes linked to macro -> global label post #492 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to feature-gate fields in a public struct.
I support features when they add to the public API in ways that don't require code changes from users.
This is fine
pub fn foo(p: &str) {
#[cfg(feature = "str")] // implementation detail
...
}
This is not
#[cfg(feature ="str")]
pub fn foo(p: &str);
#[cfg(feature ="bytes")]
pub fn foo(p: &[u8]);
As you've added it, the feature is viral - now trace_decoder
needs conditional compilation too, but I don't think it should.
Can you think of a way of phrasing the public API such that it doesn't need conditional compilation? Perhaps an enum that's always present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm broadly good with this, mostly light comments. @0xaatif could you tell us if you're satisfied with how the decoder looks now, following your latest discussions with Linda.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TODO
s are a blocker, and the inversion of control is a strong suggestion.
@0xaatif will you review the latest changes following resolved discussions or defer to other code owners? (what your latest comment seems to imply) |
This PR closes #92.
This PR introduce a burn address
BurnAddr
when thecdk_erigon
feature is activated. TheBurnAddr
receives the burnt fees when the feature is activated. This is achieved similarly to the fee payment for the sender: first, we add the maximum fee to the burn address, then we deduct the fee corresponding to the remaining gas.For now, the default value (0x0) is set in the trace decoder.