Crosschain enhanced: Arbitrum mainnet + util (getDepositStatus) #825
Conversation
There was a problem hiding this comment.
Thanks for the contribution! I left one small request and one question.
Let's not block this PR but for the future ones, keep in mind the commit message recommendation sections in the CONTRIBUTING documentation. As an example, we do not use [FEAT] prefix in this repository and prefer a bit longer explanation of why. :)
| "byzantium": true | ||
| }, | ||
| "numDeployments": 1, | ||
| "implementation": "0xFf79fca71751A5A0C4487a1aCE268d6cd2A64Db1", |
There was a problem hiding this comment.
Not blocking this PR but I see the implementation contract has one difference compared to L1BitcoinDepositor we have on main: it additionally extends Initializable. What was the reasoning behind it and can we port this back to main - in a separate PR - to have those two versions aligned?
There was a problem hiding this comment.
These artifacts are directly received from @lrsaturnino, I don't know exactly what the difference will be.
Anyway, different artifacts are used for each network.
I don't know if this will be a problem, maybe Leonardo can provide more information.
There was a problem hiding this comment.
Definitely not a problem for this PR but I wanted to clarify it and make sure what's deployed is reflected in the codebase. I will leave the discussion open so that we can hear from @lrsaturnino.
There was a problem hiding this comment.
@pdyraga It's not strictly necessary since OwnableUpgradeable already inherits from Initializable. We added it to make the use of the initializer pattern more explicit and to future-proof the contract against potential changes in the inheritance chain.
The new function getDepositStatus is changed to getDepositState to match the smart contract nomenclature. The documentation is also updated.
It's true! Sorry, I just saw the new contribution documentation. I will keep it in mind for the next ones. |
This is something I missed during the review of PR #825 adding the getDepositStatus function. The version should stay on 2.5.0-dev as this is the -dev version automatically pushed to the NPM registry and the last published non-dev version was 2.4.1. The next published non-dev release should be 2.5.0. I have also sneaked in docs regenerate change to fix the problem with a link that got updated in a fork with changes merged to this repository in #825 and #822.
|
@JSanchezFDZ A new version was published to NPM registry: https://www.npmjs.com/package/@keep-network/tbtc-v2.ts/v/2.5.0-dev.8. This version includes the changes in this PR - I had to downgrade the package version number in #826. |
|
@pdyraga Thanks for the information and sorry for the inconvenience of the indirect changes from working on a fork. |
Description
Type of change
Checks