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

batch service #1046

Closed
wants to merge 7 commits into from
Closed

batch service #1046

wants to merge 7 commits into from

Conversation

acud
Copy link
Member

@acud acud commented Dec 11, 2020

2 in 1 for better flexibility

@acud acud added the in progress ongoing development , hold out with review label Dec 11, 2020
// event BatchCreated(bytes32 indexed batchId, uint256 initialBalance, address owner, uint256 depth);
type batchCreatedEvent struct {
batchID [32]byte
balance *big.Int
Copy link
Member

Choose a reason for hiding this comment

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

balance -> value

// event BatchTopUp(bytes32 indexed batchId, uint256 topupAmount);
type batchTopUpEvent struct {
batchID [32]byte
amount *big.Int
Copy link
Member

Choose a reason for hiding this comment

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

value

}

// event BatchCreated(bytes32 indexed batchId, uint256 initialBalance, address owner, uint256 depth);
type batchCreatedEvent struct {
Copy link
Member

Choose a reason for hiding this comment

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

event BatchCreated(bytes32 indexed batchId, uint256 totalAmount, uint256 normalisedBalance, address owner, uint8 depth);

event BatchTopUp(bytes32 indexed batchId, uint256 topupAmount, uint256 normalisedBalance);

event BatchDepthIncrease(bytes32 indexed batchId, uint8 newDepth, uint256 normalisedBalance);

event PriceUpdate(uint256 price, uint256 totalOutPayment)

Copy link
Member

Choose a reason for hiding this comment

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

in. genergal all nornalisedBalance -> Value

@zelig zelig mentioned this pull request Dec 12, 2020
Copy link
Contributor

@paxthemax paxthemax left a comment

Choose a reason for hiding this comment

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

First pass review of batch, focusing on events next.

Code looks good, clean, well organized. Minor points to consider addressing.

ID []byte // batch ID
Value *big.Int // overall balance of the batch
Start uint64 // block number the batch was created
Owner []byte // owner's ethereum address
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use common.Address from go-ethereum here?


// MarshalBinary serialises a postage batch to a byte slice len 117.
func (b *Batch) MarshalBinary() ([]byte, error) {
out := make([]byte, 93)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we declare some of these values as constants? Perhaps export them?

)

// Batch represents a postage batch, a payment on the blockchain.
type Batch struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding an Eq function to compare?

if err := b.UnmarshalBinary(buf); err != nil {
t.Fatalf("unexpected error unmarshalling batch: %v", err)
}
if !bytes.Equal(b.ID, a.ID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use an Eq function here, see above.

@@ -0,0 +1 @@
package batchstore
Copy link
Contributor

Choose a reason for hiding this comment

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

File is empty, consider removing?

func (s *Store) UnmarshalBinary(buf []byte) error {
s.block = binary.BigEndian.Uint64(buf[:8])
totalLen := int(buf[8])
if totalLen > math.MaxUint8 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

"github.com/ethersphere/bee/pkg/statestore/mock"
)

func TestStore(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty test

t.Fatal(err)
}
if store2.Price().Cmp(newPrice) != 0 {
t.Fatal("price not persisted")
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider rewriting as price: got %v, want %v.


expTotal := big.NewInt(0 + 99*1100)
if store2.Total().Cmp(expTotal) != 0 {
t.Fatal("value mismatch")
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider rewriting as value: got %v, want %v.

t.Fatal(err)
}

newPrice := big.NewInt(99)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a line of comment as to what we are trying to achieve here? As in: creating the store, changing the price, expecting different result.

@paxthemax paxthemax self-requested a review December 14, 2020 16:38
@Eknir
Copy link
Contributor

Eknir commented Dec 15, 2020

Connected this issue with #928. @acud , lmk if this is good (or not)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress ongoing development , hold out with review pull-request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants