Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Trace filtering fix #2760

Merged
merged 4 commits into from
Oct 20, 2016
Merged

Trace filtering fix #2760

merged 4 commits into from
Oct 20, 2016

Conversation

ngotchac
Copy link
Contributor

Fixes #2751

Don't test contract address against empty toAddress array in trace filtering.

  Don't test contract address against empty array in trace filtering
@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Oct 20, 2016
@@ -127,7 +132,7 @@ impl Filter {
};

action || match trace.result {
Res::Create(ref create) => self.to_address.matches(&create.address),
Res::Create(ref create) => self.to_address.stricly_matches(&create.address),
Copy link
Collaborator

@tomusdrw tomusdrw Oct 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we replace L124 with self.to_address.matches(&create.address) and remove this ||? (strictly_matches would not be required at all)

Copy link
Contributor Author

@ngotchac ngotchac Oct 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that would be totally equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However the address field is in the result part of the trace, not in the action

@tomusdrw tomusdrw added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 20, 2016
@debris debris added the A8-looksgood 🦄 Pull request is reviewed well. label Oct 20, 2016
self.matches_all() || self.list.contains(address)
}
self.matches_all() || self.stricly_matches(address)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually the issue here is that this whole line is using spaces instead of tabs for indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep just saw that

}

/// Returns true if address matches at least one of the searched addresses.
pub fn stricly_matches(&self, address: &Address) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strictly*

Copy link
Contributor Author

@ngotchac ngotchac Oct 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arh...

@rphmeier rphmeier removed the A8-looksgood 🦄 Pull request is reviewed well. label Oct 20, 2016
@rphmeier rphmeier added A8-looksgood 🦄 Pull request is reviewed well. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Oct 20, 2016
@ngotchac ngotchac added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Oct 20, 2016
@ngotchac ngotchac added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Oct 20, 2016
Action::Call(ref call) => {
let from_matches = self.from_address.matches(&call.from);
let to_matches = self.to_address.matches(&call.to);
from_matches && to_matches
}
Action::Create(ref create) => {
let from_matches = self.from_address.matches(&create.from);
let to_matches = self.to_address.matches_all();

let to_matches = match trace.result {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, now that I saw it's coming from result I'm not sure if it's equivalent.
@debris is it possible that trace.action is Action::Call but the result is Res::Create somehow?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. To be honest, there should be just one enum. Having two is just a performance workaround to avoid unnecessary copying data when trace is created.

@tomusdrw tomusdrw added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 20, 2016
@gavofyork gavofyork merged commit 236fb82 into master Oct 20, 2016
@gavofyork gavofyork deleted the ng-trace-filter-fix branch October 20, 2016 21:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants