Skip to content
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

Re-design the matching_transaction logic for Ethereum #2039

Merged
merged 10 commits into from Feb 24, 2020
Merged

Conversation

@tcharding
Copy link
Contributor

tcharding commented Feb 17, 2020

Currently the logic for matching transactions is super generic. We would like an API that accepts only what is needed to match a transaction and returns what we need from the matched transaction. From the COMIT protocol level i.e., rfc003 this is a 'per action' API i.e., funded/deployed/redeem/refunded.

The API is now

matching_create_contract(...) -> Result<(Transaction, Address)>
matching_event(...) -> Result<(Transaction, Log)>

All the matching logic is contained within the btsieve/ethereum module.

Makes a start at resolving: #1781, Bitcoin side to do.

Issue #2083 tracks the Bitcoin side.

@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Feb 17, 2020

Are you sure the changelog does not need updating?

@tcharding

This comment has been minimized.

Copy link
Contributor Author

tcharding commented Feb 17, 2020

Please do 10% review @thomaseizinger

Copy link
Member

thomaseizinger left a comment

Interesting idea, I think you are on a good track!

Make sure you resolve the issue with the receipt before continuing, to validate that the design actually works.

Side-note:
What to look for correlates with the individual protocols. This is why we have HtlcDeployed etc.
Adding another layer that also uses the deployed terminology seems unnecessary.
It would be nice if the next layer, i.e. the implementation of HtlcDeployed would use a more generic layer that no longer knows about those steps.

That is sort of what we have now, it is just bad.
Maybe you can build something out of what you have here that exposes more primitive but useful matching logic like:

  • matching_contract_deployed_transactions
  • matching_event_emitted_transactions
  • ...?
@@ -194,7 +232,7 @@ mod tests {
let transaction = r#"02000000014135047eff77c95bce4955f630bc3e334690d31517176dbc23e9345493c48ecf000000004847304402200da78118d6970bca6f152a6ca81fa8c4dde856680eb6564edb329ce1808207c402203b3b4890dd203cc4c9361bbbeb7ebce70110d4b07f411208b2540b10373755ba01feffffff02644024180100000017a9142464790f3a3fddb132691fac9fd02549cdc09ff48700a3e1110000000017a914c40a2c4fd9dcad5e1694a41ca46d337eb59369d78765000000
"#.to_owned();

let bytes = decode_response::<bitcoin::Transaction>(transaction);

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 17, 2020

Member

Unrelated to the 10% review:

IMO, you are removing the most important 7 characters from this line: bitcoin.

Knowing, which transaction is decoded here is massively important because you will be able to immediately narrow down further possible paths/areas in your brain of what to think about.
Having to scroll to the top of the tests block to do that is a huge burden IMO.

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 17, 2020

Author Contributor

Will leave the paths in.

Ok(transaction)
}

pub async fn matching_funded_transaction<'b, C>(

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 17, 2020

Member

Bear in mind that funding for erc20 is different than for ether :)

The erc20 one needs to look for a Transfer event where as the ether one only needs to look for a transaction.

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 17, 2020

Author Contributor

Yep, I'm across that, will however keep it mind. Thanks

match matching_transaction(blockchain_connector, start_of_swap, |_transaction| {
// TODO: Add logic for matching a redeemed transaction.
//
// Includes using the bloom filter to conditionally fetch receipts

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 17, 2020

Member

This is a very important!

How are you going to solve the issue of needing the receipt for refunded and redeemed?
In order for the underlying code to know that it has to fetch the receipt, you will kind of have to give it the event you are looking for.

The crucial code is here:

block
.logs_bloom
.contains_input(Input::Raw(topic.0.as_ref()))

Maybe it is actually enough to pass down the topic to matching_transactions? The underlying code can then filter out all blocks where the bloom filter doesn't match and call you for those that does match with a tuple of (Transaction, Receipt).

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 17, 2020

Author Contributor

Cheers, I won't be 100% sure until I flesh it out some more but I think the design can accommodate the bloom filter correctly - time will tell.

@tcharding

This comment has been minimized.

Copy link
Contributor Author

tcharding commented Feb 17, 2020

Interesting idea, I think you are on a good track!

Make sure you resolve the issue with the receipt before continuing, to validate that the design actually works.

Side-note:
What to look for correlates with the individual protocols. This is why we have HtlcDeployed etc.
Adding another layer that also uses the deployed terminology seems unnecessary.
It would be nice if the next layer, i.e. the implementation of HtlcDeployed would use a more generic layer that no longer knows about those steps.

That is sort of what we have now, it is just bad.
Maybe you can build something out of what you have here that exposes more primitive but useful matching logic like:

  • matching_contract_deployed_transactions
  • matching_event_emitted_transactions
  • ...?

This is a nice idea, I might keep working with the functions I've got, make sure they work then see if I can generalise like you suggest. I'd like the final functions to be defined by what they accept and what they return and also not have conditions within the matching logic that require domain knowledge to understand i.e., the matching is explicit in the function names. Will keep this in mind, thanks man. Good review.

@tcharding tcharding force-pushed the 1781-transaction-patterns branch 7 times, most recently from 56089b2 to 2d1b618 Feb 17, 2020
@tcharding

This comment has been minimized.

Copy link
Contributor Author

tcharding commented Feb 18, 2020

Ready for 50% review please @thomaseizinger, @luckysori, @D4nte

Copy link
Member

thomaseizinger left a comment

Good work!

I've left a slightly more detailed review with some concrete improvement suggestions.

+ ReceiptByHash<Receipt = Option<TransactionReceipt>, TransactionHash = Hash>
+ Clone,
{
let events = vec![Event {

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 18, 2020

Member

Design challenge question: Are we ever looking for more than one event?

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 18, 2020

Author Contributor

Nice question, I thought I answered this to myself but I've mixed up Topic's and Events, we look for multiple Topic's but not multiple Events - will refactor.

+ ReceiptByHash<Receipt = Option<TransactionReceipt>, TransactionHash = Hash>
+ Clone,
{
matching_transaction(blockchain_connector, start_of_swap, |block| {

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 18, 2020

Member

Design challenge question: Why isn't matching_transaction iterating over the tx and handing them to the closure?

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 18, 2020

Author Contributor

Your observation is correct, matching_transaction could iterate over the txs but matching_transaciton_receipt cannot do this because it needs access to the block. I'm inclined to keep the two functions inline and leave the iteration in the closure.

blockchain_connector: C,
start_of_swap: NaiveDateTime,
events: Vec<Event>,
action: &'static str,

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 18, 2020

Member

Is this only for logging?

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 18, 2020

Author Contributor

Yes, open to a cleaner idea. I did not put that much thought into it.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 18, 2020

Member

I think this is the kind of problem that tracing is trying to solve. If you open a new span at the call-site that contains this action as a context, every message inside will be scoped to this span and hence doesn't need to include the action name.


if !block
.logs_bloom
.contains_input(Input::Raw(events[0].topics[0].unwrap().0.as_ref()))

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 18, 2020

Member

Looks like you always just assume one element anyway, I'd remove the Vec then.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 18, 2020

Member

Would also greatly simplify events_exist_in_receipt the method below.

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 18, 2020

Author Contributor

Have not even touched events_exist_in_receipt yet. Now the 50% review is in I'll do some more refactoring, thanks for the reminder.

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 18, 2020

Author Contributor

Misunderstood you at first here; refactored events_exist_in_receipt to accept a single Event. Did no other refactoring - I believe that's what you meant :)

}
}

async fn matching_transaction_receipt<C, F>(

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 18, 2020

Member
Suggested change
async fn matching_transaction_receipt<C, F>(
async fn matching_transaction_and_receipt<C, F>(

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 18, 2020

Author Contributor

cool

};
let events = vec![Event {
address: Some(htlc_deployment.location),
data: None,

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 18, 2020

Member

Do we ever use data?
Maybe you can remove the Event struct altogether and only pass the address and a list of topics to the function?

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 18, 2020

Author Contributor

I played with this a little and the conclusion I came to was we were trying to imitate web3 with the whole Event struct. Is this assumption correct?

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 18, 2020

Author Contributor

Answering my own question; on further investigation we can remove this, update the comment that refers to web3 and still maintain clarity while reducing complexity - WIN. Thanks

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 18, 2020

Member

I don't think we need to imitate something. The original design might have been inspired by that but the original design was also trying to be general purpose (which turned out to be a really bad idea 😅).

Question everything my friend :)

+ ReceiptByHash<Receipt = Option<TransactionReceipt>, TransactionHash = Hash>
+ Clone,
{
matching_transaction_receipt(blockchain_connector.clone(), start_of_swap, |block| {

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 18, 2020

Member

You could go ahead and pass the topics already to matching_transaction_receipt. This would allow you to do the bloom filter matching inside and call the closure with a tuple of transaction and receipt.

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 18, 2020

Author Contributor

Nice idea, will consider.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 18, 2020

Member

It would also invalidate the assumption here: #2039 (comment)

@tcharding

This comment has been minimized.

Copy link
Contributor Author

tcharding commented Feb 18, 2020

Thanks @thomaseizinger, super helpful review.

@tcharding tcharding force-pushed the 1781-transaction-patterns branch from 2d1b618 to be1d47f Feb 19, 2020
@tcharding

This comment has been minimized.

Copy link
Contributor Author

tcharding commented Feb 19, 2020

Can I get 90% review please, all that is left to do is integration testing.

Any reason why this is not a good idea please: I'm planning on adding a Mock connector and testing the new Ethereum matching transaction API using that and hopefully the test ethereum data already in cnd/tests/test_data. EDIT: I see now this is already done for the bitcoin tests :)
Fresh eyes EDIT: Some serious muppetry going on here by me, there was already an ethereum mock connector in the tests before I deleted them :(

Copy link
Member

thomaseizinger left a comment

Nice man :)

This is starting to look really great. I am excited!

Can I get 90% review please, all that is left to do is integration testing (yes I am omitting unit tests, long gone are the days of TDD :(

I've highlighted two main things:

  1. Moving the check for successful TX one layer down (into the matching_transaction_* function). Seems like the better option because then matchers can't forget to do it.
  2. Moving the fetching of the receipt one layer down as-well. This would make all the matchers non-async.

There are some optimizations we can do with this now. Previously, we always just returned the transaction and receipt from the matcher but actually, depending on what we are looking for, we want different things to be returned:

  • create-contract: tx + contract address
  • match event: tx + event data (to extract the secret)

This would remove some potential error cases in code that is called after these matching functions.

// We can get a transaction that matches even for failed attempts
// at contract creation e.g., low gas.
let receipt = fetch_receipt(blockchain_connector, transaction.hash).await?;
if receipt.status == Some(TRANSACTION_STATUS_OK.into()) {

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 19, 2020

Member

Oh, nice one! I didn't think of that!

This got me thinking:
We should probably never return any transaction that failed right?

So the whole check should probably move into matching_transaction and run after the matcher returned true. This will make this matcher non-async again :)
We probably then want to log a big warning that says:

"We found the transaction we were looking for but the execution failed!"

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 19, 2020

Member

This would also reduce some code duplication between the matchers.

let transaction =
matching_create_contract_transaction(connector, start_of_swap, htlc_params.bytecode())
.await?;
let location = calculate_contract_address_from_deployment_transaction(&transaction);

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 19, 2020

Member

The contract address is actually in the receipt!

matching_create_contract_transaction could return us the location from the receipt, then we would not need to calculate it and could throw away a whole bunch of code (and some dependencies!) :)

let blockchain_connector = blockchain_connector.clone();
let event = event.clone();
async move {
let receipt = fetch_receipt(blockchain_connector, transaction.hash).await?;

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 19, 2020

Member

Moving this one layer down would make the matcher non-async (and hence a lot more concise).

@tcharding

This comment has been minimized.

Copy link
Contributor Author

tcharding commented Feb 19, 2020

@thomaseizinger, implemented your recent suggestions - this is getting seriously sexy. Force pushed, check out the last two patches! (CI will fail due to integration tests, working on that now.)

@tcharding tcharding force-pushed the 1781-transaction-patterns branch from be1d47f to 4b77076 Feb 19, 2020
Copy link
Member

thomaseizinger left a comment

Did a slightly more thorough review, this is looking really good :)

"transaction matched {:x} but status was NOT OK",
transaction.hash,
);
// A failed transaction could not be in the same block as

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

I am not sure that is true. The EVM is turing complete so miners can't know if two transactions to the same contract will conflict or not. Hence I think they might very well include two transactions to the same contract in the same block.

I think this is a dangerous optimisation. I'd just continue matching, it should be fairly cheap :)

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 20, 2020

Author Contributor

Cool, thanks. Fixed as suggested.

#[derive(Clone, Default, Eq, PartialEq, serde::Serialize, serdebug::SerDebug)]
pub struct Event {
#[serde(skip_serializing_if = "Option::is_none")]
pub address: Option<Address>,

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

I think you should be able to remove the Option. We should always specify the source address of the event.

block_hash,
);
for transaction in block.transactions.into_iter() {
if let Some((transaction, receipt)) = matcher(transaction.clone()).await? {

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

We are in an async fn here already + we know the bloom filter hinted us that the event happend in the block.
You also have the tx id here, hence you could fetch the receipt and pass it to the matcher.
That would make matcher synchronous: Fn(Tx, Receipt) -> bool.
It is equally efficient because currently, the matcher just fetches the receipt.

}

fn event_exists_in_receipt(event: &Event, receipt: &TransactionReceipt) -> bool {
match event {

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

I think it would be the right time to refactor this method to use defensive programming:

let { topics, address } = event;

if topics.is_empty() {
   return false;
}

// etc ...

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 20, 2020

Author Contributor

Oh, you think that is nicer? Interesting, that's how I had it for a lot time I was working on this. I refactored it to use the 'if guard' within the match. Unless I'm missing the meaning of your comment?

    match event {
        Event { topics, .. } if topics.is_empty() => false,

Is the same as :

let { topics, address } = event;

if topics.is_empty() {
   return false;
}

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

Yes it is the same functionally :)
But the last time I did that, you were like: "that is just using the language features for the sake of it! :D"

And then you suggested a defensive programming approach which is also quite neat.
I am not sure what is better in this case honestly. The defensive one maps clearer to the algorithm that one would have in their mind I guess 🤷‍♂

Up to you - no strong feelings on this part :)

if let Some((transaction, receipt)) = matcher(transaction.clone()).await? {
// We can get a transaction that matches even for failed attempts
// at contract creation e.g., low gas.
if receipt.status != Some(TRANSACTION_STATUS_OK.into()) {

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

We are doing this check in two different places. Might be worth defining a trait for it an implementing it on TransactionReceipt.

.context("failed to find transaction to redeem from htlc")?;
let (transaction, receipt) =
matching_event_transaction(connector, start_of_swap, event, "redeemed").await?;

let log = receipt

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

As you can see, we iterate over all the logs and try to find the one again that we passed to matching_event_transaction.

We can do a similar optimization as with the contract address and return the event already from the matching_event_transaction function. That would save us the, in practice, unreachable error path of trying to find the one log again.

.await
.context("failed to find transaction to redeem from htlc")?;
let (transaction, receipt) =
matching_event_transaction(connector, start_of_swap, event, "redeemed").await?;

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

With the below optimization of returning the log right away, a better name for this could be matching_event?

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 20, 2020

Author Contributor

Changed API functions to

matching_create_contract() -> (transaction, location)
matching_event() -> (transaction, log)

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

Nice, looks good :)

matching_transaction_bloom_filter(
blockchain_connector.clone(),
start_of_swap,
event.topics.clone(),

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

With the matcher being non-async, this could turn into a reference.

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 20, 2020

Author Contributor

Had a go but the event being used in the closure as well as here means we still need to clone.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

Only if the closure moves it right? If it is not async, there shouldn't be a need to move it. My brain borrow checker is weak though, so could very easily be that I am wrong :D

+ Clone,
{
matching_transaction_bloom_filter(
blockchain_connector.clone(),

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

Similarly, with the matcher being non-async, you should be able to avoid the clone because you don't need the connector out here.

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 20, 2020

Author Contributor

This ones right, rustc caught this one. On a separate note, I would be interested in going through the code base and trying to remove clones at some stage when we are playing around - bit early for optimisations I know but this would be a good way to learn more about Rust life times, which is something I'd like to learn more about.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

Cloning things per se is not bad, even though the Rust community is religious about it 😅

What is important is thinking about what you are cloning here. The connector is not data, it is functionality that we are passing around. Maybe we should make that explicit and always take an Arc<C> to make it totally clear that cloning it is:

a) cheap
b) not harmful in any way

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 24, 2020

Author Contributor

@tcharding do you plan on doing the bitcoin side also in this PR?
If no, please open an issue for it so we can track the remaining work!

I have not reached a decision yet, I was planning on finishing the Ethereum side then once I know exactly whats involved looking into the Bitcoin code and seeing if we should do this now or if we should make the Bitcoin code use Generators etc (all the improvements that have been done only on the Ethereum side). Open to suggestions, will definitely create an issue if we go that way.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 24, 2020

Member

There is already a ticket for adopting the use of generators (#1878). Happy to do that one first and then follow-up with adopting the model proposed in this PR for the Bitcoin side.

Copy link
Member

thomaseizinger left a comment

Please don't change the testing strategy of btsieve.

#[tokio::test]
async fn ethereum_transaction_pattern_e2e_test() {

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 20, 2020

Member

I don't agree with repurposing this test.

The original idea was:

  • have fast (i.e. non-IO) integration tests of the whole btsieve module by using a connector mock and real data (that's the tests you deleted for now). they verify things like missing blocks, etc but are unconcerned about the actual matching (they don't need to as it is orthogonal)
  • have unit tests for the actual matching logic (with the new design, that might be obsolete but I guess it wouldn't hurt if we have some basic tests for edge-case logic). You can pass function pointers instead of closures and unit test the functions you are pointing to.
  • have a single, slow e2e test that verifies that we glued things together correctly (this test). Note that this test should be as simple as possible and not use complicated contracts like HTLCs or something.

All these three topics are orthogonal and together form a good test coverage without introducing unnecessary dependencies and levels of detail.

The added value of these tests is very little because we have the e2e tests anyway and they cover the same thing (they also use actual HTLCs).
Yet, they are super heavy and slow.

What we need to test at this level of the code are edgecases that are not covered by the e2e tests:

  • missing blocks
  • go back in the past
  • etc

but those are missing with your testing strategy.
I'd recommend to revert this and just bring the other tests back.

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 21, 2020

Author Contributor

Ok, I think I've got you. I've pushed a couple of commits that add skeleton functions. I'm going to go ahead and work on these, if you feel like taking a look at the direction I'm taking please do so. I do not think that the added integration test (using parity) adds much although it is still included because its done and working already, can probably be removed before this PR merges. Thanks

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 21, 2020

Author Contributor

In case you saw the message above mid-edit, I had this message sitting open for an hour and during which I wrote the skeleton test functions.

@tcharding tcharding force-pushed the 1781-transaction-patterns branch from d81d9ce to 9d21291 Feb 21, 2020
@thomaseizinger

This comment has been minimized.

Copy link
Member

thomaseizinger commented Feb 23, 2020

@tcharding do you plan on doing the bitcoin side also in this PR?
If no, please open an issue for it so we can track the remaining work!

Currently the logic for matching transactions is super generic.  We
would like an API that accepts only what is needed to match a
transaction and returns what we need from the matched transaction.  From
the COMIT protocol level i.e., rfc003 this is a 'per action' API i.e.,
funded/deployed/redeem/refunded.

The API is now

    matching_create_contract(...) -> Result<Transaction, Address>
    matching_events(...) -> Result<Transaction, Log>

Re-write ethereum integration tests. Use
`matching_transaction_and_receipt` to test the new matching logic.
@tcharding tcharding force-pushed the 1781-transaction-patterns branch from 9d21291 to e35a6a9 Feb 24, 2020
@tcharding tcharding marked this pull request as ready for review Feb 24, 2020
@tcharding tcharding changed the title WIP: Re-design the matching_transaction logic Re-design the matching_transaction logic Feb 24, 2020
@tcharding tcharding requested a review from thomaseizinger Feb 24, 2020
Copy link
Member

thomaseizinger left a comment

Good work!

@@ -59,21 +59,11 @@ uuid = { version = "0.8", features = ["serde", "v4"] }
void = "1.0.2"
warp = { version = "0.2", default-features = false }

# These versions need to be "in sync".
# web3 0.8 gives us primitive-types 0.3.0
# primitive-types 0.3.0 with the "rlp" feature gives us "rlp" version 0.4.2
[dependencies.web3]

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 24, 2020

Member

web3 was only declared separately so I could add this comment. Feel free to declare it next to all the other dependencies now!

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 24, 2020

Author Contributor

Cool, fixed. Thanks

@@ -24,16 +20,119 @@ use std::collections::HashSet;
type Hash = H256;
type Block = crate::ethereum::Block<Transaction>;

pub async fn matching_transaction<C>(
pub const TRANSACTION_STATUS_OK: u32 = 1;

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 24, 2020

Member

This is now unused because the trait defines it's own const.

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 24, 2020

Author Contributor

Nice, removed.


Ok(None)
/// Event works similar to web3 filters:

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 24, 2020

Member

Should update or delete these docs.

Also it would be nice to not ignore them if we keep them but just fix the compile errors in there.

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 24, 2020

Author Contributor

Had a play with this, did not remove the 'ignore'. Seems to me that adding all the type stuff to satisfy the compiler just makes the comment more cluttered with no real added advantage (since we are only parsing static strings into Rust types).

impl Predates for Block {
fn predates(&self, timestamp: NaiveDateTime) -> bool {
let unix_timestamp = timestamp.timestamp();
trait TransactionStatusOk {

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 24, 2020

Member

This could actually live in the general ethereum module.

It would also be nice if the function naming would give away that the fn returns a bool. Something like:

fn is_status_ok() -> bool {

}

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 24, 2020

Author Contributor

Fixed as suggested.

@tcharding tcharding requested review from luckysori and D4nte Feb 24, 2020
tcharding added 5 commits Feb 24, 2020
Instead of passing an action string add a tracing span
Currently we define and use helper functions for redeem/refund actions
for Ether and Erc20.  This is not strictly correct since it relies on
the fact that the log messages are the same.  This will change with the
protocol split Han/Herc20.  Lets fix this now to save time in the
future.
Rename the predicate function from `transaction_status_ok` to
`is_status_ok` and put the trait/impl in `src/ethereum`.
@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Feb 24, 2020

Are you sure the changelog does not need updating?

@thomaseizinger thomaseizinger changed the title Re-design the matching_transaction logic Re-design the matching_transaction logic for Ethereum Feb 24, 2020
Copy link
Contributor

luckysori left a comment

This is SO much better. Thank you, Tobin.

}
}

pub async fn matching_transaction_and_receipt<C, F>(

This comment has been minimized.

Copy link
@luckysori

luckysori Feb 24, 2020

Contributor

Is this public just so that we can test it?

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 24, 2020

Author Contributor

Yes, funny you should ask that ...

GeneratorState::Complete(Err(e)) => return Err(e),
// By matching against the never type explicitly, we assert that the `Ok` value of the
// result is actually the never type and has not been changed since this
// line was written. The never type can never be constructed, so we cannot

This comment has been minimized.

Copy link
@luckysori

luckysori Feb 24, 2020

Contributor

🔧

Suggested change
// line was written. The never type can never be constructed, so we cannot
// line was written. The never type can never be constructed, so we can
// By matching against the never type explicitly, we assert that the `Ok` value of the
// result is actually the never type and has not been changed since this
// line was written. The never type can never be constructed, so we cannot
// reach this line never anyway.

This comment has been minimized.

Copy link
@luckysori

luckysori Feb 24, 2020

Contributor

🔧

Suggested change
// reach this line never anyway.
// never reach this line anyway.
@@ -24,16 +20,148 @@ use std::collections::HashSet;
type Hash = H256;
type Block = crate::ethereum::Block<Transaction>;

pub async fn matching_transaction<C>(
pub async fn matching_create_contract<C>(

This comment has been minimized.

Copy link
@luckysori

luckysori Feb 24, 2020

Contributor

🤔 Probably for a follow-up PR, but what do you think about renaming this function to look_for_contract_creation and doing something similar for the other ones? I find the term matching so confusing because it's both and adjective and a verb.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 24, 2020

Member

I am onboard with this! Alternative ways could also be:

  • wait_for_contract_creation
  • watch_for_contract_created

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 24, 2020

Author Contributor

Well spotted @lucas, choosing 'watching' as a nice verb for what we are doing.

This comment has been minimized.

Copy link
@tcharding

tcharding Feb 24, 2020

Author Contributor

Renamed:

  • matching_create_contract -> watch_for_contract_creation
  • matching_event -> watch_for_event

But that leaves the functions

  • matching_transaction_and_log
  • matching_transaction_and_receipt

Also the matcher predicate function will need to be renamed to 'match' the functions (pun intended :)

Any ideas? Otherwise I'll have another think tomorrow and submit follow up PR.

This comment has been minimized.

Copy link
@thomaseizinger

thomaseizinger Feb 24, 2020

Member

I vote for follow-up PR!

Let's get this in! 🎉

tcharding added 4 commits Feb 24, 2020
'matching' is a verb and an adjective, this is bad for function names.
Lets use 'watching' instead, that's what btsieve is doing.
@mergify

This comment has been minimized.

Copy link
Contributor

mergify bot commented Feb 24, 2020

bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Feb 24, 2020

@bors bors bot merged commit dab291a into dev Feb 24, 2020
5 checks passed
5 checks passed
automation
Details
Summary 2 rules match and 1 potential rule
Details
bors Build succeeded
Details
ci/circleci: debug-build-test Your tests passed on CircleCI!
Details
license/cla All CLA requirements met.
@mergify mergify bot deleted the 1781-transaction-patterns branch Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.