Skip to content

Fix new Nitro changes#98

Closed
gzeoneth wants to merge 10 commits intoc-nitro-stablefrom
c-nitro-patch-1
Closed

Fix new Nitro changes#98
gzeoneth wants to merge 10 commits intoc-nitro-stablefrom
c-nitro-patch-1

Conversation

@gzeoneth
Copy link
Member

@gzeoneth gzeoneth commented Jun 24, 2022

OffchainLabs/nitro#623 Outbox replay state management
OffchainLabs/nitro#692 Refactor accumulator flow
OffchainLabs/nitro#697 Move address aliasing from L2 to L1
OffchainLabs/nitro#720 Expose isSpent in Outbox

Also fix a race condition in L1ToL2Message.receiptsToStatus

fredlacs and others added 5 commits June 20, 2022 13:57
* chore: move dependencies into dev dependencies

* refactor: move dotenv into dev dependency

* Add tsnode to dev dependency

* lint

* rename network id

* fix: breaking change on network config
@gzeoneth gzeoneth requested a review from yahgwai June 24, 2022 14:24
@cla-bot cla-bot bot added the cla-signed label Jun 24, 2022
@gzeoneth gzeoneth changed the title Fix calculateSubmitRetryableId after nitro#697 Fix new Nitro changes Jun 24, 2022
@yahgwai yahgwai changed the base branch from c-nitro-next to c-nitro-stable June 27, 2022 09:45
@yahgwai yahgwai changed the base branch from c-nitro-stable to c-nitro-next June 27, 2022 09:46
Copy link
Contributor

@yahgwai yahgwai left a comment

Choose a reason for hiding this comment

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

We need to PR this against c-nitro-stable. We could merge it into c-nitro-next and then cherry-pick it over to c-nitro-stable

"typechain": "7.0.0",
"@arbitrum/nitro-contracts": "1.0.0-beta.5",
"arb-bridge-peripherals": "1.0.10",
"@arbitrum/nitro-contracts": "https://gitpkg.now.sh/OffchainLabs/nitro/contracts?expose-outbox-isspent",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should release a version of the nitro contracts and include that 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.

// not redeemed, has it now expired
if (await this.isExpired()) {
try {
if (await this.isExpired()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should make isExpired atomic with getSuccessfulRedeem so that we always have consistent results for getSuccessfulRedeem.

We could adjust isExpired so that it has a try/catch internally, and rename it to retryableExists.

Then adjust getSuccessfulRedeem so that we call retryableExists somewhere at the start of it. If retryableExists is true then we can just return null straight away - since it hasnt been redeemed or expired. If retryableExists is false then we either successfully redeemed or we expired. We then look for a successful redeem, if we find it we return the tx receipt, if we dont we return some value that indicates an expiration.

Then we won't check isExpired/retryableExists in receiptToStatus, instead we just pass in the receipt, or the value that indicates expiration.

@fredlacs fredlacs changed the base branch from c-nitro-next to c-nitro-stable June 27, 2022 10:50
Comment on lines -130 to -132
const addressAlias = new Address(fromAddress)

const from = addressAlias.applyAlias()
Copy link
Contributor

Choose a reason for hiding this comment

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

sure we don't need to apply an alias here anymore?
If the user is supplying the from address as their L1 address it still needs to be aliased.

The difference is that now the alias is applied int he L1 inbox instead of by arb-os.
We should clarify what's expected in the fromAddress field. If its the value from the Inbox delivered event, it doesn't need aliasing. If its the plain user address, it needs alaising

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, calculateSubmitRetryableId is public static
I think we should just add some comment regarding the input expected.

@fredlacs
Copy link
Contributor

closed in favor of #100

@fredlacs fredlacs closed this Jun 27, 2022
@fredlacs fredlacs deleted the c-nitro-patch-1 branch July 7, 2022 10:37
umershaikh123 pushed a commit to umershaikh123/arbitrum-sdk that referenced this pull request May 20, 2024
…unning-support

Add local node support for tutorials
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants