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

Allow address inspection via the API #2184

Merged
merged 2 commits into from
Sep 25, 2020
Merged

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Sep 24, 2020

Issue Number

#2180

Overview

  • 2daae7e
    📍 implement new stateless endpoint for inspecting addresses
    It takes any arbitrary string as input and tries to deserialize it into some address information if possible, otherwise it fails with a proper error.

  • 76d13ba
    📍 add unit and integration tests for the new 'inspectAddress' function & endpoint

Comments

Screenshot from 2020-09-24 18-52-44

@KtorZ KtorZ added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Sep 24, 2020
@KtorZ KtorZ requested a review from paweljakubas September 24, 2020 16:50
@KtorZ KtorZ self-assigned this Sep 24, 2020
@KtorZ KtorZ force-pushed the KtorZ/2180/address-inspect-via-api branch 3 times, most recently from 74cf3b8 to 689bd2b Compare September 24, 2020 19:48
it "ADDRESS_INSPECT_01 - Address inspect OK" $ \ctx -> do
let str = "Ae2tdPwUPEYz6ExfbWubiXPB6daUuhJxikMEb4eXRp5oKZBKZwrbJ2k7EZe"
r <- request @Aeson.Value ctx (Link.inspectAddress str) Default Empty
expectResponseCode @IO HTTP.status200 r
Copy link
Contributor

Choose a reason for hiding this comment

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

any chance to also verify the result of this call here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes and no. It's technically possible of course. But I'll rather defer that to until we have a proper data-type for representing inspected data (as highlighted in a FIXME comment). But I'd rather introduce this data-type in cardano-addresses directly.

newtype ApiAddressInspectData = ApiAddressInspectData
{ unApiAddressInspectData :: Text }
deriving (Eq, Generic, Show)
deriving newtype (IsString)
Copy link
Contributor

Choose a reason for hiding this comment

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

could you comment why this was needed and used? (especially if we have Show above)

Copy link
Member Author

Choose a reason for hiding this comment

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

It allows for using the OverloadedString extension in a transparent fashion for literal strings like we do for Text and ByteString. So, one can write:

let inspectData = "patate"

instead of having to:

let inspectData = ApiAddressInspectData "patate"

@@ -1004,6 +1010,35 @@ instance DecodeAddress 'Mainnet where
instance DecodeAddress ('Testnet pm) where
decodeAddress = _decodeAddress SL.Testnet

decodeBytes :: Text -> Either TextDecodingError ByteString
Copy link
Contributor

Choose a reason for hiding this comment

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

we already have cardano-addresses dependency in this project https://github.com/input-output-hk/cardano-wallet/blob/master/stack.yaml#L48 and also have many bits present here waiting for reuse. Probably not in thi sPR but I am in favor of strong reuse of cardano-addresses primitives..

Copy link
Member Author

Choose a reason for hiding this comment

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

At some point yes, I agree. I started this PR with only cardano-addresses originally and I ended up reverting because errors were highly inconsistent between endpoints. So I prefer sticking to how it's done now (and also, locate the code doing it in the same place so it's either to change later).


where
toNetwork :: Byron.NetworkMagic -> SL.Network
toNetwork = \case
Byron.NetworkMainOrStage -> SL.Mainnet
Byron.NetworkTestnet{} -> SL.Testnet

-- FIXME: 'cardano-addresses' currently gives us an opaque 'Value'. It'd be
Copy link
Contributor

Choose a reason for hiding this comment

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

script_hash:
type: string
format: base16
minLength: 64
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, I think in 1.20.0 in cardano-node they use 28-byte hash for scripts, not 32-byte

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but cardano-addresses still uses 32 bytes :( ... So we need to change it there, bump revision and update here. Which was on my plate today ^^

Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

LGTM! I do think we should reconsider better reuse of lean cardano-addresses library in the future

@KtorZ
Copy link
Member Author

KtorZ commented Sep 25, 2020

bors merge

iohk-bors bot added a commit that referenced this pull request Sep 25, 2020
2138: Save intermediate state when restoring the Byron chain in the stake pools worker r=hasufell a=hasufell

This completely skips byron era for mainnet when syncing stake pools.

I extracted the first shelley block by looking at a fully synced stake pool DB and selecting the lowest `block_height` from the `pool_production` table.

For non-mainnet it will record the last byron block of a chunk from the node into the `byron_headers` table and read it out during `readPoolProductionCursor` if there are no stake pools and otherwise ignore it.

Remarks:

- the "genesis shelley header" is hardcoded, because the node currently does not have an endpoint to get this

2184: Allow address inspection via the API r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#2180 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- 2daae7e
  📍 **implement new stateless endpoint for inspecting addresses**
    It takes any arbitrary string as input and tries to deserialize it into some address information if possible, otherwise it fails with a proper error.

- 76d13ba
  📍 **add unit and integration tests for the new 'inspectAddress' function & endpoint**

# Comments

<!-- Additional comments or screenshots to attach if any -->

![Screenshot from 2020-09-24 18-52-44](https://user-images.githubusercontent.com/5680256/94175596-2b686480-fe97-11ea-8325-66fa41c5577e.png)


<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Julian Ospald <[email protected]>
Co-authored-by: KtorZ <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 25, 2020

Build failed (retrying...):

Timed out, but:

 STAKE_POOLS_JOIN_01 - Cannot join existent stakepool with wrong password FAILED [1]
    STAKE_POOLS_JOIN_01rewards - Can join a pool, earn rewards and collect them FAILED [2]
    STAKE_POOLS_JOIN_02 - Cannot join already joined stake pool FAILED [3]
    STAKE_POOLS_JOIN_03 - Cannot join a pool that has retired FAILED [4]

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 25, 2020

Merge conflict.

  It takes any arbitrary string as input and tries to deserialize it into some address information if possible, otherwise it fails with a proper error.
@KtorZ KtorZ force-pushed the KtorZ/2180/address-inspect-via-api branch from 689bd2b to 49e5000 Compare September 25, 2020 13:04
@KtorZ
Copy link
Member Author

KtorZ commented Sep 25, 2020

bors r+

iohk-bors bot added a commit that referenced this pull request Sep 25, 2020
2184: Allow address inspection via the API r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#2180 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- 2daae7e
  📍 **implement new stateless endpoint for inspecting addresses**
    It takes any arbitrary string as input and tries to deserialize it into some address information if possible, otherwise it fails with a proper error.

- 76d13ba
  📍 **add unit and integration tests for the new 'inspectAddress' function & endpoint**

# Comments

<!-- Additional comments or screenshots to attach if any -->

![Screenshot from 2020-09-24 18-52-44](https://user-images.githubusercontent.com/5680256/94175596-2b686480-fe97-11ea-8325-66fa41c5577e.png)


<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <[email protected]>
@KtorZ
Copy link
Member Author

KtorZ commented Sep 25, 2020

CI stuck on the mac-mini again. Moving forward with that one :(

@KtorZ KtorZ merged commit e324626 into master Sep 25, 2020
@KtorZ KtorZ deleted the KtorZ/2180/address-inspect-via-api branch September 25, 2020 14:47
@KtorZ
Copy link
Member Author

KtorZ commented Sep 25, 2020

bors r-

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 25, 2020

Canceled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants