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.

Sign up for GitHub

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

Jump to bottom

[staking] ReadState API support Candidate ID #4276

Merged
merged 9 commits into from
Jun 14, 2024
Merged

[staking] ReadState API support Candidate ID #4276

merged 9 commits into from
Jun 14, 2024

Conversation

envestcc
Copy link
Member

@envestcc envestcc commented May 23, 2024 •
edited
Loading

Description

Due to the introduction of a new ID as a unique identifier by the Candidate, the following modifications need to be made to the API:

  • Upgrade the candidateByAddress, candidateByName, and candidates APIs to v3, adding 'id' to the returned results.
  • Add a new API called candidateByID; the original meaning of the candidateByAddress is to query based on the owner's address.

It's based on iotexproject/iotex-proto#151

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • [] make test
  • [] fullsync
  • [] Other test (please specify)

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

  • [] My code follows the style guidelines of this project
  • [] I have performed a self-review of my code
  • [] I have commented my code, particularly in hard-to-understand areas
  • [] I have made corresponding changes to the documentation
  • [] My changes generate no new warnings
  • [] I have added tests that prove my fix is effective or that my feature works
  • [] New and existing unit tests pass locally with my changes
  • [] Any dependent changes have been merged and published in downstream modules

@dustinxie
Copy link
Member

can you rebase this PR? i think lots of code has already checked in

@envestcc envestcc force-pushed the delegate_transfer_ownership_5_cc branch from a25cf86 to 5ee2ecf Compare June 6, 2024 04:30
@envestcc envestcc marked this pull request as ready for review June 6, 2024 04:32
@envestcc envestcc requested review from CoderZhi, dustinxie, Liuhaai and a team as code owners June 6, 2024 04:32
dustinxie
dustinxie reviewed Jun 7, 2024
View reviewed changes
action/protocol/staking/candidate_statereader.go Outdated
if err != nil {
return nil, 0, err
}
cand = c.GetByIdentifier(id)
Copy link
Member

Choose a reason for hiding this comment

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

is it possible Id and OwnerAddress both exist? if yes, which one takes precedence?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated, now if both conditions are set, it means that they need to be fulfilled at mean while.

dustinxie
dustinxie reviewed Jun 7, 2024
View reviewed changes
action/protocol/staking/ethabi/common/candidatebyaddress.go
@@ -76,7 +76,7 @@ func (r *CandidateByAddressStateContext) EncodeToEth(resp *iotexapi.ReadStateRes

data, err := r.Method.Outputs.Pack(cand)
if err != nil {
return "", nil
return "", err
Copy link
Member

@dustinxie dustinxie Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

looks like this is a typo/bug in current code? we don't have a test for this? add one if not

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, existed typo, but it's not easy to cause error for abi pack

@CoderZhi
Copy link
Collaborator

CoderZhi commented Jun 7, 2024

As discussed offline, let's change the logic of creating delegate to:

if delegateByOwner(creator) != nil {
    return duplicate error
}
id := creator
while delegateByIdentifier(id) != nil { // up to N = 1000 times
    id = hash(id)
}
create delegate with id

such that an account who doesn't own a delegate could always create a delegate.

Make sure the scenario works in this PR.

@envestcc envestcc force-pushed the delegate_transfer_ownership_5_cc branch from 335baa3 to b535120 Compare June 11, 2024 15:26
@dustinxie
Copy link
Member

Member

As discussed offline, let's change the logic of creating delegate to:

if delegateByOwner(creator) != nil {
    return duplicate error
}
id := creator
while delegateByIdentifier(id) != nil { // up to N = 1000 times
    id = hash(id)
}
create delegate with id

such that an account who doesn't own a delegate could always create a delegate.

Make sure the scenario works in this PR.

agreed, discussed offline we can use logic similar to id = hash(address, blockheight)

@envestcc envestcc force-pushed the delegate_transfer_ownership_5_cc branch from c1d6b3f to 78350fc Compare June 13, 2024 03:00
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jun 13, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
24.9% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

dustinxie
dustinxie reviewed Jun 13, 2024
View reviewed changes
action/protocol/staking/handlers.go
return owner, nil
}
h := append(owner.Bytes(), byteutil.Uint64ToBytesBigEndian(height)...)
for i := 0; i < 1000; i++ {
Copy link
Member

Choose a reason for hiding this comment

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

can you quickly test how much time this 1000 loop will take? I'm thinking 128 should be enough to generate a diff ID

Copy link
Member Author

@envestcc envestcc Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

goos: darwin
goarch: arm64
pkg: github.com/iotexproject/iotex-core/action/protocol/staking
BenchmarkLoop-10            2366            489271 ns/op           33397 B/op       1024 allocs/op

around 0.5ms, benchmark exclude query from csm. I think it's acceptable

CoderZhi
CoderZhi approved these changes Jun 13, 2024
View reviewed changes
action/protocol/staking/candidate_statereader.go
@@ -478,11 +478,28 @@ func (c *candSR) readStateCandidateByName(ctx context.Context, req *iotexapi.Rea
}

func (c *candSR) readStateCandidateByAddress(ctx context.Context, req *iotexapi.ReadStakingDataRequest_CandidateByAddress) (*iotextypes.CandidateV2, uint64, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

deprecate this api

@envestcc envestcc merged commit e107a78 into master Jun 14, 2024
2 of 3 checks passed
@envestcc envestcc deleted the delegate_transfer_ownership_5_cc branch June 14, 2024 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CoderZhi CoderZhi CoderZhi approved these changes

@dustinxie dustinxie dustinxie approved these changes

@Liuhaai Liuhaai Awaiting requested review from Liuhaai Liuhaai is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@envestcc @dustinxie @CoderZhi