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

Implement transaction verification #60

Merged
merged 1 commit into from Jul 8, 2019
Merged

Implement transaction verification #60

merged 1 commit into from Jul 8, 2019

Conversation

MichaelKim20
Copy link
Member

@MichaelKim20 MichaelKim20 commented Jul 3, 2019

Contains tests for transaction verification

@MichaelKim20 MichaelKim20 changed the title Create test of transaction verification Implement test of transaction verification Jul 3, 2019
@AndrejMitrovic
Copy link
Contributor

AndrejMitrovic commented Jul 3, 2019

When @Geod24 mentioned that we should have code and tests together, what he meant was that they should be made at the same time and be submitted in the same pull request.

But the verification code itself should live in another module, not in a unittest one. Because the verification code will be used both in tests and within the node itself.

Does that make sense?

@MichaelKim20
Copy link
Member Author

MichaelKim20 commented Jul 4, 2019

When @Geod24 mentioned that we should have code and tests together, what he meant was that they should be made at the same time and be submitted in the same pull request.

But the verification code itself should live in another module, not in a unittest one. Because the verification code will be used both in tests and within the node itself.

Does that make sense?

Yeah, I got it.

@MichaelKim20
Copy link
Member Author

MichaelKim20 commented Jul 4, 2019

Added class TransactionManager.
This class has the ability to create and store transactions.
It can also verify transactions.
When @Geod24 improves the hash function, I will use that hash function.
It will be connected to the recipient of the transaction that @AndrejMitrovic is working on. So, I will add the transaction so that it can be verified at the receiving end of the transaction.

@AndrejMitrovic
Copy link
Contributor

AndrejMitrovic commented Jul 5, 2019

It will be connected to the recipient of the transaction that @AndrejMitrovic is working on. So, I will add the transaction so that it can be verified at the receiving end of the transaction.

I think this part can be added in a separate pull request later, so your work is not blocked on my work.

So the only blocker right now for both of us is hashing.

@MichaelKim20
Copy link
Member Author

MichaelKim20 commented Jul 5, 2019

It will be connected to the recipient of the transaction that @AndrejMitrovic is working on. So, I will add the transaction so that it can be verified at the receiving end of the transaction.

I think this part can be added in a separate pull request later, so your work is not blocked on my work.

So the only blocker right now for both of us is hashing.

We need to find 'Output' in 'Block' using 'Input' ('previous' and 'index').
This should also be done later after the two PR's are Merge. What do you think?

@MichaelKim20 MichaelKim20 changed the title Implement test of transaction verification Implement transaction verification Jul 5, 2019
Copy link
Collaborator

@Geod24 Geod24 left a comment

First round of review done

source/agora/common/TransactionManager.d Outdated Show resolved Hide resolved
source/agora/common/TransactionManager.d Outdated Show resolved Hide resolved
source/agora/common/TransactionManager.d Outdated Show resolved Hide resolved
source/agora/common/TransactionManager.d Outdated Show resolved Hide resolved
source/agora/common/TransactionManager.d Outdated Show resolved Hide resolved
source/agora/common/TransactionManager.d Outdated Show resolved Hide resolved
source/agora/common/TransactionManager.d Outdated Show resolved Hide resolved
source/agora/common/TransactionManager.d Outdated Show resolved Hide resolved
source/agora/common/TransactionManager.d Outdated Show resolved Hide resolved
source/agora/common/TransactionManager.d Outdated Show resolved Hide resolved
@MichaelKim20
Copy link
Member Author

MichaelKim20 commented Jul 6, 2019

First round of review done

@Geod24 All the requested modifications have been completed.

@Geod24
Copy link
Collaborator

Geod24 commented Jul 6, 2019

Great work !

One point though, if a function does not use any member of a class, and its usage is not specific to a class, it should not be part of a class. Other parts of the program might need to get the sum of the input. They should not have to have an instance of a TransactionManager to do so.

For example, imagine I want to print the sum of all transactions in a debug logging statement in the network code.

Using https://github.com/bpfkorea/agora/blob/740d54a677a209dabcb8fbd1893c8f2950089b37/source/agora/node/Network.d#L300-L306 as a base

    public void sendMessage (Transaction tx) @safe
    {
        scope tm = new TransactionManager();
        foreach (ref node; this.peers)
        {
            logDebug("Sending a transaction with %d input and %d output", tm.getInputSum(tx), tm.getOutputSum(tx));
            node.sendMessage(tx);
        }
    }

Instead, if you make it a free function:

public long getOutputSum (Transaction tx)
{
    return tx.outputs.map!(a => a.value).sum();
}

I can call it like this:

    public void sendMessage (Transaction tx) @safe
    {
        foreach (ref node; this.peers)
        {
            logDebug("Sending a transaction with %d input and %d output", tx.getInputSum(), tx.getOutputSum());
            node.sendMessage(tx);
        }
    }

Which I think is much clearer. It does not have to be a member of Transaction because we can use UFCS, as you already did with map and sum.

@AndrejMitrovic
Copy link
Contributor

AndrejMitrovic commented Jul 8, 2019

I agree with @Geod24's comments in #60 (comment).

Copy link
Collaborator

@Geod24 Geod24 left a comment

Pretty good, a few comments, mostly style

source/agora/common/TransactionManager.d Outdated Show resolved Hide resolved
source/agora/common/TransactionManager.d Outdated Show resolved Hide resolved
source/agora/common/TransactionManager.d Outdated Show resolved Hide resolved
source/agora/common/Transaction.d Show resolved Hide resolved
source/agora/common/Transaction.d Outdated Show resolved Hide resolved
source/agora/common/Transaction.d Outdated Show resolved Hide resolved
source/agora/common/Transaction.d Outdated Show resolved Hide resolved
source/agora/common/Transaction.d Outdated Show resolved Hide resolved
source/agora/common/Transaction.d Outdated Show resolved Hide resolved
source/agora/test/Transaction.d Outdated Show resolved Hide resolved
Contains class for transaction verification
Geod24
Geod24 approved these changes Jul 8, 2019
@MichaelKim20
Copy link
Member Author

MichaelKim20 commented Jul 8, 2019

@Geod24 @AndrejMitrovic
I have organized it into a single file(source/agora/common/Transaction.d).

@Geod24 Geod24 merged commit 602118d into bosagora:v0.x.x Jul 8, 2019
@Geod24 Geod24 added the type-feature An addition to the system introducing new functionalities label Jul 8, 2019
@MichaelKim20 MichaelKim20 deleted the mk08 branch Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature An addition to the system introducing new functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants