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: API: Implement StateLookupRobustAddress #8486

Merged
merged 1 commit into from
Apr 19, 2022

Conversation

geoff-vball
Copy link
Contributor

Related Issues

Fixes #5371

Proposed Changes

added endpoint StateLoockupRobustAddress which will return the robust address corresponding to any ID Address

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • All commits have a clear commit message.
  • The PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
    • area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
  • This PR has tests for new functionality or change in behaviour
  • If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
  • CI is green

@geoff-vball geoff-vball requested a review from a team as a code owner April 14, 2022 01:45
@geoff-vball geoff-vball force-pushed the gstuart/robust-address-endpoint branch 2 times, most recently from 1960c87 to f0b2713 Compare April 14, 2022 02:16
StateAccountKey(context.Context, address.Address, types.TipSetKey) (address.Address, error) //perm:read
// StateLookupRobustAddress returns the public key address of the given ID address for non-account addresses (multisig, miners etc)
StateLookupRobustAddress(context.Context, address.Address, types.TipSetKey) (address.Address, error) //perm:read
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the implementation this for sure is slow due to data structures. Do we want to allow anyone (including for example browser) to call this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Kubuxu Our thought was that it should be treated roughly the same as StateCompute which is perm: read, and not in the gateway. We can bump the perm for sure, though, I think the use cases are very few for this (but some people really want it).

Copy link
Contributor

@Kubuxu Kubuxu Apr 14, 2022

Choose a reason for hiding this comment

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

It would be best to bump the StateCompute perm as well (we could add another permission slow or something).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's a good idea, but I think it'll be a future thing.

@arajasek arajasek force-pushed the gstuart/robust-address-endpoint branch from f0b2713 to ed6e840 Compare April 14, 2022 15:43
chain/stmgr/stmgr.go Outdated Show resolved Hide resolved
if err != nil {
return address.Undef, xerrors.Errorf("loading tipset %s: %w", tsk, err)
}
if ts.Height() > policy.ChainFinality {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, thinking about this some more, we aren't actually using a finality lookback when resolving Accounts in StateAccountKey. While I think we probably should be, this method should just match behaviour there, so I'd actually drop the finality lookback here (in contrast to what I said we should do in the issue).

itests/self_sent_txn_test.go Outdated Show resolved Hide resolved
@@ -102,3 +102,23 @@ func TestSelfSentTxnV14(t *testing.T) {
require.NoError(t, err)
require.Equal(t, exitcode.Ok, mLookup.Receipt.ExitCode)
}

func TestStateLookupRobustAddress(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.

This test should move into its own file.

api/api_gateway.go Outdated Show resolved Hide resolved
return nil
})
if err != nil {
if robustAddr == address.Undef {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this check to before the error check -- assume that if robustAddr got set, you don't have an error (in contrast to actual correct error handling).

@geoff-vball geoff-vball force-pushed the gstuart/robust-address-endpoint branch from af73a17 to dc92302 Compare April 14, 2022 16:10
@codecov
Copy link

codecov bot commented Apr 14, 2022

Codecov Report

Merging #8486 (49d6c07) into master (06f8637) will decrease coverage by 0.05%.
The diff coverage is 54.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8486      +/-   ##
==========================================
- Coverage   40.80%   40.74%   -0.06%     
==========================================
  Files         686      686              
  Lines       75501    75532      +31     
==========================================
- Hits        30808    30776      -32     
- Misses      39378    39430      +52     
- Partials     5315     5326      +11     
Impacted Files Coverage Δ
api/api_full.go 57.89% <ø> (ø)
chain/stmgr/stmgr.go 63.50% <53.84%> (-1.45%) ⬇️
node/impl/full/state.go 34.97% <60.00%> (+0.27%) ⬆️
itests/kit/blockminer.go 70.65% <0.00%> (-17.94%) ⬇️
cli/util.go 70.83% <0.00%> (-8.34%) ⬇️
chain/exchange/peer_tracker.go 66.66% <0.00%> (-4.31%) ⬇️
storage/wdpost_sched.go 77.45% <0.00%> (-3.93%) ⬇️
node/hello/hello.go 63.63% <0.00%> (-3.41%) ⬇️
blockstore/buffered.go 34.44% <0.00%> (-2.23%) ⬇️
extern/storage-sealing/currentdealinfo.go 72.97% <0.00%> (-1.81%) ⬇️
... and 18 more

@geoff-vball geoff-vball force-pushed the gstuart/robust-address-endpoint branch from 4beb4ba to be69abc Compare April 14, 2022 21:32
@geoff-vball geoff-vball force-pushed the gstuart/robust-address-endpoint branch from be69abc to 49d6c07 Compare April 14, 2022 21:41
@arajasek arajasek changed the title Implemented StateLoockupRobustAddress Implemented StateLookupRobustAddress Apr 14, 2022
})
if robustAddr == address.Undef {
if err == nil {
return address.Undef, xerrors.Errorf("Address %s not found", idAddr.String())
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
return address.Undef, xerrors.Errorf("Address %s not found", idAddr.String())
return address.Undef, xerrors.Errorf("Address %d not found", idAddr)

@geoff-vball geoff-vball changed the title Implemented StateLookupRobustAddress Feat: API: Implement StateLookupRobustAddress Apr 19, 2022
@geoff-vball geoff-vball merged commit a3a3fef into master Apr 19, 2022
@geoff-vball geoff-vball deleted the gstuart/robust-address-endpoint branch April 19, 2022 15:37
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.

unknown actor code bafkqaetgnfwc6mrpon2g64tbm5sw22lomvza
3 participants