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 data in genesis #3062

Open
wants to merge 27 commits into
base: future/peggy2
Choose a base branch
from
Open

add data in genesis #3062

wants to merge 27 commits into from

Conversation

juniuszhou
Copy link
Contributor

No description provided.

@juniuszhou juniuszhou self-assigned this Jul 25, 2022
@juniuszhou juniuszhou added the Peggy Team Peggy team task label Jul 25, 2022
@codecov
Copy link

codecov bot commented Jul 25, 2022

Codecov Report

Merging #3062 (cef49c5) into future/peggy2 (ef044b7) will increase coverage by 0.22%.
The diff coverage is 90.18%.

Impacted file tree graph

@@                Coverage Diff                @@
##           future/peggy2    #3062      +/-   ##
=================================================
+ Coverage          47.96%   48.18%   +0.22%     
=================================================
  Files                169      169              
  Lines              15024    15120      +96     
=================================================
+ Hits                7206     7286      +80     
- Misses              7385     7395      +10     
- Partials             433      439       +6     
Impacted Files Coverage Δ
x/ethbridge/keeper/ethereumLockBurnSequence.go 90.32% <86.04%> (-9.68%) ⬇️
x/ethbridge/keeper/globalSequence.go 91.66% <91.66%> (ø)

@banshee
Copy link
Contributor

banshee commented Jul 25, 2022

Can we switch

func (k Keeper) getKeyViaNetworkDescriptorGlobalNonce(networkDescriptor types.NetworkDescriptor,
	globalSequence uint64) []byte {
	bs1 := make([]byte, 4)
	binary.LittleEndian.PutUint32(bs1, uint32(networkDescriptor))

	bs2 := make([]byte, 8)
	binary.LittleEndian.PutUint64(bs2, globalSequence)

	storeKey := append(append(types.GlobalNonceProphecyIDPrefix, bs1[:]...), bs2[:]...)
	return storeKey
}

to also use bigendian? If we use bigendian here, there's no littleendian in our code at all.

@juniuszhou
Copy link
Contributor Author

Can we switch

func (k Keeper) getKeyViaNetworkDescriptorGlobalNonce(networkDescriptor types.NetworkDescriptor,
	globalSequence uint64) []byte {
	bs1 := make([]byte, 4)
	binary.LittleEndian.PutUint32(bs1, uint32(networkDescriptor))

	bs2 := make([]byte, 8)
	binary.LittleEndian.PutUint64(bs2, globalSequence)

	storeKey := append(append(types.GlobalNonceProphecyIDPrefix, bs1[:]...), bs2[:]...)
	return storeKey
}

to also use bigendian? If we use bigendian here, there's no littleendian in our code at all.

yes, it is better to use bigendian. will update it.

@smartyalgo
Copy link
Contributor

Curious, why is bigEndian better?

@juniuszhou
Copy link
Contributor Author

Curious, why is bigEndian better?

just use the same endian in the code, I have other PR #2748 to use the protobuf. I think it is better solution, can avoid some mistake for different endian usage.

message GenesisState { string crosschain_fee_receive_account = 1; }
message GenesisState {
string crosschain_fee_receive_account = 1;
map<string, uint64> ethereum_lock_burn_sequence = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to include ethereum_lock_burn_sequence in genesis?
If the entry doesn't exist in keeper, keeper returns 0, which is our desired behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. if ethereum_lock_burn_sequence not set before, then no data will be included in genesis, it doesn't generate any empty/default value
  2. 0 is expected if no vallue exists, to avoid such confusion, we set the lockBurnSequence starting from 1, also in contract

@@ -62,4 +62,9 @@ enum ClaimType {
}

// GenesisState for ethbridge
message GenesisState { string crosschain_fee_receive_account = 1; }
message GenesisState {
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to update x/ethbridge/genesis.go::ValidateGenesis function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CrosschainFeeReceiveAccount already checked, for other three items, they are map, so that they are empty is correct if no any data before in peggy1.0. Do you mean we should check each entry in these maps?

}

// SetSequenceViaRawKey used in import sequence from genesis
func (k Keeper) SetSequenceViaRawKey(ctx sdk.Context, key []byte, newSequence uint64) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks very error prone.
I suggest write a function to extract NetworkDescriptor and ValidatorAddress, and use those to call SetEthereumLockBurnSequence

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will add extract method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these comments still current?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented a new method DecodeKey to extract NetworkDescriptor and ValidatorAddress according to comment. please check.

@juniuszhou juniuszhou requested a review from smartyalgo August 20, 2022 09:36
@juniuszhou
Copy link
Contributor Author

need deal with map issue first.

@juniuszhou juniuszhou marked this pull request as ready for review October 12, 2022 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Peggy Team Peggy team task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants