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

feat: adapt to new go-sdk and greenfield version #47

Merged
merged 26 commits into from
Jun 25, 2023

Conversation

randyahx
Copy link
Contributor

Description

Update current code to adapt to dependencies
Refactor code to make it cleaner

Rationale

Multiple dependencies require a change on challenger to continue functioning

Example

Changes

Notable changes:

go.mod Outdated
github.com/btcsuite/btcd => github.com/btcsuite/btcd v0.23.0
github.com/cometbft/cometbft => github.com/bnb-chain/greenfield-cometbft v0.0.1
github.com/confio/ics23/go => github.com/cosmos/cosmos-sdk/ics23/go v0.8.0
github.com/cosmos/cosmos-sdk => github.com/bnb-chain/greenfield-cosmos-sdk v0.2.1
github.com/cosmos/cosmos-sdk => github.com/bnb-chain/greenfield-cosmos-sdk v0.2.2-alpha.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use the formal release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

go.mod Outdated
@@ -6,9 +6,9 @@ require (
cosmossdk.io/math v1.0.0
github.com/avast/retry-go/v4 v4.3.1
github.com/aws/aws-sdk-go v1.40.45
github.com/bnb-chain/greenfield v0.2.1
github.com/bnb-chain/greenfield v0.2.2-alpha.2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use the formal release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}
logging.Logger.Infof("latest attested challenge ids: %+v", challengeIds)
a.mtx.Lock()
a.updateAttestedCacheAndEventStatus(a.attestedChallengeIds, challengeIds)
Copy link
Collaborator

@forcodedancing forcodedancing Jun 23, 2023

Choose a reason for hiding this comment

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

In each call, it will return all the 300 latest attested challenges. We should use some cache to filter out the ids which had been written to database. Otherwise, there are lots of useless database write/read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

config/config.go Outdated
if !ok {
panic("error converting fee_amount to math.Int")
}
if feeAmount.IsNegative() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

better to check whether it is positive or not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

s.cachedEventHash = make(map[uint64][]byte, 0)
}

time.Sleep(TxSubmitLoopInterval)
Copy link
Collaborator

Choose a reason for hiding this comment

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

better to use ticker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}
time.Sleep(executor.QueryAttestedChallengeInterval)

time.Sleep(common.RetryInterval)
Copy link
Collaborator

Choose a reason for hiding this comment

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

actually, we can use the end time in res to determine how long we should wait.

continue
}
time.Sleep(50 * time.Millisecond)
time.Sleep(TxSubmitInterval)
Copy link
Collaborator

Choose a reason for hiding this comment

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

5s seems too long for TxSubmitInterval here.
meanwhile, we can also use attestPeriodEnd to jump out of the loop - by comparing current time and the end time of attestPeriodEnd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

submitLoopCount++
if submitLoopCount == common.CacheClearIterations {
submitLoopCount = 0
s.cachedEventHash = make(map[uint64][]byte, common.CacheSize)
s.cachedEventHash = make(map[uint64][]byte, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use a lru cache instead of map for caching.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@forcodedancing
Copy link
Collaborator

There are many changes. Self testing in dev environment is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants