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

Add basic transaction data layout #55

Merged
merged 1 commit into from Jul 2, 2019

Conversation

Geod24
Copy link
Contributor

@Geod24 Geod24 commented Jul 2, 2019

As simple as that

@Geod24 Geod24 added the type-feature label Jul 2, 2019

*******************************************************************************/

public alias Amount = long;
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic Jul 2, 2019

Choose a reason for hiding this comment

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

Bitcoin seems to allow negative values (at least internally?), but I'm not sure why. Do you know?

Copy link
Contributor Author

@Geod24 Geod24 Jul 2, 2019

Choose a reason for hiding this comment

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

Not exactly. I imagine that might be related to dealing with a - b (remember Don hating on unsigned types ? ;) )

Copy link
Contributor

@AndrejMitrovic AndrejMitrovic Jul 2, 2019

Choose a reason for hiding this comment

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

Ah that's a very good point. I remember the implicit ulong => uint conversion in D1 as well. That was a fun deployment.

{
/// The monetary value of this output, in 1/10^7
Amount value;
/// The public key that can redeem this output (A = pubkey)
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic Jul 2, 2019

Choose a reason for hiding this comment

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

(A = pubkey) - what? where does "A" come from?

Copy link
Contributor Author

@Geod24 Geod24 Jul 2, 2019

Choose a reason for hiding this comment

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

Oups shall remove

Represents an entry in the UTXO

This is created by a valid `Transaction` and is added to the UTXO after
a transaction is confirmed.
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic Jul 2, 2019

Choose a reason for hiding this comment

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

What does it mean that a transaction is confirmed, in this context? It should be documented.

Copy link
Contributor Author

@Geod24 Geod24 Jul 2, 2019

Choose a reason for hiding this comment

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

I think that's pretty standard Bitcoin jargon.

Copy link
Contributor

@AndrejMitrovic AndrejMitrovic Jul 2, 2019

Choose a reason for hiding this comment

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

I still don't understand though, what does it mean in this context? I know about the "6 confirmations rule", but is that related to this at all?

Copy link
Contributor Author

@Geod24 Geod24 Jul 2, 2019

Choose a reason for hiding this comment

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

Confirmed == Included in a block

Copy link
Contributor

@AndrejMitrovic AndrejMitrovic Jul 2, 2019

Choose a reason for hiding this comment

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

got it!

public struct Input
{
/// The hash of a previous transaction containing the `Output` to spend
Hash previous;
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic Jul 2, 2019

Choose a reason for hiding this comment

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

So w.r.t. style guide, do we use explicit protection attributes? Or just make it implied that things are public?

Copy link
Contributor Author

@Geod24 Geod24 Jul 2, 2019

Choose a reason for hiding this comment

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

We do. Oups.

}

/// The input of the transaction, which spends a previously received `Output`
public struct Input
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic Jul 2, 2019

Choose a reason for hiding this comment

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

Mmmhm.. I dislike that you've ordered them as:

public struct Output { ... }
public struct Input { ... }

Can we make them Input and then Output instead? I know I'm nitpicking.

Copy link
Contributor Author

@Geod24 Geod24 Jul 2, 2019

Choose a reason for hiding this comment

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

The reasoning is that, while your approach is the natural approach, Input is actually defined in terms of Output, not the other way around. An Output never references an Input, but an Input always reference an Input. Output can exists without Input (Coinbase) but not the other way around.
It sounds backwards but that's the way it's done in BTC and I can't think of something better.

Copy link
Contributor Author

@Geod24 Geod24 Jul 2, 2019

Choose a reason for hiding this comment

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

That being said, I don't mind changing the ordering at all, just wanted to provide a reasoning for the ordering.

Copy link
Contributor

@AndrejMitrovic AndrejMitrovic Jul 2, 2019

Choose a reason for hiding this comment

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

Yeah that's fair.

/// The hash of a previous transaction containing the `Output` to spend
Hash previous;
/// Index of the `Output` in the `previous` `Transaction`
uint index;
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic Jul 2, 2019

Choose a reason for hiding this comment

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

uint index

I think in the future we should probably make this type smaller.

Copy link
Contributor Author

@Geod24 Geod24 Jul 2, 2019

Choose a reason for hiding this comment

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

Agreed

@AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Jul 2, 2019

Done with review.

source/agora/common/Transaction.d Outdated Show resolved Hide resolved
@Geod24
Copy link
Contributor Author

@Geod24 Geod24 commented Jul 2, 2019

Updated according to review. Also added protection attributes.

@AndrejMitrovic
Copy link
Contributor

@AndrejMitrovic AndrejMitrovic commented Jul 2, 2019

@TrustHenry / @MukeunKim, do you want to review this?

@AndrejMitrovic AndrejMitrovic merged commit 1e5a391 into bosagora:v0.x.x Jul 2, 2019
1 check passed
@Geod24 Geod24 deleted the transactions branch Jul 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants