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

Byzcoin roster changes #42

Merged
merged 11 commits into from
May 5, 2020
Merged

Byzcoin roster changes #42

merged 11 commits into from
May 5, 2020

Conversation

Gilthoniel
Copy link
Contributor

@Gilthoniel Gilthoniel commented May 1, 2020

This PR refactors the transactions in Byzcoin to allow multiple kinds to take advantage of the polymorphism. It then partially implements the roster transactions.

Coming: Byzcoin unit tests and full roster change support.

@Gilthoniel Gilthoniel self-assigned this May 1, 2020
It implements the roster change with only the support of removals
and without any access rights control.
@Gilthoniel Gilthoniel requested a review from nkcr May 4, 2020 13:36
@Gilthoniel Gilthoniel added the R4M 🚀 The PR is ready to be reviewed and merged label May 4, 2020
@Gilthoniel
Copy link
Contributor Author

Gilthoniel commented May 4, 2020

@nkcr The PR seems huge because I split the transaction abstraction and the task abstraction (equivalent to an instruction in cothority) so a lot of code moving around. Most of the new code is the task implementations for DARC, contracts and the roster.

Copy link
Contributor

@nkcr nkcr left a comment

Choose a reason for hiding this comment

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

looks really nice 👍
It seems that some structs miss their - implements ... comment. I didn't mark them all.

require.Equal(t, tx.GetID(), instance.GetKey())
require.Equal(t, tx.GetID(), instance.GetArcID())
require.IsType(t, (*darc.AccessControlProto)(nil), instance.GetValue())
require.Equal(t, 19, roster.Len())
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's important to comment that kind of expected result in the test, as it can greatly help understanding the system.

// Fingerprinter is an interface to perform fingerprinting on object.
type Fingerprinter interface {
// Fingerprint writes itself to the writer in a deterministic way.
Fingerprint(io.Writer, ProtoMarshaler) error
Copy link
Contributor

Choose a reason for hiding this comment

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

Fingerprint: maybe add it to the terminologies? (as we already have footprint, that would be good)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point I forgot to change footprint to fingerprint which seems to be more used.

Comment on lines 517 to 520
if c == nil {
return true
}
return c.Value <= 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if c == nil {
return true
}
return c.Value <= 0
return c == nil || c.Value <= 0

map<string, Expression> rules = 1;
}

message Task {
Copy link
Contributor

Choose a reason for hiding this comment

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

A new terminology ?

)

// clientTask is the client task of a transaction that will allow an authorized
// identity to create or update a DARC.
Copy link
Contributor

Choose a reason for hiding this comment

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

- implements ...

return nil
}

// serverTask is the server task for a DARC transaction.
Copy link
Contributor

Choose a reason for hiding this comment

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

- implements ...

return nil
}

type taskFactory struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

- implements ... ?

latest, err := ldgr.bc.GetBlock()
if err != nil {
return nil, xerrors.Errorf("couldn't read latest block: %v", err)
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

why removing those error wrappings?

Copy link
Contributor Author

@Gilthoniel Gilthoniel May 5, 2020

Choose a reason for hiding this comment

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

Oh that's because I removed the GetInstance which does not really makes sense anymore and I usually write the errors alongside the tests but the tests will come in the next PR (complete rewrite of the function).

"golang.org/x/xerrors"
)

// taskFactory is an task factory that can process several types of tasks.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// taskFactory is an task factory that can process several types of tasks.
// taskFactory is a task factory that can process several types of tasks.

@@ -12,7 +12,8 @@ type Page interface {
// the integrity.
GetFootprint() []byte

// Read returns the value stored at the given key.
// Read returns the value stored at the given key. If the key does not
// exist, it should a nil value without error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// exist, it should a nil value without error.
// exist, it should return a nil value without error.

@nkcr nkcr merged commit 12b1c9f into master May 5, 2020
@Gilthoniel Gilthoniel deleted the roster_change_tx branch May 28, 2020 08:00
bbjubjub2494 pushed a commit to bbjubjub2494/dela that referenced this pull request Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R4M 🚀 The PR is ready to be reviewed and merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants