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

Wrap ETH address handling iniside sdk #445

Merged
merged 21 commits into from
Jun 11, 2024

Conversation

r-czajkowski
Copy link
Contributor

@r-czajkowski r-czajkowski commented Jun 4, 2024

Closes: #426
Closes: #428

This PR wraps the ETH address handling inside the SDK and refactores some modules.

Here we remove the Staking module and split it into 2 separate modules Account and Protocol. The Account module exposes features related to a given Bitcoin account such as initializing deposits, fetching balance etc. The Protocol module provides general information related to the Acre protocol such as fees, minimum deposit/withdrawal amount, tvl etc.

Implement the `toAcreIdentifier` function that returns the deposit
owner/user identifier in the Acre protocol based on the bitcoin address.
The function saves calculated identifier in cache so we do not need to
recalculate it.
And update the `sharesBalance` and `estimatedBitcoinBalance` function to
rely on the bitcoin address only. The consumer should operate with a
Bitcoin address only. SDK should handle conversion from Bitcoin to EVM
identifier behind the scenes.
@r-czajkowski r-czajkowski added the 🔌 SDK TypeScript SDK Library label Jun 4, 2024
@r-czajkowski r-czajkowski requested a review from nkuba June 4, 2024 13:15
@r-czajkowski r-czajkowski self-assigned this Jun 4, 2024
Copy link

netlify bot commented Jun 4, 2024

Deploy Preview for acre-dapp-testnet ready!

Name Link
🔨 Latest commit 18e1fed
🔍 Latest deploy log https://app.netlify.com/sites/acre-dapp-testnet/deploys/6668438efc5efe000874628c
😎 Deploy Preview https://deploy-preview-445--acre-dapp-testnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

We no longer need to pass the chain identifier to the `sharesBalance`
and `estimatedBitcoinBalance` - the SDK relies on the bitcoin provider
implementation passed during the SDK initialization.
Do not export the `ethereum` lib components from the root `index.ts`
file - we should keep the Ethereum-related stuff under the hood inside
the SDK.
@r-czajkowski r-czajkowski added the 🎨 dApp dApp label Jun 4, 2024
The `EthereumNetwork` is no longer available in the `@acre/sdk` package.
The dapp should rely on the Bitcoin network only.
In the `Account` module we want to expose the features related to a
given bitcoin account.
Require the bitcoin address and acre identifier in consructor. We can
calculate the acre identifier only once in `Acre.initialize` function
and pass it to the module.
Extract general protocol functions to a separate module. This module
should provide general functions related to the Acre protocol such as:
fees, min deposit/withdrawal amount, tvl etc.
@r-czajkowski r-czajkowski marked this pull request as ready for review June 6, 2024 09:48
@r-czajkowski r-czajkowski requested a review from kkosiorowska June 6, 2024 09:48
sdk/src/modules/account/index.ts Outdated Show resolved Hide resolved
sdk/src/modules/account/index.ts Outdated Show resolved Hide resolved
sdk/src/modules/account/index.ts Outdated Show resolved Hide resolved
sdk/src/modules/account/index.ts Outdated Show resolved Hide resolved
sdk/src/modules/account/index.ts Outdated Show resolved Hide resolved
sdk/src/modules/protocol/index.ts Outdated Show resolved Hide resolved
sdk/src/modules/account/index.ts Outdated Show resolved Hide resolved
sdk/src/modules/staking/index.ts Outdated Show resolved Hide resolved
sdk/src/acre.ts Outdated Show resolved Hide resolved
sdk/src/index.ts Show resolved Hide resolved
Sine we resolve the addresses just once on the SDK initialization we can
remove this component.
Since some of the dirs contain only `index.ts` file we move them one
level higher with name of the module.
Add info about the precision of the returned value.
We should be consistent so we want to return values related to the
Bitcoin in satoshi precision (1e8).
We renamed the method `minDepositAmount` to `minimumDepositAmount` and
here we update the dapp part.
@r-czajkowski r-czajkowski requested a review from nkuba June 6, 2024 14:10
Copy link
Contributor

@kkosiorowska kkosiorowska left a comment

Choose a reason for hiding this comment

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

I am not sure why, but when I try to do staking I get the following error.

Screenshot 2024-06-10 at 10 33 15

We should add `0x` prefix to the address when creating the `VoidSigner`
instance.
@r-czajkowski
Copy link
Contributor Author

I am not sure why, but when I try to do staking I get the following error.
Screenshot 2024-06-10 at 10 33 15

Fixed in db5fdde.

)

const signer = new VoidSigner(depositOwnerEvmAddress, ethersProvider)
const signer = new VoidSigner(
`0x${accountEthereumAddress.identifierHex}`,
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the complexities ChainIdentifier and EthereumAddress types introduce (e.g. the issue in #445 (review)) I'm thinking about using plain string to deal with addresses and convert it to EthereumAddress only there where tBTC SDK requires it.
WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

It is not blocking this PR. We can handle that separately.

Copy link
Contributor Author

@r-czajkowski r-czajkowski Jun 11, 2024

Choose a reason for hiding this comment

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

Yeah but that's true. But I think we can add an alias for the EthereumAddress eg. type EthereumAddress = 0x${string} or type EthereumAddress = string IMO it improves the readability. WDYT? Anyway, I think we should address it in a separate PR because it requires a lot of changes.

sdk/src/modules/account.ts Show resolved Hide resolved
sdk/src/modules/account.ts Outdated Show resolved Hide resolved
sdk/src/modules/account.ts Outdated Show resolved Hide resolved
sdk/src/modules/account.ts Outdated Show resolved Hide resolved
sdk/src/modules/account.ts Show resolved Hide resolved
sdk/src/modules/protocol.ts Outdated Show resolved Hide resolved
@nkuba nkuba requested a review from kkosiorowska June 10, 2024 15:30
nkuba added 2 commits June 11, 2024 13:54
Here we expose in the SDK the totalAssets function from stBTC contract.
The function returns total value of tBTC under Acre protocol management.
This reverts commit 0493060.
It was pushed by accident to the wrong branch.
@nkuba nkuba mentioned this pull request Jun 11, 2024
@nkuba nkuba enabled auto-merge June 11, 2024 13:20
@nkuba nkuba merged commit ab366a9 into main Jun 11, 2024
24 checks passed
@nkuba nkuba deleted the wrap-eth-address-handling-iniside-sdk branch June 11, 2024 13:20
r-czajkowski added a commit that referenced this pull request Jun 11, 2024
Depends on #445

In this PR we expose in the SDK the totalAssets function from stBTC
contract.
The function returns the total value of tBTC under Acre protocol
management.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎨 dApp dApp 🔌 SDK TypeScript SDK Library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store deposit owner EVM address in SDK context Wrap Ethereum address handling inside the SDK
3 participants